Re: [Qemu-ppc] [PATCH v2 4/4] pc-dimm: assign and verify the "addr" property during pre_plug

[Igor Mammedov]

On Fri, 29 Jun 2018 09:14:52 +0200
David Hildenbrand <address@hidden> wrote:

> On 29.06.2018 03:19, David Gibson wrote:
> > On Thu, Jun 28, 2018 at 02:14:17PM +0200, David Hildenbrand wrote:  
> >> We can assign and verify the slot before realizing and trying to plug.
> >> reading/writing the address property should never fail, so let's reduce
> >> error handling a bit by using &error_abort. Getting access to the memory
> >> region now might however fail. So forward errors from
> >> get_memory_region() properly.
> >>
> >> Keep tracing the assigned address for now in the plug code, as that's the
> >> point we are sure plugging succeeded and the address willa ctually be
> >> used.
> >>
> >> As all memory devices should use the alignment of the underlying memory
> >> region for guest physical address asignment, do detection of the
> >> alignment in pc_dimm_pre_plug(), but allow pc.c to overwrite the
> >> alignment for compatibility handling.
> >>
> >> Signed-off-by: David Hildenbrand <address@hidden>
> >> ---
> >>  hw/i386/pc.c             | 16 ++++--------
> >>  hw/mem/pc-dimm.c         | 53 +++++++++++++++++++++++-----------------
> >>  hw/ppc/spapr.c           |  7 +++---
> >>  include/hw/mem/pc-dimm.h |  6 ++---
> >>  4 files changed, 41 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index 934b7155b1..16e4b5baff 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -1678,7 +1678,9 @@ static void pc_memory_pre_plug(HotplugHandler 
> >> *hotplug_dev, DeviceState *dev,
> >>                                 Error **errp)
> >>  {
> >>      const PCMachineState *pcms = PC_MACHINE(hotplug_dev);
> >> +    const PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> >>      const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> >> +    const uint64_t compat_align = TARGET_PAGE_SIZE;
> >>  
> >>      /*
> >>       * When -no-acpi is used with Q35 machine type, no ACPI is built,
> >> @@ -1696,7 +1698,8 @@ static void pc_memory_pre_plug(HotplugHandler 
> >> *hotplug_dev, DeviceState *dev,
> >>          return;
> >>      }
> >>  
> >> -    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp);
> >> +    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev),
> >> +                     pcmc->enforce_aligned_dimm ? NULL : &compat_align, 
> >> errp);  
> > 
> > Why would you pass a const pointer, rather than just a value?  
> 
> We could do that if we make "0" -> "no compat alignment, detect
> alignment yourself". That should work here.
I'd prefer pointer as it doesn't give any special meaning to 0 value
and more visible showing that something non trivial happens here 

> 
> Thanks!
>