[Qemu-devel] [PATCH v2 0/4] pc-dimm: pre_plug "slot" and "addr" assignment

[Qemu-devel] [PATCH v2 0/4] pc-dimm: pre_plug "slot" and "addr" assignment

[David Hildenbrand]

As requested by Igor, assign and verify "slot" and "addr" in the
pre_plug handler. Factor out the compatibility handling/configuration
for detecting the alignment to be used when searching for an address
in guest physical memory for a memory device.

This is another part of the original series
    [PATCH v4 00/14] MemoryDevice: use multi stage hotplug handlers
And is based on
    [PATCH v1 0/2] memory: fix alignment checks/asserts
    [PATCH v4 00/12] pc-dimm: next bunch of cleanups

This refactoring is the last step before factoring out pre_plug, plug and
unplug logic of memory devices completely into memory-device.c

v1 -> v2:
- dropped "machine: factor out enforce_aligned_dimm into memory_device_align"
- dropped "pc-dimm/memory-device: detect alignment internally"
- added "util/oslib-win32: indicate alignment for qemu_anon_ram_alloc()"
-- indicate the alignment and therefor simplify compat handling for
   Windows as we can ...
- added "pc: drop memory region alignment check for 0"
-- ... drop a check for memory_region_get_alignment(mr), allowing us to
   factor out compat handling for pc DIMM/NVDIMM alignment in a clean way.
- "pc-dimm: assign and verify the "addr" property during pre_plug"
-- Default to "memory_region_get_alignment(mr)" if no compat alignment
   has been specified.

David Hildenbrand (4):
  pc-dimm: assign and verify the "slot" property during pre_plug
  util/oslib-win32: indicate alignment for qemu_anon_ram_alloc()
  pc: drop memory region alignment check for 0
  pc-dimm: assign and verify the "addr" property during pre_plug

 hw/i386/pc.c             | 16 ++++------
 hw/mem/pc-dimm.c         | 66 ++++++++++++++++++++++------------------
 hw/ppc/spapr.c           | 14 ++++++---
 include/hw/mem/pc-dimm.h |  5 +--
 util/oslib-win32.c       |  4 +++
 5 files changed, 60 insertions(+), 45 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 1/4] pc-dimm: assign and verify the "slot" property during pre_plug

[David Hildenbrand]

We can assign and verify the slot before realizing and trying to plug.
reading/writing the slot property should never fail, so let's reduce
error handling a bit by using &error_abort.

To do this during pre_plug, add and use (x86, ppc) pc_dimm_pre_plug().

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/i386/pc.c             |  2 ++
 hw/mem/pc-dimm.c         | 35 ++++++++++++++++++-----------------
 hw/ppc/spapr.c           |  9 ++++++++-
 include/hw/mem/pc-dimm.h |  1 +
 4 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f310040351..bf986baf91 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1695,6 +1695,8 @@ static void pc_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
         return;
     }
+
+    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp);
 }
 
 static void pc_memory_plug(HotplugHandler *hotplug_dev,
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 65843bc52a..e56c4daef2 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -29,10 +29,27 @@
 
 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)
