| Message ID | 1383820884-29596-2-git-send-email-marcel.a@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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.
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.
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.
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. */
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(+)