Re: [Qemu-devel] [PATCH] dimm: Correct type of MemoryHotplugState->base

[Igor Mammedov]

On Fri, 22 Jan 2016 15:21:05 +0100
Paolo Bonzini <address@hidden> wrote:

> On 22/01/2016 11:02, Igor Mammedov wrote:
> > On Thu, 21 Jan 2016 12:37:51 +1100
> > David Gibson <address@hidden> 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 <address@hidden>
> >> ---
> >>  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 <address@hidden>

> 
> Paolo
>