+{
+    Error *local_err = NULL;
+    int slot;
+
+    slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
+                                   &error_abort);
+    slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
+                                 machine->ram_slots, &local_err);
+    if (local_err) {
+        goto out;
+    }
+    object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &error_abort);
+    trace_mhp_pc_dimm_assigned_slot(slot);
+out:
+    error_propagate(errp, local_err);
+}
+
 void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
                   Error **errp)
 {
-    int slot;
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
     MemoryRegion *vmstate_mr = ddc->get_vmstate_memory_region(dimm,
@@ -59,22 +76,6 @@ void pc_dimm_plug(DeviceState *dev, MachineState *machine, uint64_t align,
     }
     trace_mhp_pc_dimm_assigned_address(addr);
 
-    slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP, &local_err);
-    if (local_err) {
-        goto out;
-    }
-
-    slot = pc_dimm_get_free_slot(slot == PC_DIMM_UNASSIGNED_SLOT ? NULL : &slot,
-                                 machine->ram_slots, &local_err);
-    if (local_err) {
-        goto out;
-    }
-    object_property_set_int(OBJECT(dev), slot, PC_DIMM_SLOT_PROP, &local_err);
-    if (local_err) {
-        goto out;
-    }
-    trace_mhp_pc_dimm_assigned_slot(slot);
-
     memory_device_plug_region(machine, mr, addr);
     vmstate_register_ram(vmstate_mr, dev);
 
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b32b971a14..bf012235b6 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3191,6 +3191,7 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
     PCDIMMDevice *dimm = PC_DIMM(dev);
     PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
+    Error *local_err = NULL;
     MemoryRegion *mr;
     uint64_t size;
     Object *memdev;
@@ -3216,7 +3217,13 @@ static void spapr_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     memdev = object_property_get_link(OBJECT(dimm), PC_DIMM_MEMDEV_PROP,
                                       &error_abort);
     pagesize = host_memory_backend_pagesize(MEMORY_BACKEND(memdev));
-    spapr_check_pagesize(spapr, pagesize, errp);
+    spapr_check_pagesize(spapr, pagesize, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    pc_dimm_pre_plug(dev, MACHINE(hotplug_dev), errp);
 }
 
 struct sPAPRDIMMState {
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 26ebb7d5e9..7b120416d1 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -79,6 +79,7 @@ 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_unplug(DeviceState *dev, MachineState *machine);
-- 
2.17.1

[Qemu-devel] [PATCH v2 2/4] util/oslib-win32: indicate alignment for qemu_anon_ram_alloc()

[David Hildenbrand]

Let's set the alignment just like for the posix variant. This will
implicitly set the alignment of the underlying memory region and
therefore make memory_region_get_alignment(mr) return something > 0 for
all memory backends applicable to PCDIMM/NVDIMM.

This will allow us to drop special handling in pc.c for
memory_region_get_alignment(mr) == 0, as we can then assume that it is
always set (and AFAICS >= getpagesize()).

For pc in pc_memory_plug(), under Windows TARGET_PAGE_SIZE == getpagesize(),
therefore alignment of DIMMs will not change, and therefore also not the
guest physical memory layout.

For spapr in spapr_memory_plug(), an alignment of 0 would have been used
until now. Now getpagesize() is used. But as the size of DIMMs has to be
multiple of 256MB on spapr, automatic address asignment will not change
the layout (no memory holes will be created as the alignment change will
not apply if DIMMs start/end on 256MB boundaries).

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 util/oslib-win32.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index bb5ad28bd3..6e87721419 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -76,6 +76,10 @@ void *qemu_anon_ram_alloc(size_t size, uint64_t *align, bool shared)
        memory is page aligned. */
     ptr = VirtualAlloc(NULL, size, MEM_COMMIT, PAGE_READWRITE);
     trace_qemu_anon_ram_alloc(size, ptr);
+
+    if (ptr && align) {
+        *align = getpagesize();
+    }
     return ptr;
 }
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 2/4] util/oslib-win32: indicate alignment for qemu_anon_ram_alloc()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2037 bytes --]

On Thu, Jun 28, 2018 at 02:14:15PM +0200, David Hildenbrand wrote:
> Let's set the alignment just like for the posix variant. This will
> implicitly set the alignment of the underlying memory region and
> therefore make memory_region_get_alignment(mr) return something > 0 for
> all memory backends applicable to PCDIMM/NVDIMM.
> 
> This will allow us to drop special handling in pc.c for
> memory_region_get_alignment(mr) == 0, as we can then assume that it is
> always set (and AFAICS >= getpagesize()).
> 
> For pc in pc_memory_plug(), under Windows TARGET_PAGE_SIZE == getpagesize(),
> therefore alignment of DIMMs will not change, and therefore also not the
> guest physical memory layout.
> 
> For spapr in spapr_memory_plug(), an alignment of 0 would have been used
> until now. Now getpagesize() is used. But as the size of DIMMs has to be
> multiple of 256MB on spapr, automatic address asignment will not change
> the layout (no memory holes will be created as the alignment change will
> not apply if DIMMs start/end on 256MB boundaries).
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

To the very limited extent that I'm able to review a win32 patch:

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  util/oslib-win32.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index bb5ad28bd3..6e87721419 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -76,6 +76,10 @@ void *qemu_anon_ram_alloc(size_t size, uint64_t *align, bool shared)
>         memory is page aligned. */
>      ptr = VirtualAlloc(NULL, size, MEM_COMMIT, PAGE_READWRITE);
>      trace_qemu_anon_ram_alloc(size, ptr);
> +
> +    if (ptr && align) {
> +        *align = getpagesize();
> +    }
>      return ptr;
>  }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v2 2/4] util/oslib-win32: indicate alignment for qemu_anon_ram_alloc()

[Igor Mammedov]

On Thu, 28 Jun 2018 14:14:15 +0200
David Hildenbrand <david@redhat.com> wrote:

