On Fri, 29 Jun 2018 09:14:52 +0200
David Hildenbrand <da...@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 <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;
> >>
> >> /*
> >> * 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!
>
