| Message ID | 20180628121417.1980-5-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | pc-dimm: pre_plug "slot" and "addr" assignment | expand |
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 <david@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; > > /* > * 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? > } > > 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); > + } > + > + 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); > out: > error_propagate(errp, local_err); > } > > -void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align, > - Error **errp) > +void pc_dimm_plug(DeviceState *dev, MachineState *machine, Error **errp) > { > PCDIMMDevice *dimm = PC_DIMM(dev); > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm, > &error_abort); > MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort); > - Error *local_err = NULL; > uint64_t addr; > > - addr = object_property_get_uint(OBJECT(dimm), > - PC_DIMM_ADDR_PROP, &local_err); > - if (local_err) { > - goto out; > - } > - > - 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, &local_err); > - if (local_err) { > - goto out; > - } > + addr = object_property_get_uint(OBJECT(dev), PC_DIMM_ADDR_PROP, > + &error_abort); > trace_mhp_pc_dimm_assigned_address(addr); > > memory_device_plug_region(machine, mr, addr); > vmstate_register_ram(vmstate_mr, dev); > - > -out: > - error_propagate(errp, local_err); > } > > void pc_dimm_unplug(DeviceState *dev, MachineState *machine) > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index bf012235b6..33543c6373 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3150,13 +3150,12 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > PCDIMMDevice *dimm = PC_DIMM(dev); > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort); > - uint64_t align, size, addr; > + uint64_t size, addr; > uint32_t node; > > - align = memory_region_get_alignment(mr); > size = memory_region_size(mr); > > - pc_dimm_plug(dev, MACHINE(ms), align, &local_err); > + pc_dimm_plug(dev, MACHINE(ms), &local_err); > if (local_err) { > goto out; > } > @@ -3223,7 +3222,7 @@ static void spapr_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), NULL, errp); > } > > struct sPAPRDIMMState { > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h > index 7b120416d1..6457b2a25f 100644 > --- a/include/hw/mem/pc-dimm.h > +++ b/include/hw/mem/pc-dimm.h > @@ -79,8 +79,8 @@ typedef struct PCDIMMDeviceClass { > Error **errp); > } PCDIMMDeviceClass; > > -void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp); > -void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align, > - Error **errp); > +void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, > + const uint64_t *compat_align, Error **errp); > +void pc_dimm_plug(DeviceState *dev, MachineState *machine, Error **errp); > void pc_dimm_unplug(DeviceState *dev, MachineState *machine); > #endif
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 <david@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; >> >> /* >> * 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!
On Fri, 29 Jun 2018 09:14:52 +0200 David Hildenbrand <david@redhat.com> 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 <david@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; > >> > >> /* > >> * 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! >
On Thu, 28 Jun 2018 14:14:17 +0200 David Hildenbrand <david@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 > 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 <david@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/ > > /* > * 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); > + > + 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 > out: > error_propagate(errp, local_err); > } > > -void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align, > - Error **errp) > +void pc_dimm_plug(DeviceState *dev, MachineState *machine, Error **errp) > { > PCDIMMDevice *dimm = PC_DIMM(dev); > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm, > &error_abort); > MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort); > - Error *local_err = NULL; > uint64_t addr; > > - addr = object_property_get_uint(OBJECT(dimm), > - PC_DIMM_ADDR_PROP, &local_err); > - if (local_err) { > - goto out; > - } > - > - 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, &local_err); > - if (local_err) { > - goto out; > - } > + addr = object_property_get_uint(OBJECT(dev), PC_DIMM_ADDR_PROP, > + &error_abort); > trace_mhp_pc_dimm_assigned_address(addr); > > memory_device_plug_region(machine, mr, addr); > vmstate_register_ram(vmstate_mr, dev); > - > -out: > - error_propagate(errp, local_err); > } > > void pc_dimm_unplug(DeviceState *dev, MachineState *machine) > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index bf012235b6..33543c6373 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3150,13 +3150,12 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > PCDIMMDevice *dimm = PC_DIMM(dev); > PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort); > - uint64_t align, size, addr; > + uint64_t size, addr; > uint32_t node; > > - align = memory_region_get_alignment(mr); > size = memory_region_size(mr); > > - pc_dimm_plug(dev, MACHINE(ms), align, &local_err); > + pc_dimm_plug(dev, MACHINE(ms), &local_err); > if (local_err) { > goto out; > } > @@ -3223,7 +3222,7 @@ static void spapr_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), NULL, errp); > } > > struct sPAPRDIMMState { > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h > index 7b120416d1..6457b2a25f 100644 > --- a/include/hw/mem/pc-dimm.h > +++ b/include/hw/mem/pc-dimm.h > @@ -79,8 +79,8 @@ typedef struct PCDIMMDeviceClass { > Error **errp); > } PCDIMMDeviceClass; > > -void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp); > -void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align, > - Error **errp); > +void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, > + const uint64_t *compat_align, Error **errp); > +void pc_dimm_plug(DeviceState *dev, MachineState *machine, Error **errp); > void pc_dimm_unplug(DeviceState *dev, MachineState *machine); > #endif
On 29.06.2018 17:08, Igor Mammedov wrote: > On Thu, 28 Jun 2018 14:14:17 +0200 > David Hildenbrand <david@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 <david@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.
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); } 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); + } + + 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); out: error_propagate(errp, local_err); } -void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align, - Error **errp) +void pc_dimm_plug(DeviceState *dev, MachineState *machine, Error **errp) { PCDIMMDevice *dimm = PC_DIMM(dev); PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm, &error_abort); MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort); - Error *local_err = NULL; uint64_t addr; - addr = object_property_get_uint(OBJECT(dimm), - PC_DIMM_ADDR_PROP, &local_err); - if (local_err) { - goto out; - } - - 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, &local_err); - if (local_err) { - goto out; - } + addr = object_property_get_uint(OBJECT(dev), PC_DIMM_ADDR_PROP, + &error_abort); trace_mhp_pc_dimm_assigned_address(addr); memory_device_plug_region(machine, mr, addr); vmstate_register_ram(vmstate_mr, dev); - -out: - error_propagate(errp, local_err); } void pc_dimm_unplug(DeviceState *dev, MachineState *machine) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index bf012235b6..33543c6373 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3150,13 +3150,12 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, PCDIMMDevice *dimm = PC_DIMM(dev); PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort); - uint64_t align, size, addr; + uint64_t size, addr; uint32_t node; - align = memory_region_get_alignment(mr); size = memory_region_size(mr); - pc_dimm_plug(dev, MACHINE(ms), align, &local_err); + pc_dimm_plug(dev, MACHINE(ms), &local_err); if (local_err) { goto out; } @@ -3223,7 +3222,7 @@ static void spapr_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), NULL, errp); } struct sPAPRDIMMState { diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h index 7b120416d1..6457b2a25f 100644 --- a/include/hw/mem/pc-dimm.h +++ b/include/hw/mem/pc-dimm.h @@ -79,8 +79,8 @@ typedef struct PCDIMMDeviceClass { Error **errp); } PCDIMMDeviceClass; -void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, Error **errp); -void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align, - Error **errp); +void pc_dimm_pre_plug(DeviceState *dev, MachineState *machine, + const uint64_t *compat_align, Error **errp); +void pc_dimm_plug(DeviceState *dev, MachineState *machine, Error **errp); void pc_dimm_unplug(DeviceState *dev, MachineState *machine); #endif
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 <david@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(-)