> Let's set the alignment just like for the posix variant. This will
> implicitly set the alignment of the underlying memory region and
> therefore make memory_region_get_alignment(mr) return something > 0 for
> all memory backends applicable to PCDIMM/NVDIMM.
> 
> This will allow us to drop special handling in pc.c for
> memory_region_get_alignment(mr) == 0, as we can then assume that it is
> always set (and AFAICS >= getpagesize()).
> 
> For pc in pc_memory_plug(), under Windows TARGET_PAGE_SIZE == getpagesize(),
> therefore alignment of DIMMs will not change, and therefore also not the
> guest physical memory layout.
why not use QEMU_VMALLOC_ALIGN for consistency (on win => getpagesize())
instead of TARGET_PAGE_SIZE like linux allocator does?

Also looking at FIXME comment it notes that VirtualAlloc might have 64K
alignment (though I haven't found it in VirtualAlloc manual).
If that's true then we might need set *align to it to avoid auto-picked
address overlap with previous allocation (not really sure about it).


> For spapr in spapr_memory_plug(), an alignment of 0 would have been used
note that align == 0 would lead to crash where QEMU_ALIGN_UP() is used,
so we don't care to keep it compatible (the same like in commit 92a37a04d)

> until now. Now getpagesize() is used. But as the size of DIMMs has to be
> multiple of 256MB on spapr, automatic address asignment will not change
> the layout (no memory holes will be created as the alignment change will
> not apply if DIMMs start/end on 256MB boundaries).
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  util/oslib-win32.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index bb5ad28bd3..6e87721419 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -76,6 +76,10 @@ void *qemu_anon_ram_alloc(size_t size, uint64_t *align, bool shared)
>         memory is page aligned. */
>      ptr = VirtualAlloc(NULL, size, MEM_COMMIT, PAGE_READWRITE);
>      trace_qemu_anon_ram_alloc(size, ptr);
> +
> +    if (ptr && align) {
> +        *align = getpagesize();
> +    }
>      return ptr;
>  }
>  

Re: [Qemu-devel] [PATCH v2 2/4] util/oslib-win32: indicate alignment for qemu_anon_ram_alloc()

[David Hildenbrand]

On 29.06.2018 16:49, Igor Mammedov wrote:
> On Thu, 28 Jun 2018 14:14:15 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Let's set the alignment just like for the posix variant. This will
>> implicitly set the alignment of the underlying memory region and
>> therefore make memory_region_get_alignment(mr) return something > 0 for
>> all memory backends applicable to PCDIMM/NVDIMM.
>>
>> This will allow us to drop special handling in pc.c for
>> memory_region_get_alignment(mr) == 0, as we can then assume that it is
>> always set (and AFAICS >= getpagesize()).
>>
>> For pc in pc_memory_plug(), under Windows TARGET_PAGE_SIZE == getpagesize(),
>> therefore alignment of DIMMs will not change, and therefore also not the
>> guest physical memory layout.
> why not use QEMU_VMALLOC_ALIGN for consistency (on win => getpagesize())
> instead of TARGET_PAGE_SIZE like linux allocator does?

Sure we can do that, I wanted to match here exactly what has been
written in the comment.

> 
> Also looking at FIXME comment it notes that VirtualAlloc might have 64K
> alignment (though I haven't found it in VirtualAlloc manual).
> If that's true then we might need set *align to it to avoid auto-picked
> address overlap with previous allocation (not really sure about it).

"To determine the size of a page and the allocation granularity on the
host computer, use the GetSystemInfo" [1]

"The size of the region, in bytes. If the lpAddress parameter is NULL,
this value is rounded up to the next page boundary. " [1]

Historically, this seems to be 64k. But it will always be at least 4k
(page size). So what we could do is query the actual allocation granularity:

int get_allocation_granularity(void) {
	SYSTEM_INFO system_info;

	GetSystemInfo(&system_info);
	return system_info.dwAllocationGranularity
}


"dwAllocationGranularity: The granularity for the starting address at
which virtual memory can be allocated. For more information, see
VirtualAlloc." [2]

What do you think?

> 
> 
>> For spapr in spapr_memory_plug(), an alignment of 0 would have been used
> note that align == 0 would lead to crash where QEMU_ALIGN_UP() is used,
> so we don't care to keep it compatible (the same like in commit 92a37a04d)

Good point!

[1]
https://msdn.microsoft.com/en-us/library/windows/desktop/aa366887(v=vs.85).aspx
[2]
https://msdn.microsoft.com/en-us/library/windows/desktop/ms724958(v=vs.85).aspx

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 2/4] util/oslib-win32: indicate alignment for qemu_anon_ram_alloc()

