On 29.06.2018 17:08, Igor Mammedov wrote:
> On Thu, 28 Jun 2018 14:14:17 +0200
> David Hildenbrand <da...@redhat.com> 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.
> I'd move tracing to pre_plug as well, so even if pre_plug fails we could
> see in trace what addr was during the failure
I can do that.
>
>> 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.
>
> patch look ok,
> see some nits below
>
>
>> Signed-off-by: David Hildenbrand <da...@redhat.com>
>> ---
>> 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;
> s/compat_align/legacy_align/
agreed.
>
>>
>> /*
>> * 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);
>> }
>>
>> static void pc_memory_plug(HotplugHandler *hotplug_dev,
>> @@ -1705,18 +1708,9 @@ static void pc_memory_plug(HotplugHandler
>> *hotplug_dev,
>> HotplugHandlerClass *hhc;
>> Error *local_err = NULL;
>> PCMachineState *pcms = PC_MACHINE(hotplug_dev);
>> - PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>> - PCDIMMDevice *dimm = PC_DIMM(dev);
>> - PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>> - MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort);
>> - uint64_t align = TARGET_PAGE_SIZE;
>> bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>>
>> - if (pcmc->enforce_aligned_dimm) {
>> - align = memory_region_get_alignment(mr);
>> - }
>> -
>> - pc_dimm_plug(dev, MACHINE(pcms), align, &local_err);
>> + pc_dimm_plug(dev, MACHINE(pcms), &local_err);
>> if (local_err) {
>> goto out;
>> }
>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
>> index e56c4daef2..8e2e8549dc 100644
>> --- a/hw/mem/pc-dimm.c
>> +++ b/hw/mem/pc-dimm.c
>> @@ -29,9 +29,14 @@
>>
>> static int pc_dimm_get_free_slot(const int *hint, int max_slots, Error
>> **errp);
>>
>> -void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp)
>> +void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine,
>> + const uint64_t *compat_align, Error **errp)
>> {
>> + PCDIMMDevice *dimm = PC_DIMM(dev);
>> + PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
>> Error *local_err = NULL;
>> + MemoryRegion *mr;
>> + uint64_t addr, align;
>> int slot;
>>
>> slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
>> @@ -43,44 +48,46 @@ void pc_dimm_pre_plug(DeviceState *dev, MachineState
>> *machine, Error **errp)
>> }
>> object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP,
>> &error_abort);
>> trace_mhp_pc_dimm_assigned_slot(slot);
>> +
>> + mr = ddc->get_memory_region(dimm, &local_err);
>> + if (local_err) {
>> + goto out;
>> + }
>> +
>> + if (compat_align) {
>> + align = *compat_align;
>> + } else {
>> + align = memory_region_get_alignment(mr);
>> + }
> might be:
>
> align = compat_align ? *compat_align :
> memory_region_get_alignment(mr);
Will do if it fits into a single line (it seems it does)
>
>> +
>> + addr = object_property_get_uint(OBJECT(dev), PC_DIMM_ADDR_PROP,
>> + &error_abort);
>> + addr = memory_device_get_free_addr(machine, !addr ? NULL : &addr, align,
>> + memory_region_size(mr), &local_err);
>> + if (local_err) {
>> + goto out;
>> + }
>> + object_property_set_uint(OBJECT(dev), addr, PC_DIMM_ADDR_PROP,
>> + &error_abort);
> if one considers memory device as separate black box, then set might fail
> (there is no guaranties that later on set could return a error)
>
> So I'd keep local_err & co for set actions through out all *_plug handlers
In this part we are still in the pc_dimm specific code. I agree that
this should be done when moving this code to the memory device specific
code. For now I think this is fine.
--
Thanks,
David / dhildenb
