Patchwork [for-1.7,v2,1/8] exec: declare TARGET_PHYS_ADDR_SPACE_MAX to limit memory regions rendered by exec

login
register
mail settings
Submitter Marcel Apfelbaum
Date Nov. 7, 2013, 10:41 a.m.
Message ID <1383820884-29596-2-git-send-email-marcel.a@redhat.com>
Download mbox | patch
Permalink /patch/289280/
State New
Headers show

Comments

Marcel Apfelbaum - Nov. 7, 2013, 10:41 a.m.
The page table logic in exec.c assumes
that memory addresses are at most TARGET_PHYS_ADDR_SPACE_BITS.
Use TARGET_PHYS_ADDR_SPACE_MAX as max size for memory regions
rendered by exec.

Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
---
 include/exec/address-spaces.h | 4 ++++
 1 file changed, 4 insertions(+)
Peter Maydell - Nov. 7, 2013, 10:49 a.m.
On 7 November 2013 10:41, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> The page table logic in exec.c assumes
> that memory addresses are at most TARGET_PHYS_ADDR_SPACE_BITS.
> Use TARGET_PHYS_ADDR_SPACE_MAX as max size for memory regions
> rendered by exec.
>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
>  include/exec/address-spaces.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
> index 3d12cdd..174cc05 100644
> --- a/include/exec/address-spaces.h
> +++ b/include/exec/address-spaces.h
> @@ -23,6 +23,10 @@
>
>  #ifndef CONFIG_USER_ONLY
>
> +#define TARGET_PHYS_ADDR_SPACE_MAX                         \
> +    (TARGET_PHYS_ADDR_SPACE_BITS == 64 ?                   \
> +    UINT64_MAX : (0x1ULL << TARGET_PHYS_ADDR_SPACE_BITS))
> +

I think it's worth adding a comment that this is a
size intended for use in memory_region_init() calls and
so follows the odd convention used by that API that
it is a size in bytes with the exception that UINT64_MAX
represents 2^64.

(it follows from this that using the #define anywhere
except in a memory_region_init() call is probably a bug)

-- PMM
Marcel Apfelbaum - Nov. 7, 2013, 11:32 a.m.
On Thu, 2013-11-07 at 10:49 +0000, Peter Maydell wrote:
> On 7 November 2013 10:41, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> > The page table logic in exec.c assumes
> > that memory addresses are at most TARGET_PHYS_ADDR_SPACE_BITS.
> > Use TARGET_PHYS_ADDR_SPACE_MAX as max size for memory regions
> > rendered by exec.
> >
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  include/exec/address-spaces.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
> > index 3d12cdd..174cc05 100644
> > --- a/include/exec/address-spaces.h
> > +++ b/include/exec/address-spaces.h
> > @@ -23,6 +23,10 @@
> >
> >  #ifndef CONFIG_USER_ONLY
> >
> > +#define TARGET_PHYS_ADDR_SPACE_MAX                         \
> > +    (TARGET_PHYS_ADDR_SPACE_BITS == 64 ?                   \
> > +    UINT64_MAX : (0x1ULL << TARGET_PHYS_ADDR_SPACE_BITS))
> > +
> 
> I think it's worth adding a comment that this is a
> size intended for use in memory_region_init() calls and
> so follows the odd convention used by that API that
> it is a size in bytes with the exception that UINT64_MAX
> represents 2^64.
I will add the comment in v3.

Thanks,
Marcel

> 
> (it follows from this that using the #define anywhere
> except in a memory_region_init() call is probably a bug)
> 
> -- PMM
Michael S. Tsirkin - Nov. 7, 2013, 12:04 p.m.
On Thu, Nov 07, 2013 at 10:49:50AM +0000, Peter Maydell wrote:
> On 7 November 2013 10:41, Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> > The page table logic in exec.c assumes
> > that memory addresses are at most TARGET_PHYS_ADDR_SPACE_BITS.
> > Use TARGET_PHYS_ADDR_SPACE_MAX as max size for memory regions
> > rendered by exec.
> >
> > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> > ---
> >  include/exec/address-spaces.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
> > index 3d12cdd..174cc05 100644
> > --- a/include/exec/address-spaces.h
> > +++ b/include/exec/address-spaces.h
> > @@ -23,6 +23,10 @@
> >
> >  #ifndef CONFIG_USER_ONLY
> >
> > +#define TARGET_PHYS_ADDR_SPACE_MAX                         \
> > +    (TARGET_PHYS_ADDR_SPACE_BITS == 64 ?                   \
> > +    UINT64_MAX : (0x1ULL << TARGET_PHYS_ADDR_SPACE_BITS))
> > +
> 
> I think it's worth adding a comment that this is a
> size intended for use in memory_region_init() calls and
> so follows the odd convention used by that API that
> it is a size in bytes with the exception that UINT64_MAX
> represents 2^64.
> 
> (it follows from this that using the #define anywhere
> except in a memory_region_init() call is probably a bug)
> 
> -- PMM

BTW how about we change the API to pass in int128?
Not for 1.7 of course.

This will help make sure it's only used for MRs.
Paolo Bonzini - Nov. 7, 2013, 1:49 p.m.
Il 07/11/2013 13:04, Michael S. Tsirkin ha scritto:
>> > (it follows from this that using the #define anywhere
>> > except in a memory_region_init() call is probably a bug)
>> > 
>> > -- PMM
> 
> BTW how about we change the API to pass in int128?

The vast majority of memory regions have small sizes, it would add
boilerplate to wrap the size with an int128_make64 everywhere.

Paolo

> Not for 1.7 of course.
> 
> This will help make sure it's only used for MRs.
Michael S. Tsirkin - Nov. 7, 2013, 3:40 p.m.
On Thu, Nov 07, 2013 at 02:49:06PM +0100, Paolo Bonzini wrote:
> Il 07/11/2013 13:04, Michael S. Tsirkin ha scritto:
> >> > (it follows from this that using the #define anywhere
> >> > except in a memory_region_init() call is probably a bug)
> >> > 
> >> > -- PMM
> > 
> > BTW how about we change the API to pass in int128?
> 
> The vast majority of memory regions have small sizes, it would add
> boilerplate to wrap the size with an int128_make64 everywhere.
> 
> Paolo

Well let's have two APIs for 64 bit and for 128 bit.

> > Not for 1.7 of course.
> > 
> > This will help make sure it's only used for MRs.

Patch

diff --git a/include/exec/address-spaces.h b/include/exec/address-spaces.h
index 3d12cdd..174cc05 100644
--- a/include/exec/address-spaces.h
+++ b/include/exec/address-spaces.h
@@ -23,6 +23,10 @@ 
 
 #ifndef CONFIG_USER_ONLY
 
+#define TARGET_PHYS_ADDR_SPACE_MAX                         \
+    (TARGET_PHYS_ADDR_SPACE_BITS == 64 ?                   \
+    UINT64_MAX : (0x1ULL << TARGET_PHYS_ADDR_SPACE_BITS))
+
 /* Get the root memory region.  This interface should only be used temporarily
  * until a proper bus interface is available.
  */