[Igor Mammedov]

On Fri, 29 Jun 2018 17:39:10 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 29.06.2018 16:49, Igor Mammedov wrote:
> > On Thu, 28 Jun 2018 14:14:15 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> Let's set the alignment just like for the posix variant. This will
> >> implicitly set the alignment of the underlying memory region and
> >> therefore make memory_region_get_alignment(mr) return something > 0 for
> >> all memory backends applicable to PCDIMM/NVDIMM.
> >>
> >> This will allow us to drop special handling in pc.c for
> >> memory_region_get_alignment(mr) == 0, as we can then assume that it is
> >> always set (and AFAICS >= getpagesize()).
> >>
> >> For pc in pc_memory_plug(), under Windows TARGET_PAGE_SIZE == getpagesize(),
> >> therefore alignment of DIMMs will not change, and therefore also not the
> >> guest physical memory layout.  
> > why not use QEMU_VMALLOC_ALIGN for consistency (on win => getpagesize())
> > instead of TARGET_PAGE_SIZE like linux allocator does?  
> 
> Sure we can do that, I wanted to match here exactly what has been
> written in the comment.
> 
> > 
> > Also looking at FIXME comment it notes that VirtualAlloc might have 64K
> > alignment (though I haven't found it in VirtualAlloc manual).
> > If that's true then we might need set *align to it to avoid auto-picked
> > address overlap with previous allocation (not really sure about it).  
> 
> "To determine the size of a page and the allocation granularity on the
> host computer, use the GetSystemInfo" [1]
> 
> "The size of the region, in bytes. If the lpAddress parameter is NULL,
> this value is rounded up to the next page boundary. " [1]
> 
> Historically, this seems to be 64k. But it will always be at least 4k
> (page size). So what we could do is query the actual allocation granularity:
> 
> int get_allocation_granularity(void) {
> 	SYSTEM_INFO system_info;
> 
> 	GetSystemInfo(&system_info);
> 	return system_info.dwAllocationGranularity
> }
> 
> 
> "dwAllocationGranularity: The granularity for the starting address at
> which virtual memory can be allocated. For more information, see
> VirtualAlloc." [2]
> 
> What do you think?
maybe do following:

    *align = MAX(get_allocation_granularity(), getpagesize())

> 
> > 
> >   
> >> For spapr in spapr_memory_plug(), an alignment of 0 would have been used  
> > note that align == 0 would lead to crash where QEMU_ALIGN_UP() is used,
> > so we don't care to keep it compatible (the same like in commit 92a37a04d)  
> 
> Good point!
> 
> [1]
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa366887(v=vs.85).aspx
> [2]
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms724958(v=vs.85).aspx
> 

Re: [Qemu-devel] [PATCH v2 2/4] util/oslib-win32: indicate alignment for qemu_anon_ram_alloc()

[David Hildenbrand]

On 29.06.2018 17:56, Igor Mammedov wrote:
> On Fri, 29 Jun 2018 17:39:10 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 29.06.2018 16:49, Igor Mammedov wrote:
>>> On Thu, 28 Jun 2018 14:14:15 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> Let's set the alignment just like for the posix variant. This will
>>>> implicitly set the alignment of the underlying memory region and
>>>> therefore make memory_region_get_alignment(mr) return something > 0 for
>>>> all memory backends applicable to PCDIMM/NVDIMM.
>>>>
>>>> This will allow us to drop special handling in pc.c for
>>>> memory_region_get_alignment(mr) == 0, as we can then assume that it is
>>>> always set (and AFAICS >= getpagesize()).
>>>>
>>>> For pc in pc_memory_plug(), under Windows TARGET_PAGE_SIZE == getpagesize(),
>>>> therefore alignment of DIMMs will not change, and therefore also not the
>>>> guest physical memory layout.  
>>> why not use QEMU_VMALLOC_ALIGN for consistency (on win => getpagesize())
>>> instead of TARGET_PAGE_SIZE like linux allocator does?  
>>
>> Sure we can do that, I wanted to match here exactly what has been
>> written in the comment.
>>
>>>
>>> Also looking at FIXME comment it notes that VirtualAlloc might have 64K
>>> alignment (though I haven't found it in VirtualAlloc manual).
>>> If that's true then we might need set *align to it to avoid auto-picked
>>> address overlap with previous allocation (not really sure about it).  
>>
>> "To determine the size of a page and the allocation granularity on the
>> host computer, use the GetSystemInfo" [1]
>>
>> "The size of the region, in bytes. If the lpAddress parameter is NULL,
>> this value is rounded up to the next page boundary. " [1]
>>
>> Historically, this seems to be 64k. But it will always be at least 4k
>> (page size). So what we could do is query the actual allocation granularity:
>>
>> int get_allocation_granularity(void) {
>> 	SYSTEM_INFO system_info;
>>
>> 	GetSystemInfo(&system_info);
>> 	return system_info.dwAllocationGranularity
>> }
>>
>>
>> "dwAllocationGranularity: The granularity for the starting address at
>> which virtual memory can be allocated. For more information, see
>> VirtualAlloc." [2]
>>
>> What do you think?
> maybe do following:
> 
>     *align = MAX(get_allocation_granularity(), getpagesize())

That sounds like the best alternative when having to guess what is
actually going on behind the curtains.

-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH v2 3/4] pc: drop memory region alignment check for 0

[David Hildenbrand]

All applicable memory regions always have an alignment > 0. All memory
backends result in file_ram_alloc() or qemu_anon_ram_alloc() getting
called, setting the alignment to > 0.

So a PCDIMM memory region always has an alignment > 0. NVDIMM copy the
alignment of the original memory memory region into the handcrafted memory
region that will be used at this place.

So the check for 0 can be dropped and we can reduce the special
handling.

Dropping this check makes factoring out of alignment handling easier as
compat handling only has to look at pcmc->enforce_aligned_dimm and not
care about the alignment of the memory region.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/i386/pc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index bf986baf91..934b7155b1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1712,7 +1712,7 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
     uint64_t align = TARGET_PAGE_SIZE;
     bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
 
-    if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
+    if (pcmc->enforce_aligned_dimm) {
         align = memory_region_get_alignment(mr);
     }
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 3/4] pc: drop memory region alignment check for 0

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1657 bytes --]

