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

[David Hildenbrand]

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.

Thanks!

-- 

Thanks,

David / dhildenb