| Message ID | 1453340271-7417-1-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On Thu, 21 Jan 2016 12:37:51 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > The 'base' field of MemoryHotplugState is ram_addr_t, which indicates that > it exists in the abstract address space of RAM regions. > > However, the actual usage of this field indicates that it is a concrete > physical address (it's passed as an offset to memory_region_add_subgregion > for example). > > So, correct its type to 'hwaddr'. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > include/hw/mem/pc-dimm.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h > index d83bf30..218dfb0 100644 > --- a/include/hw/mem/pc-dimm.h > +++ b/include/hw/mem/pc-dimm.h > @@ -77,7 +77,7 @@ typedef struct PCDIMMDeviceClass { > * @mr: hotplug memory address space container > */ > typedef struct MemoryHotplugState { > - ram_addr_t base; > + hwaddr base; > MemoryRegion mr; > } MemoryHotplugState; > I agree with this fix but that's not the only place where ram_addr_t needs to be replaced with hwaddr. For example type of MachineState.[max]ram_size fields needs to be changed as well. Because QEMU builds without CONFIG_XEN_BACKEND on 32-bit hosts are broken since ram_addr_t is 32-bits there while some targets assume and use it as 64-bit one.
On 22/01/2016 11:02, Igor Mammedov wrote: > On Thu, 21 Jan 2016 12:37:51 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > >> The 'base' field of MemoryHotplugState is ram_addr_t, which indicates that >> it exists in the abstract address space of RAM regions. >> >> However, the actual usage of this field indicates that it is a concrete >> physical address (it's passed as an offset to memory_region_add_subgregion >> for example). >> >> So, correct its type to 'hwaddr'. >> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> >> --- >> include/hw/mem/pc-dimm.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h >> index d83bf30..218dfb0 100644 >> --- a/include/hw/mem/pc-dimm.h >> +++ b/include/hw/mem/pc-dimm.h >> @@ -77,7 +77,7 @@ typedef struct PCDIMMDeviceClass { >> * @mr: hotplug memory address space container >> */ >> typedef struct MemoryHotplugState { >> - ram_addr_t base; >> + hwaddr base; >> MemoryRegion mr; >> } MemoryHotplugState; >> > > I agree with this fix but that's not the only place where > ram_addr_t needs to be replaced with hwaddr. > For example type of MachineState.[max]ram_size fields needs > to be changed as well. Because QEMU builds without CONFIG_XEN_BACKEND > on 32-bit hosts are broken since ram_addr_t is 32-bits there > while some targets assume and use it as 64-bit one. But on a 32-bit system without CONFIG_XEN_BACKEND you cannot allocate more than 4G anyway, so the choice of ram_addr_t is understandable in that case. On the other hand, on a 32-bit system without CONFIG_XEN_BACKEND you definitely can place 128M of hot plugged memory between say 4096MB and 4224MB. Paolo
On Fri, 22 Jan 2016 15:21:05 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 22/01/2016 11:02, Igor Mammedov wrote: > > On Thu, 21 Jan 2016 12:37:51 +1100 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > >> The 'base' field of MemoryHotplugState is ram_addr_t, which indicates that > >> it exists in the abstract address space of RAM regions. > >> > >> However, the actual usage of this field indicates that it is a concrete > >> physical address (it's passed as an offset to memory_region_add_subgregion > >> for example). > >> > >> So, correct its type to 'hwaddr'. > >> > >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > >> --- > >> include/hw/mem/pc-dimm.h | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h > >> index d83bf30..218dfb0 100644 > >> --- a/include/hw/mem/pc-dimm.h > >> +++ b/include/hw/mem/pc-dimm.h > >> @@ -77,7 +77,7 @@ typedef struct PCDIMMDeviceClass { > >> * @mr: hotplug memory address space container > >> */ > >> typedef struct MemoryHotplugState { > >> - ram_addr_t base; > >> + hwaddr base; > >> MemoryRegion mr; > >> } MemoryHotplugState; > >> > > > > I agree with this fix but that's not the only place where > > ram_addr_t needs to be replaced with hwaddr. > > For example type of MachineState.[max]ram_size fields needs > > to be changed as well. Because QEMU builds without CONFIG_XEN_BACKEND > > on 32-bit hosts are broken since ram_addr_t is 32-bits there > > while some targets assume and use it as 64-bit one. > > But on a 32-bit system without CONFIG_XEN_BACKEND you cannot allocate > more than 4G anyway, so the choice of ram_addr_t is understandable in > that case. QEMU build will probably fail with above config but if it succeeds then maxmem will be silently truncated. > > On the other hand, on a 32-bit system without CONFIG_XEN_BACKEND you > definitely can place 128M of hot plugged memory between say 4096MB and > 4224MB. True. Anyway for this patch Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > Paolo >
On Fri, Jan 22, 2016 at 03:32:52PM +0100, Igor Mammedov wrote: > On Fri, 22 Jan 2016 15:21:05 +0100 > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 22/01/2016 11:02, Igor Mammedov wrote: > > > On Thu, 21 Jan 2016 12:37:51 +1100 > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > >> The 'base' field of MemoryHotplugState is ram_addr_t, which indicates that > > >> it exists in the abstract address space of RAM regions. > > >> > > >> However, the actual usage of this field indicates that it is a concrete > > >> physical address (it's passed as an offset to memory_region_add_subgregion > > >> for example). > > >> > > >> So, correct its type to 'hwaddr'. > > >> > > >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > >> --- > > >> include/hw/mem/pc-dimm.h | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h > > >> index d83bf30..218dfb0 100644 > > >> --- a/include/hw/mem/pc-dimm.h > > >> +++ b/include/hw/mem/pc-dimm.h > > >> @@ -77,7 +77,7 @@ typedef struct PCDIMMDeviceClass { > > >> * @mr: hotplug memory address space container > > >> */ > > >> typedef struct MemoryHotplugState { > > >> - ram_addr_t base; > > >> + hwaddr base; > > >> MemoryRegion mr; > > >> } MemoryHotplugState; > > >> > > > > > > I agree with this fix but that's not the only place where > > > ram_addr_t needs to be replaced with hwaddr. > > > For example type of MachineState.[max]ram_size fields needs > > > to be changed as well. Because QEMU builds without CONFIG_XEN_BACKEND > > > on 32-bit hosts are broken since ram_addr_t is 32-bits there > > > while some targets assume and use it as 64-bit one. > > > > But on a 32-bit system without CONFIG_XEN_BACKEND you cannot allocate > > more than 4G anyway, so the choice of ram_addr_t is understandable in > > that case. > QEMU build will probably fail with above config but if it succeeds > then maxmem will be silently truncated. > > > > > On the other hand, on a 32-bit system without CONFIG_XEN_BACKEND you > > definitely can place 128M of hot plugged memory between say 4096MB and > > 4224MB. > True. > > Anyway for this patch > Reviewed-by: Igor Mammedov <imammedo@redhat.com> Who needs to take this patch? I'm not sure if I need to do anything further to push it forwards.
On Mon, Feb 01, 2016 at 01:33:09PM +1100, David Gibson wrote: > On Fri, Jan 22, 2016 at 03:32:52PM +0100, Igor Mammedov wrote: > > On Fri, 22 Jan 2016 15:21:05 +0100 > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > On 22/01/2016 11:02, Igor Mammedov wrote: > > > > On Thu, 21 Jan 2016 12:37:51 +1100 > > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > >> The 'base' field of MemoryHotplugState is ram_addr_t, which indicates that > > > >> it exists in the abstract address space of RAM regions. > > > >> > > > >> However, the actual usage of this field indicates that it is a concrete > > > >> physical address (it's passed as an offset to memory_region_add_subgregion > > > >> for example). > > > >> > > > >> So, correct its type to 'hwaddr'. > > > >> > > > >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > >> --- > > > >> include/hw/mem/pc-dimm.h | 2 +- > > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > > >> > > > >> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h > > > >> index d83bf30..218dfb0 100644 > > > >> --- a/include/hw/mem/pc-dimm.h > > > >> +++ b/include/hw/mem/pc-dimm.h > > > >> @@ -77,7 +77,7 @@ typedef struct PCDIMMDeviceClass { > > > >> * @mr: hotplug memory address space container > > > >> */ > > > >> typedef struct MemoryHotplugState { > > > >> - ram_addr_t base; > > > >> + hwaddr base; > > > >> MemoryRegion mr; > > > >> } MemoryHotplugState; > > > >> > > > > > > > > I agree with this fix but that's not the only place where > > > > ram_addr_t needs to be replaced with hwaddr. > > > > For example type of MachineState.[max]ram_size fields needs > > > > to be changed as well. Because QEMU builds without CONFIG_XEN_BACKEND > > > > on 32-bit hosts are broken since ram_addr_t is 32-bits there > > > > while some targets assume and use it as 64-bit one. > > > > > > But on a 32-bit system without CONFIG_XEN_BACKEND you cannot allocate > > > more than 4G anyway, so the choice of ram_addr_t is understandable in > > > that case. > > QEMU build will probably fail with above config but if it succeeds > > then maxmem will be silently truncated. > > > > > > > > On the other hand, on a 32-bit system without CONFIG_XEN_BACKEND you > > > definitely can place 128M of hot plugged memory between say 4096MB and > > > 4224MB. > > True. > > > > Anyway for this patch > > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > Who needs to take this patch? I'm not sure if I need to do anything > further to push it forwards. Most pc-dimm changes were merged either by me or Michael. I can merge it if I get an Acked-by from Michael. Otherwise, Michael has my Acked-by if he wants to merge it through his tree. Acked-by: Eduardo Habkost <ehabkost@redhat.com>
On Tue, Feb 02, 2016 at 03:37:12PM -0200, Eduardo Habkost wrote: > On Mon, Feb 01, 2016 at 01:33:09PM +1100, David Gibson wrote: > > On Fri, Jan 22, 2016 at 03:32:52PM +0100, Igor Mammedov wrote: > > > On Fri, 22 Jan 2016 15:21:05 +0100 > > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > > > On 22/01/2016 11:02, Igor Mammedov wrote: > > > > > On Thu, 21 Jan 2016 12:37:51 +1100 > > > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > > > >> The 'base' field of MemoryHotplugState is ram_addr_t, which indicates that > > > > >> it exists in the abstract address space of RAM regions. > > > > >> > > > > >> However, the actual usage of this field indicates that it is a concrete > > > > >> physical address (it's passed as an offset to memory_region_add_subgregion > > > > >> for example). > > > > >> > > > > >> So, correct its type to 'hwaddr'. > > > > >> > > > > >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > > >> --- > > > > >> include/hw/mem/pc-dimm.h | 2 +- > > > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > > > >> > > > > >> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h > > > > >> index d83bf30..218dfb0 100644 > > > > >> --- a/include/hw/mem/pc-dimm.h > > > > >> +++ b/include/hw/mem/pc-dimm.h > > > > >> @@ -77,7 +77,7 @@ typedef struct PCDIMMDeviceClass { > > > > >> * @mr: hotplug memory address space container > > > > >> */ > > > > >> typedef struct MemoryHotplugState { > > > > >> - ram_addr_t base; > > > > >> + hwaddr base; > > > > >> MemoryRegion mr; > > > > >> } MemoryHotplugState; > > > > >> > > > > > > > > > > I agree with this fix but that's not the only place where > > > > > ram_addr_t needs to be replaced with hwaddr. > > > > > For example type of MachineState.[max]ram_size fields needs > > > > > to be changed as well. Because QEMU builds without CONFIG_XEN_BACKEND > > > > > on 32-bit hosts are broken since ram_addr_t is 32-bits there > > > > > while some targets assume and use it as 64-bit one. > > > > > > > > But on a 32-bit system without CONFIG_XEN_BACKEND you cannot allocate > > > > more than 4G anyway, so the choice of ram_addr_t is understandable in > > > > that case. > > > QEMU build will probably fail with above config but if it succeeds > > > then maxmem will be silently truncated. > > > > > > > > > > > On the other hand, on a 32-bit system without CONFIG_XEN_BACKEND you > > > > definitely can place 128M of hot plugged memory between say 4096MB and > > > > 4224MB. > > > True. > > > > > > Anyway for this patch > > > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > > > Who needs to take this patch? I'm not sure if I need to do anything > > further to push it forwards. > > Most pc-dimm changes were merged either by me or Michael. I can > merge it if I get an Acked-by from Michael. > > Otherwise, Michael has my Acked-by if he wants to merge it > through his tree. > > Acked-by: Eduardo Habkost <ehabkost@redhat.com> Ah, I didn't realise Michael worked in this area. Adding him to CC..
On Wed, Feb 03, 2016 at 04:08:17PM +1100, David Gibson wrote: > On Tue, Feb 02, 2016 at 03:37:12PM -0200, Eduardo Habkost wrote: > > On Mon, Feb 01, 2016 at 01:33:09PM +1100, David Gibson wrote: > > > On Fri, Jan 22, 2016 at 03:32:52PM +0100, Igor Mammedov wrote: > > > > On Fri, 22 Jan 2016 15:21:05 +0100 > > > > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > > > > > On 22/01/2016 11:02, Igor Mammedov wrote: > > > > > > On Thu, 21 Jan 2016 12:37:51 +1100 > > > > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > > > > > >> The 'base' field of MemoryHotplugState is ram_addr_t, which indicates that > > > > > >> it exists in the abstract address space of RAM regions. > > > > > >> > > > > > >> However, the actual usage of this field indicates that it is a concrete > > > > > >> physical address (it's passed as an offset to memory_region_add_subgregion > > > > > >> for example). > > > > > >> > > > > > >> So, correct its type to 'hwaddr'. > > > > > >> > > > > > >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > > > >> --- > > > > > >> include/hw/mem/pc-dimm.h | 2 +- > > > > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > >> > > > > > >> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h > > > > > >> index d83bf30..218dfb0 100644 > > > > > >> --- a/include/hw/mem/pc-dimm.h > > > > > >> +++ b/include/hw/mem/pc-dimm.h > > > > > >> @@ -77,7 +77,7 @@ typedef struct PCDIMMDeviceClass { > > > > > >> * @mr: hotplug memory address space container > > > > > >> */ > > > > > >> typedef struct MemoryHotplugState { > > > > > >> - ram_addr_t base; > > > > > >> + hwaddr base; > > > > > >> MemoryRegion mr; > > > > > >> } MemoryHotplugState; > > > > > >> > > > > > > > > > > > > I agree with this fix but that's not the only place where > > > > > > ram_addr_t needs to be replaced with hwaddr. > > > > > > For example type of MachineState.[max]ram_size fields needs > > > > > > to be changed as well. Because QEMU builds without CONFIG_XEN_BACKEND > > > > > > on 32-bit hosts are broken since ram_addr_t is 32-bits there > > > > > > while some targets assume and use it as 64-bit one. > > > > > > > > > > But on a 32-bit system without CONFIG_XEN_BACKEND you cannot allocate > > > > > more than 4G anyway, so the choice of ram_addr_t is understandable in > > > > > that case. > > > > QEMU build will probably fail with above config but if it succeeds > > > > then maxmem will be silently truncated. > > > > > > > > > > > > > > On the other hand, on a 32-bit system without CONFIG_XEN_BACKEND you > > > > > definitely can place 128M of hot plugged memory between say 4096MB and > > > > > 4224MB. > > > > True. > > > > > > > > Anyway for this patch > > > > Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > > > > > Who needs to take this patch? I'm not sure if I need to do anything > > > further to push it forwards. > > > > Most pc-dimm changes were merged either by me or Michael. I can > > merge it if I get an Acked-by from Michael. > > > > Otherwise, Michael has my Acked-by if he wants to merge it > > through his tree. > > > > Acked-by: Eduardo Habkost <ehabkost@redhat.com> > > Ah, I didn't realise Michael worked in this area. Adding him to CC.. Ooops, I thought Michael was already on CC. He is the PC and PCI maintainer, but he has been merging machine.c and other generic hw/ patches recently. I sent another message to qemu-devel today, asking people where could be the default destination for hw/core and other generic hw/ code that has no maintainer in MAINTAINERS.
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h index d83bf30..218dfb0 100644 --- a/include/hw/mem/pc-dimm.h +++ b/include/hw/mem/pc-dimm.h @@ -77,7 +77,7 @@ typedef struct PCDIMMDeviceClass { * @mr: hotplug memory address space container */ typedef struct MemoryHotplugState { - ram_addr_t base; + hwaddr base; MemoryRegion mr; } MemoryHotplugState;
The 'base' field of MemoryHotplugState is ram_addr_t, which indicates that it exists in the abstract address space of RAM regions. However, the actual usage of this field indicates that it is a concrete physical address (it's passed as an offset to memory_region_add_subgregion for example). So, correct its type to 'hwaddr'. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- include/hw/mem/pc-dimm.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)