On Thu, Jun 28, 2018 at 02:14:16PM +0200, David Hildenbrand wrote:
> All applicable memory regions always have an alignment > 0. All memory
> backends result in file_ram_alloc() or qemu_anon_ram_alloc() getting
> called, setting the alignment to > 0.
> 
> So a PCDIMM memory region always has an alignment > 0. NVDIMM copy the
> alignment of the original memory memory region into the handcrafted memory
> region that will be used at this place.
> 
> So the check for 0 can be dropped and we can reduce the special
> handling.
> 
> Dropping this check makes factoring out of alignment handling easier as
> compat handling only has to look at pcmc->enforce_aligned_dimm and not
> care about the alignment of the memory region.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/i386/pc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index bf986baf91..934b7155b1 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1712,7 +1712,7 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
>      uint64_t align = TARGET_PAGE_SIZE;
>      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>  
> -    if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
> +    if (pcmc->enforce_aligned_dimm) {
>          align = memory_region_get_alignment(mr);
>      }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v2 3/4] pc: drop memory region alignment check for 0

[Igor Mammedov]

On Thu, 28 Jun 2018 14:14:16 +0200
David Hildenbrand <david@redhat.com> wrote:

> All applicable memory regions always have an alignment > 0. All memory
> backends result in file_ram_alloc() or qemu_anon_ram_alloc() getting
> called, setting the alignment to > 0.
> 
> So a PCDIMM memory region always has an alignment > 0. NVDIMM copy the
> alignment of the original memory memory region into the handcrafted memory
> region that will be used at this place.
> 
> So the check for 0 can be dropped and we can reduce the special
> handling.
> 
> Dropping this check makes factoring out of alignment handling easier as
> compat handling only has to look at pcmc->enforce_aligned_dimm and not
> care about the alignment of the memory region.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/i386/pc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index bf986baf91..934b7155b1 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1712,7 +1712,7 @@ static void pc_memory_plug(HotplugHandler *hotplug_dev,
>      uint64_t align = TARGET_PAGE_SIZE;
>      bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>  
> -    if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) {
> +    if (pcmc->enforce_aligned_dimm) {
>          align = memory_region_get_alignment(mr);
>      }
>  

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

[David Hildenbrand]

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);
 }
 
 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
-- 
2.17.1

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 8513 bytes --]

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

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [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 <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!

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [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 <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!
> 

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

[Igor Mammedov]

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

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

[David Hildenbrand]

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.

-- 

Thanks,

David / dhildenb