MemoryDevice: use multi stage hotplug handlers

The start of the address space does not have to be aligned for the search. Handle this case explicitly when starting the search for a new address.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/mem/memory-device.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

hw/mem/memory-device.c +1 -2

index 3e04f3954e..361d38bfc5 100644
@@ -116,7 +116,6 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
     address_space_start = ms->device_memory->base;
     address_space_end = address_space_start +
                         memory_region_size(&ms->device_memory->mr);
-    g_assert(QEMU_ALIGN_UP(address_space_start, align) == address_space_start);
     g_assert(address_space_end >= address_space_start);
 
     memory_device_check_addable(ms, size, errp);

@@ -149,7 +148,7 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
             return 0;
         }
     } else {
-        new_addr = address_space_start;
+        new_addr = QEMU_ALIGN_UP(address_space_start, align);
     }
 
     /* find address range that will fit new memory device */

Quoted 3 lines · On Thu, 17 May 2018 10:15:14 +0200 David Hildenbrand <[email protected]> wrote:

The start of the address space does not have to be aligned for the search. Handle this case explicitly when starting the search for a new address.

That's true,
but commit message doesn't explain why address_space_start should be allowed to be non aligned.

At least with this assert we would notice early that board allocating misaligned address space.
I'd keep the assert unless there is a good reason to drop it.

Quoted 29 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/mem/memory-device.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

hw/mem/memory-device.c +1 -2

index 3e04f3954e..361d38bfc5 100644
@@ -116,7 +116,6 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
     address_space_start = ms->device_memory->base;
     address_space_end = address_space_start +
                         memory_region_size(&ms->device_memory->mr);
-    g_assert(QEMU_ALIGN_UP(address_space_start, align) == address_space_start);
     g_assert(address_space_end >= address_space_start);
 
     memory_device_check_addable(ms, size, errp);

@@ -149,7 +148,7 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
             return 0;
         }
     } else {
-        new_addr = address_space_start;
+        new_addr = QEMU_ALIGN_UP(address_space_start, align);
     }
 
     /* find address range that will fit new memory device */

Quoted 6 lines · Igor Mammedov

Quoted 3 lines · On Thu, 17 May 2018 10:15:14 +0200 David Hildenbrand <[email protected]> wrote:

The start of the address space does not have to be aligned for the search. Handle this case explicitly when starting the search for a new address.

Quoted 7 lines

That's true,
but commit message doesn't explain why address_space_start should be allowed to be non aligned.

At least with this assert we would notice early that board allocating misaligned address space.
I'd keep the assert unless there is a good reason to drop it.

That reason might be that I can easily crash QEMU

./x86_64-softmmu/qemu-system-x86_64 -m 256M,maxmem=20G,slots=2 -object
memory-backend-file,id=mem0,size=8192M,mem-path=/dev/zero,align=8192M -device pc-dimm,id=dimm1,memdev=mem0

ERROR:hw/mem/memory-device.c:146:memory_device_get_free_addr: assertion
failed: (QEMU_ALIGN_UP(address_space_start, align) == address_space_start)

Quoted 29 lines

Quoted 28 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/mem/memory-device.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

hw/mem/memory-device.c +1 -2

index 3e04f3954e..361d38bfc5 100644
@@ -116,7 +116,6 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
     address_space_start = ms->device_memory->base;
     address_space_end = address_space_start +
                         memory_region_size(&ms->device_memory->mr);
-    g_assert(QEMU_ALIGN_UP(address_space_start, align) == address_space_start);
     g_assert(address_space_end >= address_space_start);
 
     memory_device_check_addable(ms, size, errp);

@@ -149,7 +148,7 @@ uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
             return 0;
         }
     } else {
-        new_addr = address_space_start;
+        new_addr = QEMU_ALIGN_UP(address_space_start, align);
     }
 
     /* find address range that will fit new memory device */  

Quoted 15 lines · On Tue, 29 May 2018 18:02:06 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 6 lines · Igor Mammedov

Quoted 3 lines · On Thu, 17 May 2018 10:15:14 +0200 David Hildenbrand <[email protected]> wrote:

The start of the address space does not have to be aligned for the search. Handle this case explicitly when starting the search for a new address.

Quoted 7 lines

That's true,
but commit message doesn't explain why address_space_start should be allowed to be non aligned.

At least with this assert we would notice early that board allocating misaligned address space.
I'd keep the assert unless there is a good reason to drop it.

Quoted 8 lines

That reason might be that I can easily crash QEMU

./x86_64-softmmu/qemu-system-x86_64 -m 256M,maxmem=20G,slots=2 -object
memory-backend-file,id=mem0,size=8192M,mem-path=/dev/zero,align=8192M -device pc-dimm,id=dimm1,memdev=mem0

ERROR:hw/mem/memory-device.c:146:memory_device_get_free_addr: assertion
failed: (QEMU_ALIGN_UP(address_space_start, align) == address_space_start)

it looks like a different issue.
As I remember user visible 'align' property was added as duct tape since we can't figure out alignment for DAX device no the host,
so it was left upto upper layers to magically figure that out.

However we probably shouldn't allow arbitrary or bigger aligment than max page size supported by target machine/cpu
(i.e. currently hardcoded address_space_start alignment),
as it creates unnecessary fragmentation and not counted in size of hotplug region (for x86 we count in additional 1Gb per memory device).

How about turning that assert into error check that inhibits plugging in devices with alignment values larger than address_space_start alignment?

Quoted 37 lines · Igor Mammedov

Quoted 15 lines · On Tue, 29 May 2018 18:02:06 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 6 lines · Igor Mammedov

Quoted 3 lines · On Thu, 17 May 2018 10:15:14 +0200 David Hildenbrand <[email protected]> wrote:

The start of the address space does not have to be aligned for the search. Handle this case explicitly when starting the search for a new address.

Quoted 7 lines

That's true,
but commit message doesn't explain why address_space_start should be allowed to be non aligned.

At least with this assert we would notice early that board allocating misaligned address space.
I'd keep the assert unless there is a good reason to drop it.

Quoted 8 lines

That reason might be that I can easily crash QEMU

./x86_64-softmmu/qemu-system-x86_64 -m 256M,maxmem=20G,slots=2 -object
memory-backend-file,id=mem0,size=8192M,mem-path=/dev/zero,align=8192M -device pc-dimm,id=dimm1,memdev=mem0

ERROR:hw/mem/memory-device.c:146:memory_device_get_free_addr: assertion
failed: (QEMU_ALIGN_UP(address_space_start, align) == address_space_start)

it looks like a different issue.
As I remember user visible 'align' property was added as duct tape since we can't figure out alignment for DAX device no the host,
so it was left upto upper layers to magically figure that out.

However we probably shouldn't allow arbitrary or bigger aligment than max page size supported by target machine/cpu
(i.e. currently hardcoded address_space_start alignment),
as it creates unnecessary fragmentation and not counted in size of hotplug region (for x86 we count in additional 1Gb per memory device).

Quoted 3 lines

How about turning that assert into error check that inhibits plugging in devices with alignment values larger than address_space_start alignment?

Let me explain a little bit why I don't like such restrictions (for which I don't see a need yet):

virtio-mem devices will later have a certain block size (e.g. 4MB). I want to give devices during resource assignment the possibility to specify their alignment requirements.

For said virtio-mem device, this would e.g. be 4MB. (see patch 10 and 14 of how this call "get_align" comes into play), because the addresses of the memory blocks are all aligned by e.g. 4MB. This is what is guaranteed by the device specification.

E.g. for DIMMs we might want to allow to specify the section size (e.g. 128MB on x86), otherwise e.g. Linux is not able to add all memory. (but we should not hardcode this, as this is a Linux specific requirement - still it would be nice to specify)

So in general, a memory device might have some alignment that is to be taken care of.

I don't understand right now why an upper limit on the alignment would make sense at all. We can easily handle it during our search. And we have to handle it either way during the search, if we plug some device with strange sizes (e.g. 1MB DIMM).

Of course, we might end up fragmenting guest physical memory, but that is purely a setup issue (choosing sizes of devices + main memory properly). I don't see a reason to error out (and of course also not to assert out :) ).

Quoted 3 lines

Let me explain a little bit why I don't like such restrictions (for which I don't see a need yet):

(*) being conservative is good here because we can always relax restrictions in future if it's needed without breaking users, but we can't take away something thing that's been shipped (and if we do it, it typically introduces a bunch of compat code to keep old machines working). Also beside of giving as some freedom of movement in the future, restrictions also to some degree prevent user from misconfiguration)

Quoted 4 lines

virtio-mem devices will later have a certain block size (e.g. 4MB). I want to give devices during resource assignment the possibility to specify their alignment requirements.

size and alignment are 2 diffrent things here, alignment in our design is dictated by backing storage page size and for performance reasons HVA and GPA should be aligned on the same boundary, users are free to pick another GPA manually as far as it has the same alignment. But for automatic placement we use backend's alignment to make placement as compact as possible but still keeping GPA aligned with HVA.

Quoted 4 lines

For said virtio-mem device, this would e.g. be 4MB. (see patch 10 and 14 of how this call "get_align" comes into play), because the addresses of the memory blocks are all aligned by e.g. 4MB. This is what is guaranteed by the device specification.

where does this 4Mb magic comes from and why block must be aligned on this size?

Quoted 5 lines

E.g. for DIMMs we might want to allow to specify the section size (e.g. 128MB on x86), otherwise e.g. Linux is not able to add all memory. (but we should not hardcode this, as this is a Linux specific requirement - still it would be nice to specify)

true, it's guest specific and we do not have restrictions here. The only restriction we have here is that size must be multiple of backing storage page size (i.e. alignment) so that guest would be able to use tail page.

Quoted 2 lines

So in general, a memory device might have some alignment that is to be taken care of.

Do we really need introducing frontend specific alignment?
I'd try reuse backend's one and go for frontend's only in case we have to.

Quoted 10 lines

I don't understand right now why an upper limit on the alignment would make sense at all. We can easily handle it during our search. And we have to handle it either way during the search, if we plug some device with strange sizes (e.g. 1MB DIMM).

Of course, we might end up fragmenting guest physical memory, but that is purely a setup issue (choosing sizes of devices + main memory properly). I don't see a reason to error out (and of course also not to assert out :) ).

Agreed about assert, but I'd still prefer error out there so that users won't crate insane config and then complain (see below and *).

Upper alignment value is useful for proper sizing of hotplug address space, so that user could plug #slots devices upto #maxmem specified on CLI. It's still possible to misconfigure using manual placement, but default one just works, user either consumes all memory #maxmem and/or #slots.

There is no misunderstanding in case of error here as it works same as on baremetal, one doesn't have a free place to put more memory or all memory one asked for is already there.

So it might be that #slots decoupling from memory device a premature action and we can still use it with virtio-mem/pmem.

Quoted 8 lines

Quoted 2 lines

Let me explain a little bit why I don't like such restrictions (for which I don't see a need yet):

(*) being conservative is good here because we can always relax restrictions in future if it's needed without breaking users, but we can't take away something thing that's been shipped (and if we do it, it typically introduces a bunch of compat code to keep old machines working). Also beside of giving as some freedom of movement in the future, restrictions also to some degree prevent user from misconfiguration)

Right, but I consider a maximum on an alignment arbitrary (and even difficult to implement - as you said, "max page size supported by target machine/cpu" - there are even different huge page sizes e.g. on x86).

So I'd really vote against introducing such checks.

And another point against introducing a check for the maimum alignment would be: There might be users with a specified alignment > max page size. Changing the behavior will break these setups (strange but true).

Quoted 10 lines

Quoted 3 lines

virtio-mem devices will later have a certain block size (e.g. 4MB). I want to give devices during resource assignment the possibility to specify their alignment requirements.

size and alignment are 2 diffrent things here, alignment in our design is dictated by backing storage page size and for performance reasons HVA and GPA should be aligned on the same boundary, users are free to pick another GPA manually as far as it has the same alignment. But for automatic placement we use backend's alignment to make placement as compact as possible but still keeping GPA aligned with HVA.

Quoted 6 lines

Quoted 4 lines

For said virtio-mem device, this would e.g. be 4MB. (see patch 10 and 14 of how this call "get_align" comes into play), because the addresses of the memory blocks are all aligned by e.g. 4MB. This is what is guaranteed by the device specification.

where does this 4Mb magic comes from and why block must be aligned on this size?

(knowing it is hard to get the idea without the current prototype in place, but that will be fixed soon)

virtio-mem works in blocks. The block size is the size in which memory can be plugged or unplugged by the guest. It also determines the granularity (and therefore the bitmap) we have to use to keep track of unplugged memory. It is configurable (e.g. for migration purposes), but might also depend on the backend memory type. E.g. if huge pages are used in the host, the huge page size defines the minimum block size. But consider the last part a special case. Huge pages will not be supported for now.

The block size also defines the alignment that has to be used by the guest for plugging/unplugging (because that's how blocks gets mapped to the bitmap entries). So a virtio-mem device really needs a block-aligned start address,

For now I use 4MB because it matches what guests (e.g. Linux) can actually support and keeps the bitmap small. But as I said, it is configurable. (-device virtio-mem,block_size=1MB, ...)

Quoted 12 lines

Quoted 5 lines

E.g. for DIMMs we might want to allow to specify the section size (e.g. 128MB on x86), otherwise e.g. Linux is not able to add all memory. (but we should not hardcode this, as this is a Linux specific requirement - still it would be nice to specify)

true, it's guest specific and we do not have restrictions here. The only restriction we have here is that size must be multiple of backing storage page size (i.e. alignment) so that guest would be able to use tail page.

Quoted 2 lines

So in general, a memory device might have some alignment that is to be taken care of.

Quoted 2 lines

Do we really need introducing frontend specific alignment?
I'd try reuse backend's one and go for frontend's only in case we have to.

I think so. But I consider this a special case for virtio-mem. (specified indirectly via block_size). For the remaining stuff, we might be able to live with the memory backend aligment. And I agree that this should be the default if possible.

Quoted 9 lines

Quoted 9 lines

I don't understand right now why an upper limit on the alignment would make sense at all. We can easily handle it during our search. And we have to handle it either way during the search, if we plug some device with strange sizes (e.g. 1MB DIMM).

Of course, we might end up fragmenting guest physical memory, but that is purely a setup issue (choosing sizes of devices + main memory properly). I don't see a reason to error out (and of course also not to assert out :) ).

Quoted 7 lines

Agreed about assert, but I'd still prefer error out there so that users won't crate insane config and then complain (see below and *).

Upper alignment value is useful for proper sizing of hotplug address space, so that user could plug #slots devices upto #maxmem specified on CLI. It's still possible to misconfigure using manual placement, but default one just works, user either consumes all memory #maxmem and/or #slots.

Please not that it will still work in most cases. Only if people start to define crazy alignments (like I did), crazy DIMM sizes (e.g. 1MB) or crazy block sizes for virtio-mem, we might have a fragmented guest physical memory. This should usually not happen, but if so, we already have error messages for reporting this "fragmented memory".

And I consider these "strange configs" similar as "strange manual placement". Same result: fragmented memory, error out.

Personally, I don't think we have to guarantee that automatic placement works in all scenarios that the user is able to come up with. It should work in sane configurations, which is still that case.

Quoted 4 lines

There is no misunderstanding in case of error here as it works same as on baremetal, one doesn't have a free place to put more memory or all memory one asked for is already there.

Quoted 3 lines

So it might be that #slots decoupling from memory device a premature action and we can still use it with virtio-mem/pmem.

I treat #slots on x86 as #acpi_slots, that's why I don't think they apply here. I can see how they are used (on x86 only!) to size the address space - but I consider this a "nice to have feature" that should not be supposed to work in all possible scenarios.

E.g. powerpc already relies on sane user configs. #slots is not used to size the guest address space. Similar things will apply on s390x.

Quoted 4 lines

And another point against introducing a check for the maimum alignment would be: There might be users with a specified alignment > max page size. Changing the behavior will break these setups (strange but true).

check is already there implicitly as assert, and you're removing it with this patch.
What I'm suggesting is to drop this patch or replace assert with graceful error.
If you have actual usecase that requires this check being dropped, then commit message should ddescribe it properly and the patch should be a part of series that introduces that requirement.

Quoted 54 lines

Quoted 11 lines

Quoted 4 lines

virtio-mem devices will later have a certain block size (e.g. 4MB). I want to give devices during resource assignment the possibility to specify their alignment requirements.

size and alignment are 2 diffrent things here, alignment in our design is dictated by backing storage page size and for performance reasons HVA and GPA should be aligned on the same boundary, users are free to pick another GPA manually as far as it has the same alignment. But for automatic placement we use backend's alignment to make placement as compact as possible but still keeping GPA aligned with HVA.

Quoted 6 lines

Quoted 4 lines

For said virtio-mem device, this would e.g. be 4MB. (see patch 10 and 14 of how this call "get_align" comes into play), because the addresses of the memory blocks are all aligned by e.g. 4MB. This is what is guaranteed by the device specification.

where does this 4Mb magic comes from and why block must be aligned on this size?

(knowing it is hard to get the idea without the current prototype in place, but that will be fixed soon)

virtio-mem works in blocks. The block size is the size in which memory can be plugged or unplugged by the guest. It also determines the granularity (and therefore the bitmap) we have to use to keep track of unplugged memory. It is configurable (e.g. for migration purposes), but might also depend on the backend memory type. E.g. if huge pages are used in the host, the huge page size defines the minimum block size. But consider the last part a special case. Huge pages will not be supported for now.

The block size also defines the alignment that has to be used by the guest for plugging/unplugging (because that's how blocks gets mapped to the bitmap entries). So a virtio-mem device really needs a block-aligned start address,

For now I use 4MB because it matches what guests (e.g. Linux) can actually support and keeps the bitmap small. But as I said, it is configurable. (-device virtio-mem,block_size=1MB, ...)

Quoted 14 lines

Quoted 5 lines

E.g. for DIMMs we might want to allow to specify the section size (e.g. 128MB on x86), otherwise e.g. Linux is not able to add all memory. (but we should not hardcode this, as this is a Linux specific requirement - still it would be nice to specify)

true, it's guest specific and we do not have restrictions here. The only restriction we have here is that size must be multiple of backing storage page size (i.e. alignment) so that guest would be able to use tail page.

Quoted 2 lines

So in general, a memory device might have some alignment that is to be taken care of.

Do we really need introducing frontend specific alignment?
I'd try reuse backend's one and go for frontend's only in case we have to.

Quoted 4 lines

I think so. But I consider this a special case for virtio-mem. (specified indirectly via block_size). For the remaining stuff, we might be able to live with the memory backend aligment. And I agree that this should be the default if possible.

So if you make block_size to be multiple of backing storage alignment (i.e. page size) and we should keep GPA mapping (picking address to map region) based on backend's alignment. virtio-mem probably would even work even with huge pages at cost of increased granularity.

Quoted 25 lines

Quoted 10 lines

Quoted 10 lines

I don't understand right now why an upper limit on the alignment would make sense at all. We can easily handle it during our search. And we have to handle it either way during the search, if we plug some device with strange sizes (e.g. 1MB DIMM).

Of course, we might end up fragmenting guest physical memory, but that is purely a setup issue (choosing sizes of devices + main memory properly). I don't see a reason to error out (and of course also not to assert out :) ).

Quoted 7 lines

Agreed about assert, but I'd still prefer error out there so that users won't crate insane config and then complain (see below and *).

Upper alignment value is useful for proper sizing of hotplug address space, so that user could plug #slots devices upto #maxmem specified on CLI. It's still possible to misconfigure using manual placement, but default one just works, user either consumes all memory #maxmem and/or #slots.

Please not that it will still work in most cases. Only if people start to define crazy alignments (like I did), crazy DIMM sizes (e.g. 1MB) or crazy block sizes for virtio-mem, we might have a fragmented guest physical memory. This should usually not happen, but if so, we already have error messages for reporting this "fragmented memory".

Quoted 6 lines

And I consider these "strange configs" similar as "strange manual placement". Same result: fragmented memory, error out.

Personally, I don't think we have to guarantee that automatic placement works in all scenarios that the user is able to come up with. It should work in sane configurations, which is still that case.

It work now on x86 and I'd wish to keep it this way.

Quoted 5 lines

Quoted 5 lines

There is no misunderstanding in case of error here as it works same as on baremetal, one doesn't have a free place to put more memory or all memory one asked for is already there.

Quoted 8 lines

Quoted 3 lines

So it might be that #slots decoupling from memory device a premature action and we can still use it with virtio-mem/pmem.

I treat #slots on x86 as #acpi_slots, that's why I don't think they apply here. I can see how they are used (on x86 only!) to size the address space - but I consider this a "nice to have feature" that should not be supposed to work in all possible scenarios.

slot is more user space concept, as it is on real hw where you stick a plank with memory, so it's easily understood by users.
On ACPI side it's just implementation detail, we can go beyond that if it's needed without any adverse effects.
Restriction mostly comes from KVM side on # of memory slots and vhost.

Quoted 3 lines

E.g. powerpc already relies on sane user configs. #slots is not used to size the guest address space. Similar things will apply on s390x.

They both could benefit from it the same way as x86 providing properly sized area, so users won't have to scratch their head trying to understand why they can't plug more memory.

Some architectures might support memory devices, while they don't support DIMM/NVDIMM. So let's
- Rename CONFIG_MEM_HOTPLUG to CONFIG_MEM_DEVICE
- Introduce CONFIG_DIMM and use it similarly to CONFIG NVDIMM

CONFIG_DIMM and CONFIG_NVDIMM require CONFIG_MEM_DEVICE.

Signed-off-by David Hildenbrand <[email protected]>

---
 default-configs/i386-softmmu.mak   | 3 ++-
 default-configs/ppc64-softmmu.mak  | 3 ++-
 default-configs/x86_64-softmmu.mak | 3 ++-
 hw/Makefile.objs                   | 2 +-
 hw/mem/Makefile.objs               | 4 ++--
 qapi/misc.json                     | 2 +-
 6 files changed, 10 insertions(+), 7 deletions(-)

default-configs/i386-softmmu.mak +2 -1

index 8c7d4a0fa0..4c1637338b 100644
@@ -50,7 +50,8 @@ CONFIG_PCI_Q35=y
 CONFIG_APIC=y
 CONFIG_IOAPIC=y
 CONFIG_PVPANIC=y
-CONFIG_MEM_HOTPLUG=y
+CONFIG_MEM_DEVICE=y
+CONFIG_DIMM=y
 CONFIG_NVDIMM=y
 CONFIG_ACPI_NVDIMM=y
 CONFIG_PCIE_PORT=y

default-configs/ppc64-softmmu.mak +2 -1

index b94af6c7c6..f550573782 100644
@@ -16,4 +16,5 @@ CONFIG_VIRTIO_VGA=y
 CONFIG_XICS=$(CONFIG_PSERIES)
 CONFIG_XICS_SPAPR=$(CONFIG_PSERIES)
 CONFIG_XICS_KVM=$(call land,$(CONFIG_PSERIES),$(CONFIG_KVM))
-CONFIG_MEM_HOTPLUG=y
+CONFIG_MEM_DEVICE=y
+CONFIG_DIMM=y

default-configs/x86_64-softmmu.mak +2 -1

index 0390b4303c..7785351414 100644
@@ -50,7 +50,8 @@ CONFIG_PCI_Q35=y
 CONFIG_APIC=y
 CONFIG_IOAPIC=y
 CONFIG_PVPANIC=y
-CONFIG_MEM_HOTPLUG=y
+CONFIG_MEM_DEVICE=y
+CONFIG_DIMM=y
 CONFIG_NVDIMM=y
 CONFIG_ACPI_NVDIMM=y
 CONFIG_PCIE_PORT=y

hw/Makefile.objs +1 -1

index 6a0ffe0afd..127a60eca4 100644
@@ -33,7 +33,7 @@ devices-dirs-$(CONFIG_SOFTMMU) += vfio/
 devices-dirs-$(CONFIG_SOFTMMU) += virtio/
 devices-dirs-$(CONFIG_SOFTMMU) += watchdog/
 devices-dirs-$(CONFIG_SOFTMMU) += xen/
-devices-dirs-$(CONFIG_MEM_HOTPLUG) += mem/
+devices-dirs-$(CONFIG_MEM_DEVICE) += mem/
 devices-dirs-$(CONFIG_SOFTMMU) += smbios/
 devices-dirs-y += core/
 common-obj-y += $(devices-dirs-y)

hw/mem/Makefile.objs +2 -2

index 10be4df2a2..3e2f7c5ca2 100644
@@ -1,3 +1,3 @@
-common-obj-$(CONFIG_MEM_HOTPLUG) += pc-dimm.o
-common-obj-$(CONFIG_MEM_HOTPLUG) += memory-device.o
+common-obj-$(CONFIG_DIMM) += pc-dimm.o
+common-obj-$(CONFIG_MEM_DEVICE) += memory-device.o
 common-obj-$(CONFIG_NVDIMM) += nvdimm.o

qapi/misc.json +1 -1

index f5988cc0b5..4e6265cd2e 100644
@@ -2060,7 +2060,7 @@
 #
 # @plugged-memory: size of memory that can be hot-unplugged. This field
 #                  is omitted if target doesn't support memory hotplug
-#                  (i.e. CONFIG_MEM_HOTPLUG not defined on build time).
+#                  (i.e. CONFIG_MEM_DEVICE not defined at build time).
 #
 # Since: 2.11.0
 ##

From: Igor Mammedov <[email protected]>

it will allow to return another hotplug handler than the default one for a specific bus based device type. Which is needed to handle non trivial plug/unplug sequences that need the access to resources configured outside of bus where device is attached.

That will allow for returned hotplug handler to orchestrate wiring in arbitrary order, by chaining other hotplug handlers when it's needed.

PS:
It could be used for hybrid virtio-mem and virtio-pmem devices where it will return machine as hotplug handler which will do necessary wiring at machine level and then pass control down the chain to bus specific hotplug handler.

Example of top level hotplug handler override and custom plug sequence:

  some_machine_get_hotplug_handler(machine){
      if (object_dynamic_cast(OBJECT(dev), TYPE_SOME_BUS_DEVICE)) {
          return HOTPLUG_HANDLER(machine);
      }
      return NULL;
  }

  some_machine_device_plug(hotplug_dev, dev) {
      if (object_dynamic_cast(OBJECT(dev), TYPE_SOME_BUS_DEVICE)) {
          /* do machine specific initialization */
          some_machine_init_special_device(dev)

          /* pass control to bus specific handler */
          hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev)
      }

}

Signed-off-by Igor Mammedov <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/core/qdev.c         |  6 ++----
 include/hw/qdev-core.h | 11 +++++++++++
 2 files changed, 13 insertions(+), 4 deletions(-)

hw/core/qdev.c +2 -4

index f6f92473b8..885286f579 100644
@@ -261,12 +261,10 @@ HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
 
 HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
 {
-    HotplugHandler *hotplug_ctrl;
+    HotplugHandler *hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
 
-    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+    if (hotplug_ctrl == NULL && dev->parent_bus) {
         hotplug_ctrl = dev->parent_bus->hotplug_handler;
-    } else {
-        hotplug_ctrl = qdev_get_machine_hotplug_handler(dev);
     }
     return hotplug_ctrl;
 }

include/hw/qdev-core.h +11 -0

index 9453588160..e6a8eca558 100644
@@ -286,6 +286,17 @@ void qdev_init_nofail(DeviceState *dev);
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
 HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev);
+/**
+ * qdev_get_hotplug_handler: Get handler responsible for device wiring
+ *
+ * Find HOTPLUG_HANDLER for @dev that provides [pre|un]plug callbacks for it.
+ *
+ * Note: in case @dev has a parent bus, it will be returned as handler unless
+ * machine handler overrides it.
+ *
+ * Returns: pointer to object that implements TYPE_HOTPLUG_HANDLER interface
+ *          or NULL if there aren't any.
+ */
 HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev);
 void qdev_unplug(DeviceState *dev, Error **errp);
 void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,

For multi stage hotplug handlers, we'll have to do some error handling in some hotplug functions, so let's use a local error variable (except for unplug requests).

Also, add code to pass control to the final stage hotplug handler at the parent bus.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/i386/pc.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

hw/i386/pc.c +31 -8

index d768930d02..510076e156 100644
@@ -2007,19 +2007,32 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
 static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
                                           DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_pre_plug(hotplug_dev, dev, errp);
+        pc_cpu_pre_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
+                                 &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_plug(hotplug_dev, dev, errp);
+        pc_dimm_plug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_plug(hotplug_dev, dev, errp);
+        pc_cpu_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,

@@ -2029,7 +2042,10 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
         pc_dimm_unplug_request(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
-    } else {
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev,
+                                       errp);
+    } else if (!dev->parent_bus) {
         error_setg(errp, "acpi: device unplug request for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
     }

@@ -2038,14 +2054,21 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
 static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_unplug(hotplug_dev, dev, errp);
+        pc_dimm_unplug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_unplug_cb(hotplug_dev, dev, errp);
-    } else {
-        error_setg(errp, "acpi: device unplug for not supported device"
+        pc_cpu_unplug_cb(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
+                               &local_err);
+    } else if (!dev->parent_bus) {
+        error_setg(&local_err, "acpi: device unplug for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
     }
+    error_propagate(errp, local_err);
 }
 
 static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,

Quoted 3 lines · On Thu, 17 May 2018 10:15:17 +0200 David Hildenbrand <[email protected]> wrote:

For multi stage hotplug handlers, we'll have to do some error handling in some hotplug functions, so let's use a local error variable (except for unplug requests).

I'd split out introducing local error into separate patch so patch would do a single thing.

Quoted 2 lines

Also, add code to pass control to the final stage hotplug handler at the parent bus.

But I don't agree with generic
"} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {"
forwarding, it's done by 3/14 for generic case and in case of special device that needs bus handler called from machine one, I'd suggest to do forwarding explicitly for that device only like we do with acpi_dev.

From this handling it is clear that

Quoted 85 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/i386/pc.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

hw/i386/pc.c +31 -8

index d768930d02..510076e156 100644
@@ -2007,19 +2007,32 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
 static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
                                           DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_pre_plug(hotplug_dev, dev, errp);
+        pc_cpu_pre_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
+                                 &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_plug(hotplug_dev, dev, errp);
+        pc_dimm_plug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_plug(hotplug_dev, dev, errp);
+        pc_cpu_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,

@@ -2029,7 +2042,10 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
         pc_dimm_unplug_request(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
-    } else {
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev,
+                                       errp);
+    } else if (!dev->parent_bus) {
         error_setg(errp, "acpi: device unplug request for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
     }

@@ -2038,14 +2054,21 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
 static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_unplug(hotplug_dev, dev, errp);
+        pc_dimm_unplug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_unplug_cb(hotplug_dev, dev, errp);
-    } else {
-        error_setg(errp, "acpi: device unplug for not supported device"
+        pc_cpu_unplug_cb(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
+                               &local_err);
+    } else if (!dev->parent_bus) {
+        error_setg(&local_err, "acpi: device unplug for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
     }
+    error_propagate(errp, local_err);
 }
 
 static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,

Quoted 9 lines · Igor Mammedov

Quoted 3 lines · On Thu, 17 May 2018 10:15:17 +0200 David Hildenbrand <[email protected]> wrote:

For multi stage hotplug handlers, we'll have to do some error handling in some hotplug functions, so let's use a local error variable (except for unplug requests).

I'd split out introducing local error into separate patch so patch would do a single thing.

Quoted 8 lines

Quoted 2 lines

Also, add code to pass control to the final stage hotplug handler at the parent bus.

But I don't agree with generic
"} else if ("dev->parent_bus && dev->parent_bus->hotplug_handler) {"
forwarding, it's done by 3/14 for generic case and in case of special device that needs bus handler called from machine one, I'd suggest to do forwarding explicitly for that device only like we do with acpi_dev.

I decided to do it that way because it is generic and results in nicer recovery handling (e.g. in case pc_dimm plug fails, we can simply rollback all (for now MemoryDevice) previous plug operations).

IMHO, the resulting code is easier to read.

From this handling it is clear that
"if we reach the hotplug handler, and it is not some special device plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug handler if any exists"

Quoted 87 lines

Quoted 86 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/i386/pc.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

hw/i386/pc.c +31 -8

index d768930d02..510076e156 100644
@@ -2007,19 +2007,32 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
 static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
                                           DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_pre_plug(hotplug_dev, dev, errp);
+        pc_cpu_pre_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
+                                 &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void pc_machine_device_plug_cb(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_plug(hotplug_dev, dev, errp);
+        pc_dimm_plug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_plug(hotplug_dev, dev, errp);
+        pc_cpu_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,

@@ -2029,7 +2042,10 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
         pc_dimm_unplug_request(hotplug_dev, dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
-    } else {
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev,
+                                       errp);
+    } else if (!dev->parent_bus) {
         error_setg(errp, "acpi: device unplug request for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
     }

@@ -2038,14 +2054,21 @@ static void pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
 static void pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
                                         DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        pc_dimm_unplug(hotplug_dev, dev, errp);
+        pc_dimm_unplug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
-        pc_cpu_unplug_cb(hotplug_dev, dev, errp);
-    } else {
-        error_setg(errp, "acpi: device unplug for not supported device"
+        pc_cpu_unplug_cb(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
+                               &local_err);
+    } else if (!dev->parent_bus) {
+        error_setg(&local_err, "acpi: device unplug for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
     }
+    error_propagate(errp, local_err);
 }
 
 static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,  

Quoted 1 line · On Wed, 30 May 2018 16:13:32 +0200 David Hildenbrand <[email protected]> wrote:

On 30.05.2018 15:08, Igor Mammedov wrote:

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines · On Thu, 17 May 2018 10:15:17 +0200 David Hildenbrand <[email protected]> wrote:

For multi stage hotplug handlers, we'll have to do some error handling in some hotplug functions, so let's use a local error variable (except for unplug requests).

I'd split out introducing local error into separate patch so patch would do a single thing.

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

Also, add code to pass control to the final stage hotplug handler at the parent bus.

But I don't agree with generic
"} else if ("dev->parent_bus && dev->parent_bus->hotplug_handler) {"
forwarding, it's done by 3/14 for generic case and in case of special device that needs bus handler called from machine one, I'd suggest to do forwarding explicitly for that device only like we do with acpi_dev.

Quoted 3 lines

I decided to do it that way because it is generic and results in nicer recovery handling (e.g. in case pc_dimm plug fails, we can simply rollback all (for now MemoryDevice) previous plug operations).

rollback should be managed by the caller of pc_dimm plug directly, so it's not relevant here.

Quoted 6 lines

IMHO, the resulting code is easier to read.

From this handling it is clear that
"if we reach the hotplug handler, and it is not some special device plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug handler if any exists"

I strongly disagree with that it's easier to deal with. You are basically duplicating already generalized code from qdev_get_hotplug_handler() back into boards.

If a device doesn't have to be handled by machine handler, than qdev_get_hotplug_handler() must return its bus handler if any directly. So branch in question that your are copying is a dead one, pls drop it.

Quoted 4 lines · Igor Mammedov

Quoted 1 line · On Wed, 30 May 2018 16:13:32 +0200 David Hildenbrand <[email protected]> wrote:

On 30.05.2018 15:08, Igor Mammedov wrote:

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines · On Thu, 17 May 2018 10:15:17 +0200 David Hildenbrand <[email protected]> wrote:

For multi stage hotplug handlers, we'll have to do some error handling in some hotplug functions, so let's use a local error variable (except for unplug requests).

I'd split out introducing local error into separate patch so patch would do a single thing.

I can do that if it makes review easier.

Quoted 22 lines

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

Also, add code to pass control to the final stage hotplug handler at the parent bus.

But I don't agree with generic
"} else if ("dev->parent_bus && dev->parent_bus->hotplug_handler) {"
forwarding, it's done by 3/14 for generic case and in case of special device that needs bus handler called from machine one, I'd suggest to do forwarding explicitly for that device only like we do with acpi_dev.

Quoted 3 lines

I decided to do it that way because it is generic and results in nicer recovery handling (e.g. in case pc_dimm plug fails, we can simply rollback all (for now MemoryDevice) previous plug operations).

rollback should be managed by the caller of pc_dimm plug directly, so it's not relevant here.

Quoted 6 lines

IMHO, the resulting code is easier to read.

From this handling it is clear that
"if we reach the hotplug handler, and it is not some special device plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug handler if any exists"

Quoted 8 lines

I strongly disagree with that it's easier to deal with. You are basically duplicating already generalized code from qdev_get_hotplug_handler() back into boards.

If a device doesn't have to be handled by machine handler, than qdev_get_hotplug_handler() must return its bus handler if any directly. So branch in question that your are copying is a dead one, pls drop it.

We forward selected (pc_get_hotpug_handler()) devices to the right hotplug handler. Nothing wrong about that. I don't agree with "basically duplicating already generalized code" wrong. We have to forward at some place. Your idea simply places that code at some other place.

But I think we have to get the general idea sorted out first.

What you have in mind (e.g. plug):

if (TYPE_MEMORY_DEVICE) {
	memory_device_plug();
	if (local_err) {
		goto out;
	}

	if (TYPE_PC_DIMM) {
		pc_dimm_plug(hotplug_dev, dev, &local_err);
	} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
		hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
	}
	if (local_err) {
		memory_device_unplug()
		goto out;
	}
} else if (TYPE_CPU)

...

What I have in mind (and implemented in this series):

if (TYPE_MEMORY_DEVICE) {
	memory_device_plug();
}
/* possibly other interfaces */
if (local_err) {
	error_handling();
	return;
}

if (TYPE_PC_DIMM) {
	pc_dimm_plug(hotplug_dev, dev, &local_err);
} ...
} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
	hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
}

if (local_err) {
	if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
		memory_device_unplug()
	}
	/* possibly other interfaces */
}

...

I claim that my variant is more generic because:
- it easily supports multiple interfaces (like MemoryDevice) per Device that need a hotplug handler call
- there is only one call to hotplug_handler_plug() in case we add similar handling for another device

Apart from that they do exactly the same thing.

Quoted 89 lines · On Mon, 4 Jun 2018 13:27:01 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 4 lines · Igor Mammedov

Quoted 1 line · On Wed, 30 May 2018 16:13:32 +0200 David Hildenbrand <[email protected]> wrote:

On 30.05.2018 15:08, Igor Mammedov wrote:

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines · On Thu, 17 May 2018 10:15:17 +0200 David Hildenbrand <[email protected]> wrote:

For multi stage hotplug handlers, we'll have to do some error handling in some hotplug functions, so let's use a local error variable (except for unplug requests).

I'd split out introducing local error into separate patch so patch would do a single thing.

I can do that if it makes review easier.

Quoted 22 lines

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

Also, add code to pass control to the final stage hotplug handler at the parent bus.

But I don't agree with generic
"} else if ("dev->parent_bus && dev->parent_bus->hotplug_handler) {"
forwarding, it's done by 3/14 for generic case and in case of special device that needs bus handler called from machine one, I'd suggest to do forwarding explicitly for that device only like we do with acpi_dev.

Quoted 3 lines

I decided to do it that way because it is generic and results in nicer recovery handling (e.g. in case pc_dimm plug fails, we can simply rollback all (for now MemoryDevice) previous plug operations).

rollback should be managed by the caller of pc_dimm plug directly, so it's not relevant here.

Quoted 6 lines

IMHO, the resulting code is easier to read.

From this handling it is clear that
"if we reach the hotplug handler, and it is not some special device plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug handler if any exists"

Quoted 8 lines

I strongly disagree with that it's easier to deal with. You are basically duplicating already generalized code from qdev_get_hotplug_handler() back into boards.

If a device doesn't have to be handled by machine handler, than qdev_get_hotplug_handler() must return its bus handler if any directly. So branch in question that your are copying is a dead one, pls drop it.

We forward selected (pc_get_hotpug_handler()) devices to the right hotplug handler. Nothing wrong about that. I don't agree with "basically duplicating already generalized code" wrong. We have to forward at some place. Your idea simply places that code at some other place.

But I think we have to get the general idea sorted out first.

What you have in mind (e.g. plug):

if (TYPE_MEMORY_DEVICE) {
	memory_device_plug();
	if (local_err) {
		goto out;
	}

	if (TYPE_PC_DIMM) {
		pc_dimm_plug(hotplug_dev, dev, &local_err);
	} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
		hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
	}
	if (local_err) {
		memory_device_unplug()
		goto out;
	}
} else if (TYPE_CPU)

...

What I have in mind (and implemented in this series):

if (TYPE_MEMORY_DEVICE) {
	memory_device_plug();
}
/* possibly other interfaces */
if (local_err) {
	error_handling();
	return;
}

Quoted 6 lines

if (TYPE_PC_DIMM) {
	pc_dimm_plug(hotplug_dev, dev, &local_err);
} ...
} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
	hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
}

I don't like this implicit wiring, where reader of this part of code has no idea that TYPE_MEMORY_DEVICE might be also bus device that needs request forwarding.
I'd prefer [pre/un]plug handlers be concrete and explicit spaghetti code, something like this:

if (TYPE_PC_DIMM) {
    pc_dimm_plug()
    /* do here additional concrete machine specific things */
} else if (TYPE_VIRTIO_MEM) {
    virtio_mem_plug() <- do forwarding in there
    /* and do here additional concrete machine specific things */
} else if (TYPE_CPU) {
    cpu_plug()
    /* do here additional concrete machine specific things */
}

Quoted 7 lines

if (local_err) {
	if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
		memory_device_unplug()
	}
	/* possibly other interfaces */
}

Quoted 8 lines

...

I claim that my variant is more generic because:
- it easily supports multiple interfaces (like MemoryDevice) per Device that need a hotplug handler call
- there is only one call to hotplug_handler_plug() in case we add similar handling for another device

As someone said "one more layer of indirection would solve problem". But then one would have a clue how it works after a while (including author of the feature).
I don't think we have a problem here and need generalization.

Quoted 3 lines

Apart from that they do exactly the same thing.

Quoted 104 lines · Igor Mammedov

Quoted 89 lines · On Mon, 4 Jun 2018 13:27:01 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 4 lines · Igor Mammedov

Quoted 1 line · On Wed, 30 May 2018 16:13:32 +0200 David Hildenbrand <[email protected]> wrote:

On 30.05.2018 15:08, Igor Mammedov wrote:

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines · On Thu, 17 May 2018 10:15:17 +0200 David Hildenbrand <[email protected]> wrote:

For multi stage hotplug handlers, we'll have to do some error handling in some hotplug functions, so let's use a local error variable (except for unplug requests).

I'd split out introducing local error into separate patch so patch would do a single thing.

I can do that if it makes review easier.

Quoted 22 lines

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

Also, add code to pass control to the final stage hotplug handler at the parent bus.

But I don't agree with generic
"} else if ("dev->parent_bus && dev->parent_bus->hotplug_handler) {"
forwarding, it's done by 3/14 for generic case and in case of special device that needs bus handler called from machine one, I'd suggest to do forwarding explicitly for that device only like we do with acpi_dev.

Quoted 3 lines

I decided to do it that way because it is generic and results in nicer recovery handling (e.g. in case pc_dimm plug fails, we can simply rollback all (for now MemoryDevice) previous plug operations).

rollback should be managed by the caller of pc_dimm plug directly, so it's not relevant here.

Quoted 6 lines

IMHO, the resulting code is easier to read.

From this handling it is clear that
"if we reach the hotplug handler, and it is not some special device plugged by the machine (CPU, PC_DIMM), pass it on to the actual hotplug handler if any exists"

Quoted 8 lines

I strongly disagree with that it's easier to deal with. You are basically duplicating already generalized code from qdev_get_hotplug_handler() back into boards.

If a device doesn't have to be handled by machine handler, than qdev_get_hotplug_handler() must return its bus handler if any directly. So branch in question that your are copying is a dead one, pls drop it.

We forward selected (pc_get_hotpug_handler()) devices to the right hotplug handler. Nothing wrong about that. I don't agree with "basically duplicating already generalized code" wrong. We have to forward at some place. Your idea simply places that code at some other place.

But I think we have to get the general idea sorted out first.

What you have in mind (e.g. plug):

if (TYPE_MEMORY_DEVICE) {
	memory_device_plug();
	if (local_err) {
		goto out;
	}

	if (TYPE_PC_DIMM) {
		pc_dimm_plug(hotplug_dev, dev, &local_err);
	} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
		hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
	}
	if (local_err) {
		memory_device_unplug()
		goto out;
	}
} else if (TYPE_CPU)

...

What I have in mind (and implemented in this series):

if (TYPE_MEMORY_DEVICE) {
	memory_device_plug();
}
/* possibly other interfaces */
if (local_err) {
	error_handling();
	return;
}

Quoted 6 lines

if (TYPE_PC_DIMM) {
	pc_dimm_plug(hotplug_dev, dev, &local_err);
} ...
} else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
	hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
}  

I don't like this implicit wiring, where reader of this part of code has no idea that TYPE_MEMORY_DEVICE might be also bus device that needs request forwarding.
I'd prefer [pre/un]plug handlers be concrete and explicit spaghetti code, something like this:

Quoted 10 lines

if (TYPE_PC_DIMM) {
    pc_dimm_plug()
    /* do here additional concrete machine specific things */
} else if (TYPE_VIRTIO_MEM) {
    virtio_mem_plug() <- do forwarding in there
    /* and do here additional concrete machine specific things */
} else if (TYPE_CPU) {
    cpu_plug()
    /* do here additional concrete machine specific things */
}  

That will result in a lot of duplicate code - for every machine we support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - virtio-mem and virtio-pmem could most probably share the code.

Quoted 25 lines

Quoted 8 lines

  
if (local_err) {
	if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
		memory_device_unplug()
	}
	/* possibly other interfaces */
}

Quoted 8 lines

...

I claim that my variant is more generic because:
- it easily supports multiple interfaces (like MemoryDevice) per Device that need a hotplug handler call
- there is only one call to hotplug_handler_plug() in case we add similar handling for another device

As someone said "one more layer of indirection would solve problem". But then one would have a clue how it works after a while (including author of the feature).
I don't think we have a problem here and need generalization.

Quoted 3 lines

Apart from that they do exactly the same thing.

Quoted 12 lines

Quoted 11 lines

if (TYPE_PC_DIMM) {
    pc_dimm_plug()
    /* do here additional concrete machine specific things */
} else if (TYPE_VIRTIO_MEM) {
    virtio_mem_plug() <- do forwarding in there
    /* and do here additional concrete machine specific things */
} else if (TYPE_CPU) {
    cpu_plug()
    /* do here additional concrete machine specific things */
}  

Quoted 3 lines

That will result in a lot of duplicate code - for every machine we support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - virtio-mem and virtio-pmem could most probably share the code.

maybe or maybe not, depending on if pmem endups as memory device or separate controller. And even with duplication, machine code would be easy to follow just down one explicit call chain.

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

if (TYPE_PC_DIMM) {
    pc_dimm_plug()
    /* do here additional concrete machine specific things */
} else if (TYPE_VIRTIO_MEM) {
    virtio_mem_plug() <- do forwarding in there
    /* and do here additional concrete machine specific things */
} else if (TYPE_CPU) {
    cpu_plug()
    /* do here additional concrete machine specific things */
}  

Quoted 6 lines

Quoted 3 lines

That will result in a lot of duplicate code - for every machine we support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - virtio-mem and virtio-pmem could most probably share the code.

maybe or maybe not, depending on if pmem endups as memory device or separate controller. And even with duplication, machine code would be easy to follow just down one explicit call chain.

Not 100% convinced but I am now going into that direction.

Signature · 3 lines

Thanks,

David / dhildenb

Quoted 15 lines · David Hildenbrand

Quoted 15 lines

Quoted 15 lines

Quoted 11 lines

if (TYPE_PC_DIMM) {
    pc_dimm_plug()
    /* do here additional concrete machine specific things */
} else if (TYPE_VIRTIO_MEM) {
    virtio_mem_plug() <- do forwarding in there
    /* and do here additional concrete machine specific things */
} else if (TYPE_CPU) {
    cpu_plug()
    /* do here additional concrete machine specific things */
}  

That will result in a lot of duplicate code - for every machine we support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - virtio-mem and virtio-pmem could most probably share the code.

Quoted 5 lines

Quoted 3 lines

maybe or maybe not, depending on if pmem endups as memory device or separate controller. And even with duplication, machine code would be easy to follow just down one explicit call chain.

Not 100% convinced but I am now going into that direction.

Can this go into DeviceClass? Failover has the same need to allocate/free resources for vfio without a full realize/unrealize.

Quoted 5 lines

Signature · 3 lines

Thanks,

David / dhildenb

Quoted 16 lines · Michael S. Tsirkin

Quoted 15 lines · David Hildenbrand

Quoted 15 lines

Quoted 15 lines

Quoted 11 lines

if (TYPE_PC_DIMM) {
    pc_dimm_plug()
    /* do here additional concrete machine specific things */
} else if (TYPE_VIRTIO_MEM) {
    virtio_mem_plug() <- do forwarding in there
    /* and do here additional concrete machine specific things */
} else if (TYPE_CPU) {
    cpu_plug()
    /* do here additional concrete machine specific things */
}  

That will result in a lot of duplicate code - for every machine we support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - virtio-mem and virtio-pmem could most probably share the code.

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

maybe or maybe not, depending on if pmem endups as memory device or separate controller. And even with duplication, machine code would be easy to follow just down one explicit call chain.

Not 100% convinced but I am now going into that direction.

Can this go into DeviceClass? Failover has the same need to allocate/free resources for vfio without a full realize/unrealize.

Conceptually, I would have called here something like

virtio_mem_plug() ...

Which would end up calling memory_device_plug() and triggering the target hotplug handler.

I assume this can also be done from a device class callback.

So we would need a total of 3 callbacks for

a) pre_plug
b) plug
c) unplug

In addition, unplug requests might be necessary, so

d) unplug_request

Quoted 5 lines

Quoted 5 lines

Signature · 3 lines

Thanks,

David / dhildenb

Signature · 3 lines

Thanks,

David / dhildenb

Quoted 42 lines · David Hildenbrand

Quoted 16 lines · Michael S. Tsirkin

Quoted 15 lines · David Hildenbrand

Quoted 15 lines

Quoted 15 lines

Quoted 11 lines

if (TYPE_PC_DIMM) {
    pc_dimm_plug()
    /* do here additional concrete machine specific things */
} else if (TYPE_VIRTIO_MEM) {
    virtio_mem_plug() <- do forwarding in there
    /* and do here additional concrete machine specific things */
} else if (TYPE_CPU) {
    cpu_plug()
    /* do here additional concrete machine specific things */
}  

That will result in a lot of duplicate code - for every machine we support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - virtio-mem and virtio-pmem could most probably share the code.

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

maybe or maybe not, depending on if pmem endups as memory device or separate controller. And even with duplication, machine code would be easy to follow just down one explicit call chain.

Not 100% convinced but I am now going into that direction.

Can this go into DeviceClass? Failover has the same need to allocate/free resources for vfio without a full realize/unrealize.

Conceptually, I would have called here something like

virtio_mem_plug() ...

Which would end up calling memory_device_plug() and triggering the target hotplug handler.

I assume this can also be done from a device class callback.

So we would need a total of 3 callbacks for

a) pre_plug
b) plug
c) unplug

Quoted 3 lines

In addition, unplug requests might be necessary, so

d) unplug_request

Right - basically HotplugHandlerClass.

Quoted 12 lines

Quoted 5 lines

Quoted 5 lines

Signature · 3 lines

Thanks,

David / dhildenb

Signature · 3 lines

Thanks,

David / dhildenb

Quoted 43 lines · Michael S. Tsirkin

Quoted 42 lines · David Hildenbrand

Quoted 16 lines · Michael S. Tsirkin

Quoted 15 lines · David Hildenbrand

Quoted 15 lines

Quoted 15 lines

Quoted 11 lines

 
if (TYPE_PC_DIMM) {
    pc_dimm_plug()
    /* do here additional concrete machine specific things */
} else if (TYPE_VIRTIO_MEM) {
    virtio_mem_plug() <- do forwarding in there
    /* and do here additional concrete machine specific things */
} else if (TYPE_CPU) {
    cpu_plug()
    /* do here additional concrete machine specific things */
}    

That will result in a lot of duplicate code - for every machine we support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - virtio-mem and virtio-pmem could most probably share the code.

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

maybe or maybe not, depending on if pmem endups as memory device or separate controller. And even with duplication, machine code would be easy to follow just down one explicit call chain.

Not 100% convinced but I am now going into that direction.

Can this go into DeviceClass? Failover has the same need to allocate/free resources for vfio without a full realize/unrealize.

Conceptually, I would have called here something like

virtio_mem_plug() ...

Which would end up calling memory_device_plug() and triggering the target hotplug handler.

I assume this can also be done from a device class callback.

So we would need a total of 3 callbacks for

a) pre_plug
b) plug
c) unplug

Quoted 6 lines

Quoted 3 lines

In addition, unplug requests might be necessary, so

d) unplug_request

Right - basically HotplugHandlerClass.

Looking into this idea:

What I would have right now (conceptually)

if (TYPE_PC_DIMM) {
    pc_dimm_plug(machine);
} else if (TYPE_CPU) {
    cpu_plug(machine);
} else if (TYPE_VIRTIO_MEM) {
    virtio_mem_plug(machine);
}

Instead you want something like:

if (TYPE_PC_DIMM) {
    pc_dimm_plug(machine);
} else if (TYPE_CPU) {
    cpu_plug(machine);
// igor requested an explicit list here, we could also check for
// DEVICE_CLASS(device)->plug and make it generic
} else if (TYPE_VIRTIO_MEM) {
    DEVICE_CLASS(device)->plug(machine);
    // call bus hotplug handler if necessary, or let the previous call
    // handle it?

}

We cannot pass the machine directly (due to board.h and user-only), instead we would have to pass it as hotplug handler. Then, the device class code would however make assumptions that always a machine is passed.

Any opinions?

Quoted 12 lines

Quoted 12 lines

Quoted 5 lines

Quoted 5 lines

Signature · 3 lines

Thanks,

David / dhildenb  

Signature · 3 lines

Thanks,

David / dhildenb  

Quoted 65 lines · On Wed, 13 Jun 2018 12:58:46 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 43 lines · Michael S. Tsirkin

Quoted 42 lines · David Hildenbrand

Quoted 16 lines · Michael S. Tsirkin

Quoted 15 lines · David Hildenbrand

Quoted 15 lines

Quoted 15 lines

Quoted 11 lines

 
if (TYPE_PC_DIMM) {
    pc_dimm_plug()
    /* do here additional concrete machine specific things */
} else if (TYPE_VIRTIO_MEM) {
    virtio_mem_plug() <- do forwarding in there
    /* and do here additional concrete machine specific things */
} else if (TYPE_CPU) {
    cpu_plug()
    /* do here additional concrete machine specific things */
}    

That will result in a lot of duplicate code - for every machine we support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - virtio-mem and virtio-pmem could most probably share the code.

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

maybe or maybe not, depending on if pmem endups as memory device or separate controller. And even with duplication, machine code would be easy to follow just down one explicit call chain.

Not 100% convinced but I am now going into that direction.

Can this go into DeviceClass? Failover has the same need to allocate/free resources for vfio without a full realize/unrealize.

Conceptually, I would have called here something like

virtio_mem_plug() ...

Which would end up calling memory_device_plug() and triggering the target hotplug handler.

I assume this can also be done from a device class callback.

So we would need a total of 3 callbacks for

a) pre_plug
b) plug
c) unplug

Quoted 6 lines

Quoted 3 lines

In addition, unplug requests might be necessary, so

d) unplug_request

Right - basically HotplugHandlerClass.

Looking into this idea:

What I would have right now (conceptually)

if (TYPE_PC_DIMM) {
    pc_dimm_plug(machine);
} else if (TYPE_CPU) {
    cpu_plug(machine);
} else if (TYPE_VIRTIO_MEM) {
    virtio_mem_plug(machine);
}

Instead you want something like:

Quoted 10 lines

if (TYPE_PC_DIMM) {
    pc_dimm_plug(machine);
} else if (TYPE_CPU) {
    cpu_plug(machine);
// igor requested an explicit list here, we could also check for
// DEVICE_CLASS(device)->plug and make it generic
} else if (TYPE_VIRTIO_MEM) {
    DEVICE_CLASS(device)->plug(machine);
    // call bus hotplug handler if necessary, or let the previous call
    // handle it?

not exactly this, I suggested following:

[ ... specific to machine_foo wiring ...]

virtio_mem_plug() {
[... some machine specific wiring ...]

bus_hotplug_ctrl = qdev_get_bus_hotplug_handler() bus_hotplug_ctrl->plug(bus_hotplug_ctrl, dev)

[... some more machine specific wiring ...]
}

[ ... specific to machine_foo wiring ...]

i.e. device itself doesn't participate in attaching to external entities, those entities (machine or bus controller virtio device is attached to) do wiring on their own within their own domain.

Quoted 93 lines · Igor Mammedov

Quoted 65 lines · On Wed, 13 Jun 2018 12:58:46 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 43 lines · Michael S. Tsirkin

Quoted 42 lines · David Hildenbrand

Quoted 16 lines · Michael S. Tsirkin

Quoted 15 lines · David Hildenbrand

Quoted 15 lines

Quoted 15 lines

Quoted 11 lines

 
if (TYPE_PC_DIMM) {
    pc_dimm_plug()
    /* do here additional concrete machine specific things */
} else if (TYPE_VIRTIO_MEM) {
    virtio_mem_plug() <- do forwarding in there
    /* and do here additional concrete machine specific things */
} else if (TYPE_CPU) {
    cpu_plug()
    /* do here additional concrete machine specific things */
}    

That will result in a lot of duplicate code - for every machine we support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - virtio-mem and virtio-pmem could most probably share the code.

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

maybe or maybe not, depending on if pmem endups as memory device or separate controller. And even with duplication, machine code would be easy to follow just down one explicit call chain.

Not 100% convinced but I am now going into that direction.

Can this go into DeviceClass? Failover has the same need to allocate/free resources for vfio without a full realize/unrealize.

Conceptually, I would have called here something like

virtio_mem_plug() ...

Which would end up calling memory_device_plug() and triggering the target hotplug handler.

I assume this can also be done from a device class callback.

So we would need a total of 3 callbacks for

a) pre_plug
b) plug
c) unplug

Quoted 6 lines

Quoted 3 lines

In addition, unplug requests might be necessary, so

d) unplug_request

Right - basically HotplugHandlerClass.

Looking into this idea:

What I would have right now (conceptually)

if (TYPE_PC_DIMM) {
    pc_dimm_plug(machine);
} else if (TYPE_CPU) {
    cpu_plug(machine);
} else if (TYPE_VIRTIO_MEM) {
    virtio_mem_plug(machine);
}

Instead you want something like:

Quoted 10 lines

if (TYPE_PC_DIMM) {
    pc_dimm_plug(machine);
} else if (TYPE_CPU) {
    cpu_plug(machine);
// igor requested an explicit list here, we could also check for
// DEVICE_CLASS(device)->plug and make it generic
} else if (TYPE_VIRTIO_MEM) {
    DEVICE_CLASS(device)->plug(machine);
    // call bus hotplug handler if necessary, or let the previous call
    // handle it?

not exactly this, I suggested following:

[ ... specific to machine_foo wiring ...]

virtio_mem_plug() {
[... some machine specific wiring ...]

bus_hotplug_ctrl = qdev_get_bus_hotplug_handler() bus_hotplug_ctrl->plug(bus_hotplug_ctrl, dev)

[... some more machine specific wiring ...]
}

[ ... specific to machine_foo wiring ...]

Quoted 3 lines

i.e. device itself doesn't participate in attaching to external entities, those entities (machine or bus controller virtio device is attached to) do wiring on their own within their own domain.

I am fine with this, but Michael asked if this ("virtio_mem_plug()") could go via new DeviceClass functions. Michael, would that also work for your use case?

Signature · 5 lines

-- 

Thanks,

David / dhildenb

Quoted 17 lines

Quoted 16 lines

[ ... specific to machine_foo wiring ...]

virtio_mem_plug() {
[... some machine specific wiring ...]

bus_hotplug_ctrl = qdev_get_bus_hotplug_handler() bus_hotplug_ctrl->plug(bus_hotplug_ctrl, dev)

[... some more machine specific wiring ...]
}

[ ... specific to machine_foo wiring ...]

i.e. device itself doesn't participate in attaching to external entities, those entities (machine or bus controller virtio device is attached to) do wiring on their own within their own domain.

Quoted 3 lines

I am fine with this, but Michael asked if this ("virtio_mem_plug()") could go via new DeviceClass functions. Michael, would that also work for your use case?

It's not virtio specifically, I'm interested in how this will work for PCI generally. Right now we call do_pci_register_device which immediately makes it guest visible.

Quoted 8 lines

Signature · 5 lines

-- 

Thanks,

David / dhildenb

Quoted 21 lines

Quoted 20 lines

Quoted 8 lines

[ ... specific to machine_foo wiring ...]

virtio_mem_plug() {
[... some machine specific wiring ...]

bus_hotplug_ctrl = qdev_get_bus_hotplug_handler() bus_hotplug_ctrl->plug(bus_hotplug_ctrl, dev)

Quoted 8 lines

[... some more machine specific wiring ...]
}

[ ... specific to machine_foo wiring ...]

i.e. device itself doesn't participate in attaching to external entities, those entities (machine or bus controller virtio device is attached to) do wiring on their own within their own domain.

I am fine with this, but Michael asked if this ("virtio_mem_plug()") could go via new DeviceClass functions. Michael, would that also work for your use case?

Quoted 3 lines

It's not virtio specifically, I'm interested in how this will work for PCI generally. Right now we call do_pci_register_device which immediately makes it guest visible.

So you're telling me that a PCI device exposes itself to the system in pci_qdev_realize() instead of letting a hotplug handler handle that? My assumption is that the PCI bridge hotplug handler handles this.

What am
I missing?

I can see that e.g. for a virtio device the realize call chain is

pci_qdev_realize() -> virtio_pci_realize() -> virtio_XXX__pci_realize -> virtio_XXX_realize()

If any realization in pci_qdev_realize() fails, we do a do_pci_unregister_device().

So if it is true what you're saying than we're already exposing partially realized (and possibly unrealized again) devices via PCI. I *guess* because we're holding the iothread mutex this is okay and actually not visible.

And we only seem to be sending events in the PCI bridge hotplug handlers, so my assumption is that this is fine.

Quoted 8 lines

Quoted 8 lines

Signature · 5 lines

-- 

Thanks,

David / dhildenb

Signature · 3 lines

Thanks,

David / dhildenb

Quoted 21 lines · David Hildenbrand

Quoted 21 lines

Quoted 20 lines

Quoted 8 lines

[ ... specific to machine_foo wiring ...]

virtio_mem_plug() {
[... some machine specific wiring ...]

bus_hotplug_ctrl = qdev_get_bus_hotplug_handler() bus_hotplug_ctrl->plug(bus_hotplug_ctrl, dev)

Quoted 8 lines

[... some more machine specific wiring ...]
}

[ ... specific to machine_foo wiring ...]

i.e. device itself doesn't participate in attaching to external entities, those entities (machine or bus controller virtio device is attached to) do wiring on their own within their own domain.

I am fine with this, but Michael asked if this ("virtio_mem_plug()") could go via new DeviceClass functions. Michael, would that also work for your use case?

Quoted 7 lines

Quoted 3 lines

It's not virtio specifically, I'm interested in how this will work for PCI generally. Right now we call do_pci_register_device which immediately makes it guest visible.

So you're telling me that a PCI device exposes itself to the system in pci_qdev_realize() instead of letting a hotplug handler handle that? My assumption is that the PCI bridge hotplug handler handles this.

Well given how things work in qemu that's not exactly the case. See below.

Quoted 11 lines

What am
I missing?

I can see that e.g. for a virtio device the realize call chain is

pci_qdev_realize() -> virtio_pci_realize() -> virtio_XXX__pci_realize -> virtio_XXX_realize()

If any realization in pci_qdev_realize() fails, we do a do_pci_unregister_device().

Quoted 4 lines

So if it is true what you're saying than we're already exposing partially realized (and possibly unrealized again) devices via PCI. I *guess* because we're holding the iothread mutex this is okay and actually not visible.

For now but we need ability to have separate new commands for realize and plug, so we will drop the mutex.

Quoted 3 lines

And we only seem to be sending events in the PCI bridge hotplug handlers, so my assumption is that this is fine.

For core PCI, it's mostly just this line:

    bus->devices[devfn] = pci_dev;

which makes it accessible to pci config cycles.

But failover also cares about vfio, which seems to set up e.g. irqfs on realize.

Quoted 49 lines · Michael S. Tsirkin

Quoted 21 lines · David Hildenbrand

Quoted 21 lines

Quoted 20 lines

Quoted 8 lines

[ ... specific to machine_foo wiring ...]

virtio_mem_plug() {
[... some machine specific wiring ...]

bus_hotplug_ctrl = qdev_get_bus_hotplug_handler() bus_hotplug_ctrl->plug(bus_hotplug_ctrl, dev)

Quoted 8 lines

[... some more machine specific wiring ...]
}

[ ... specific to machine_foo wiring ...]

i.e. device itself doesn't participate in attaching to external entities, those entities (machine or bus controller virtio device is attached to) do wiring on their own within their own domain.

I am fine with this, but Michael asked if this ("virtio_mem_plug()") could go via new DeviceClass functions. Michael, would that also work for your use case?

Quoted 7 lines

Quoted 3 lines

It's not virtio specifically, I'm interested in how this will work for PCI generally. Right now we call do_pci_register_device which immediately makes it guest visible.

So you're telling me that a PCI device exposes itself to the system in pci_qdev_realize() instead of letting a hotplug handler handle that? My assumption is that the PCI bridge hotplug handler handles this.

Well given how things work in qemu that's not exactly the case. See below.

Quoted 15 lines

What am
I missing?

I can see that e.g. for a virtio device the realize call chain is

pci_qdev_realize() -> virtio_pci_realize() -> virtio_XXX__pci_realize -> virtio_XXX_realize()

If any realization in pci_qdev_realize() fails, we do a do_pci_unregister_device().

So if it is true what you're saying than we're already exposing partially realized (and possibly unrealized again) devices via PCI. I *guess* because we're holding the iothread mutex this is okay and actually not visible.

Quoted 2 lines

For now but we need ability to have separate new commands for realize and plug, so we will drop the mutex.

If you want to actually drop the mutex, I assume that quite some rework will be necessary (not only for this specific PCI hotplug handler case), most probably also in other code parts (to) - all of the hotplug/realize code seems to rely on the mutex being locked and not being dropped temporarily.

Quoted 6 lines

Quoted 3 lines

And we only seem to be sending events in the PCI bridge hotplug handlers, so my assumption is that this is fine.

For core PCI, it's mostly just this line:

Quoted 1 line

    bus->devices[devfn] = pci_dev;  

This should go into the hotplug handler if I am not wrong. From what I learned from Igor, this is a layer violation. Resource assignment should happen during pre_plug / plug. But then you might need a different way to "reserve" a function (if there could be races then with the lock temporarily dropped).

Quoted 4 lines

which makes it accessible to pci config cycles.

But failover also cares about vfio, which seems to set up e.g. irqfs on realize.

Quoted 93 lines · Igor Mammedov

Quoted 65 lines · On Wed, 13 Jun 2018 12:58:46 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 43 lines · Michael S. Tsirkin

Quoted 42 lines · David Hildenbrand

Quoted 16 lines · Michael S. Tsirkin

Quoted 15 lines · David Hildenbrand

Quoted 15 lines

Quoted 15 lines

Quoted 11 lines

   
if (TYPE_PC_DIMM) {
    pc_dimm_plug()
    /* do here additional concrete machine specific things */
} else if (TYPE_VIRTIO_MEM) {
    virtio_mem_plug() <- do forwarding in there
    /* and do here additional concrete machine specific things */
} else if (TYPE_CPU) {
    cpu_plug()
    /* do here additional concrete machine specific things */
}      

That will result in a lot of duplicate code - for every machine we support (dimm/virtio-mem/virtio-pmem/*add more memory devices here*) - virtio-mem and virtio-pmem could most probably share the code.

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

maybe or maybe not, depending on if pmem endups as memory device or separate controller. And even with duplication, machine code would be easy to follow just down one explicit call chain.

Not 100% convinced but I am now going into that direction.

Can this go into DeviceClass? Failover has the same need to allocate/free resources for vfio without a full realize/unrealize.

Conceptually, I would have called here something like

virtio_mem_plug() ...

Which would end up calling memory_device_plug() and triggering the target hotplug handler.

I assume this can also be done from a device class callback.

So we would need a total of 3 callbacks for

a) pre_plug
b) plug
c) unplug

Quoted 6 lines

Quoted 3 lines

In addition, unplug requests might be necessary, so

d) unplug_request

Right - basically HotplugHandlerClass.

Looking into this idea:

What I would have right now (conceptually)

if (TYPE_PC_DIMM) {
    pc_dimm_plug(machine);
} else if (TYPE_CPU) {
    cpu_plug(machine);
} else if (TYPE_VIRTIO_MEM) {
    virtio_mem_plug(machine);
}

Instead you want something like:

Quoted 10 lines

if (TYPE_PC_DIMM) {
    pc_dimm_plug(machine);
} else if (TYPE_CPU) {
    cpu_plug(machine);
// igor requested an explicit list here, we could also check for
// DEVICE_CLASS(device)->plug and make it generic
} else if (TYPE_VIRTIO_MEM) {
    DEVICE_CLASS(device)->plug(machine);
    // call bus hotplug handler if necessary, or let the previous call
    // handle it?  

not exactly this, I suggested following:

[ ... specific to machine_foo wiring ...]

virtio_mem_plug() {
[... some machine specific wiring ...]

bus_hotplug_ctrl = qdev_get_bus_hotplug_handler() bus_hotplug_ctrl->plug(bus_hotplug_ctrl, dev)

[... some more machine specific wiring ...]
}

[ ... specific to machine_foo wiring ...]

Quoted 3 lines

i.e. device itself doesn't participate in attaching to external entities, those entities (machine or bus controller virtio device is attached to) do wiring on their own within their own domain.

I am fine with this, but Michael asked if this ("virtio_mem_plug()") could go via new DeviceClass functions. Michael, would that also work for your use case?

Necessary to hotplug them cleanly later. We could drop the PC_DIMM check, as PC_DIMM are just memory devices, but this approach is cleaner.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/i386/pc.c | 2 ++
 1 file changed, 2 insertions(+)

hw/i386/pc.c +1 -0

index 510076e156..8bc41ef24b 100644
@@ -74,6 +74,7 @@
 #include "hw/nmi.h"
 #include "hw/i386/intel_iommu.h"
 #include "hw/net/ne2000-isa.h"
+#include "hw/mem/memory-device.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ

hw/i386/pc.c +0 -0

         object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         return HOTPLUG_HANDLER(machine);
     }

Quoted 21 lines · On Thu, 17 May 2018 10:15:18 +0200 David Hildenbrand <[email protected]> wrote:

Necessary to hotplug them cleanly later. We could drop the PC_DIMM check, as PC_DIMM are just memory devices, but this approach is cleaner.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/i386/pc.c | 2 ++
 1 file changed, 2 insertions(+)

hw/i386/pc.c +1 -0

index 510076e156..8bc41ef24b 100644
@@ -74,6 +74,7 @@
 #include "hw/nmi.h"
 #include "hw/i386/intel_iommu.h"
 #include "hw/net/ne2000-isa.h"
+#include "hw/mem/memory-device.h"
 
 /* debug PC/ISA interrupts */
 //#define DEBUG_IRQ

hw/i386/pc.c static HotplugHandler *pc_get_hotpug_handler(MachineState *machine, · line 2079

Quoted 4 lines

hw/i386/pc.c +1 -0

@@ -2075,6 +2076,7 @@ static HotplugHandler *pc_get_hotpug_handler(MachineState *machine,
                                              DeviceState *dev)
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
+        object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE) ||

you probably could drop TYPE_PC_DIMM above, it's redundant since DIMM can be cast to TYPE_MEMORY_DEVICE

ditto for spapr

Quoted 3 lines

hw/i386/pc.c +0 -0

         object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         return HOTPLUG_HANDLER(machine);
     }

Quoted 7 lines

Quoted 2 lines

     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||

+ object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE) ||

you probably could drop TYPE_PC_DIMM above, it's redundant since DIMM can be cast to TYPE_MEMORY_DEVICE

ditto for spapr

Yes, had the same in mind but left it for now this way (basically because we do special handling for PC_DIMM, so anybody reading this code is not directly confused)

Quoted 7 lines · David Hildenbrand

Quoted 7 lines

Quoted 2 lines

     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||

+ object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE) ||

you probably could drop TYPE_PC_DIMM above, it's redundant since DIMM can be cast to TYPE_MEMORY_DEVICE

ditto for spapr

Quoted 4 lines

Yes, had the same in mind but left it for now this way (basically because we do special handling for PC_DIMM, so anybody reading this code is not directly confused)

Hmm, I think what Igor proposes is nicer. Can you send a v5 when he's done with review?

Paolo

For multi stage hotplug handlers, we'll have to do some error handling in some hotplug functions, so let's use a local error variable (except for unplug requests).

Also, add code to pass control to the final stage hotplug handler at the parent bus.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 11 deletions(-)

hw/ppc/spapr.c +2 -0

index ebf30dd60b..b7c5c95f7a 100644
@@ -3571,27 +3571,48 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
 {
     MachineState *ms = MACHINE(hotplug_dev);
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
+    Error *local_err = NULL;
 
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         int node;

hw/ppc/spapr.c +38 -7

-            return;
+        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP,
+                                        &local_err);
+        if (local_err) {
+            goto out;
         }
         if (node < 0 || node >= MAX_NODES) {
-            error_setg(errp, "Invaild node %d", node);
-            return;
+            error_setg(&local_err, "Invaild node %d", node);
+            goto out;
         }
 
-        spapr_memory_plug(hotplug_dev, dev, node, errp);
+        spapr_memory_plug(hotplug_dev, dev, node, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
-        spapr_core_plug(hotplug_dev, dev, errp);
+        spapr_core_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
+    }
+out:
+    error_propagate(errp, local_err);
+}
+
+static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
+                                        DeviceState *dev, Error **errp)
+{
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
+    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
+                               &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,

@@ -3618,17 +3639,27 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
             return;
         }
         spapr_core_unplug_request(hotplug_dev, dev, errp);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev,
+                                       errp);
     }
 }
 
 static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
                                           DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        spapr_memory_pre_plug(hotplug_dev, dev, errp);
+        spapr_memory_pre_plug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
-        spapr_core_pre_plug(hotplug_dev, dev, errp);
+        spapr_core_pre_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
+                                 &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,

@@ -3988,6 +4019,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->get_default_cpu_node_id = spapr_get_default_cpu_node_id;
     mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids;
     hc->unplug_request = spapr_machine_device_unplug_request;
+    hc->unplug = spapr_machine_device_unplug;
 
     smc->dr_lmb_enabled = true;
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");

For multi stage hotplug handlers, we'll have to do some error handling in some hotplug functions, so let's use a local error variable (except for unplug requests).

Also, add code to pass control to the final stage hotplug handler at the parent bus.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 11 deletions(-)

hw/ppc/spapr.c +43 -11

index ebf30dd60b..b7c5c95f7a 100644
@@ -3571,27 +3571,48 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
 {
     MachineState *ms = MACHINE(hotplug_dev);
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
+    Error *local_err = NULL;
 
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         int node;
 
         if (!smc->dr_lmb_enabled) {
-            error_setg(errp, "Memory hotplug not supported for this machine");
-            return;
+            error_setg(&local_err,
+                       "Memory hotplug not supported for this machine");
+            goto out;
         }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp);
-        if (*errp) {
-            return;
+        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP,
+                                        &local_err);
+        if (local_err) {
+            goto out;
         }
         if (node < 0 || node >= MAX_NODES) {
-            error_setg(errp, "Invaild node %d", node);
-            return;
+            error_setg(&local_err, "Invaild node %d", node);
+            goto out;
         }
 
-        spapr_memory_plug(hotplug_dev, dev, node, errp);
+        spapr_memory_plug(hotplug_dev, dev, node, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
-        spapr_core_plug(hotplug_dev, dev, errp);
+        spapr_core_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
+    }
+out:
+    error_propagate(errp, local_err);
+}
+
+static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
+                                        DeviceState *dev, Error **errp)
+{
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
+    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
+                               &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,

@@ -3618,17 +3639,27 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
             return;
         }
         spapr_core_unplug_request(hotplug_dev, dev, errp);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev,
+                                       errp);
     }
 }
 
 static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
                                           DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        spapr_memory_pre_plug(hotplug_dev, dev, errp);
+        spapr_memory_pre_plug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
-        spapr_core_pre_plug(hotplug_dev, dev, errp);
+        spapr_core_pre_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
+                                 &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,

@@ -3988,6 +4019,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->get_default_cpu_node_id = spapr_get_default_cpu_node_id;
     mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids;
     hc->unplug_request = spapr_machine_device_unplug_request;
+    hc->unplug = spapr_machine_device_unplug;
 
     smc->dr_lmb_enabled = true;
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");

For multi stage hotplug handlers, we'll have to do some error handling in some hotplug functions, so let's use a local error variable (except for unplug requests).

Also, add code to pass control to the final stage hotplug handler at the parent bus.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 11 deletions(-)

hw/ppc/spapr.c +17 -9

index ebf30dd60b..b7c5c95f7a 100644
@@ -3571,27 +3571,48 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
 {
     MachineState *ms = MACHINE(hotplug_dev);
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
+    Error *local_err = NULL;
 
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         int node;
 
         if (!smc->dr_lmb_enabled) {
-            error_setg(errp, "Memory hotplug not supported for this machine");
-            return;
+            error_setg(&local_err,
+                       "Memory hotplug not supported for this machine");
+            goto out;
         }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp);
-        if (*errp) {
-            return;
+        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP,
+                                        &local_err);
+        if (local_err) {
+            goto out;
         }
         if (node < 0 || node >= MAX_NODES) {
-            error_setg(errp, "Invaild node %d", node);
-            return;
+            error_setg(&local_err, "Invaild node %d", node);
+            goto out;
         }
 
-        spapr_memory_plug(hotplug_dev, dev, node, errp);
+        spapr_memory_plug(hotplug_dev, dev, node, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
-        spapr_core_plug(hotplug_dev, dev, errp);
+        spapr_core_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
+    }
+out:

hw/ppc/spapr.c +16 -2

+        hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
+                               &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,

@@ -3618,17 +3639,27 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev,
             return;
         }
         spapr_core_unplug_request(hotplug_dev, dev, errp);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev,
+                                       errp);
     }
 }
 
 static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
                                           DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        spapr_memory_pre_plug(hotplug_dev, dev, errp);
+        spapr_memory_pre_plug(hotplug_dev, dev, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
-        spapr_core_pre_plug(hotplug_dev, dev, errp);
+        spapr_core_pre_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
+                                 &local_err);
     }
+    error_propagate(errp, local_err);
 }
 
 static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,

@@ -3988,6 +4019,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->get_default_cpu_node_id = spapr_get_default_cpu_node_id;
     mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids;
     hc->unplug_request = spapr_machine_device_unplug_request;
+    hc->unplug = spapr_machine_device_unplug;
 
     smc->dr_lmb_enabled = true;
     mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");

Quoted 63 lines · David Hildenbrand

For multi stage hotplug handlers, we'll have to do some error handling in some hotplug functions, so let's use a local error variable (except for unplug requests).

Also, add code to pass control to the final stage hotplug handler at the parent bus.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 11 deletions(-)

hw/ppc/spapr.c +23 -9

index ebf30dd60b..b7c5c95f7a 100644
@@ -3571,27 +3571,48 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
 {
     MachineState *ms = MACHINE(hotplug_dev);
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
+    Error *local_err = NULL;
 
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         int node;
 
         if (!smc->dr_lmb_enabled) {
-            error_setg(errp, "Memory hotplug not supported for this machine");
-            return;
+            error_setg(&local_err,
+                       "Memory hotplug not supported for this machine");
+            goto out;
         }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp);
-        if (*errp) {
-            return;
+        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP,
+                                        &local_err);
+        if (local_err) {
+            goto out;
         }
         if (node < 0 || node >= MAX_NODES) {
-            error_setg(errp, "Invaild node %d", node);
-            return;
+            error_setg(&local_err, "Invaild node %d", node);
+            goto out;
         }
 
-        spapr_memory_plug(hotplug_dev, dev, node, errp);
+        spapr_memory_plug(hotplug_dev, dev, node, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
-        spapr_core_plug(hotplug_dev, dev, errp);
+        spapr_core_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
+    }
+out:
+    error_propagate(errp, local_err);
+}
+
+static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
+                                        DeviceState *dev, Error **errp)
+{

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3611

Quoted 4 lines

hw/ppc/spapr.c +4 -0

+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
+    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {

As I think Igor said on the equivalent PC patch, I don't quite get this. Isn't this already handled by the generic hotplug code picking up the bus's hotplug handler if the machine doesn't supply one?

Quoted 64 lines · David Gibson

Quoted 63 lines · David Hildenbrand

For multi stage hotplug handlers, we'll have to do some error handling in some hotplug functions, so let's use a local error variable (except for unplug requests).

Also, add code to pass control to the final stage hotplug handler at the parent bus.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 11 deletions(-)

hw/ppc/spapr.c +23 -9

index ebf30dd60b..b7c5c95f7a 100644
@@ -3571,27 +3571,48 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
 {
     MachineState *ms = MACHINE(hotplug_dev);
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
+    Error *local_err = NULL;
 
+    /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         int node;
 
         if (!smc->dr_lmb_enabled) {
-            error_setg(errp, "Memory hotplug not supported for this machine");
-            return;
+            error_setg(&local_err,
+                       "Memory hotplug not supported for this machine");
+            goto out;
         }
-        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp);
-        if (*errp) {
-            return;
+        node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP,
+                                        &local_err);
+        if (local_err) {
+            goto out;
         }
         if (node < 0 || node >= MAX_NODES) {
-            error_setg(errp, "Invaild node %d", node);
-            return;
+            error_setg(&local_err, "Invaild node %d", node);
+            goto out;
         }
 
-        spapr_memory_plug(hotplug_dev, dev, node, errp);
+        spapr_memory_plug(hotplug_dev, dev, node, &local_err);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
-        spapr_core_plug(hotplug_dev, dev, errp);
+        spapr_core_plug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err);
+    }
+out:
+    error_propagate(errp, local_err);
+}
+
+static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
+                                        DeviceState *dev, Error **errp)
+{

Quoted 8 lines

hw/ppc/spapr.c static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, · line 3611

Quoted 4 lines

hw/ppc/spapr.c +4 -0

+    Error *local_err = NULL;
+
+    /* final stage hotplug handler */
+    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {  

As I think Igor said on the equivalent PC patch, I don't quite get this. Isn't this already handled by the generic hotplug code picking up the bus's hotplug handler if the machine doesn't supply one?

See my reply to patch nr 4.

What we do is, we install the machine hotplug handler as an "intermediate" hotplug handler.

E.g. if we have a VIRTIO based MemoryDevice, we have to initialize the MemoryDevice specific stuff in the machine hotplug handler, but then pass the device onto the last stage hotplug handler (which will eventually attach it to a bus and notify the guest).

Necessary to hotplug them cleanly later.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 1 +
 1 file changed, 1 insertion(+)

hw/ppc/spapr.c +1 -0

index b7c5c95f7a..2f315f963b 100644
@@ -3666,6 +3666,7 @@ static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,
                                                  DeviceState *dev)
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
+        object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE) ||
         object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
         return HOTPLUG_HANDLER(machine);
     }

Quoted 3 lines · David Hildenbrand

Necessary to hotplug them cleanly later.

Signed-off-by David Hildenbrand <[email protected]>

As for PC, I think it would be nicer to drop the explicit check against PC_DIMM, since it is covered by MEMORY_DEVICE.

Quoted 17 lines

---
 hw/ppc/spapr.c | 1 +
 1 file changed, 1 insertion(+)

hw/ppc/spapr.c +1 -0

index b7c5c95f7a..2f315f963b 100644
@@ -3666,6 +3666,7 @@ static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine,
                                                  DeviceState *dev)
 {
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
+        object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE) ||
         object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
         return HOTPLUG_HANDLER(machine);
     }

Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/ unplug memory devices (which a pc-dimm is) later.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

hw/ppc/spapr.c +5 -3

index 2f315f963b..286c38c842 100644
@@ -3291,7 +3291,8 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
 /* Callback to be called during DRC release. */
 void spapr_lmb_release(DeviceState *dev)
 {
-    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
+    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
+    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl);
     sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
 
     /* This information will get lost if a migration occurs

@@ -3309,9 +3310,21 @@ void spapr_lmb_release(DeviceState *dev)
 
     /*
      * Now that all the LMBs have been removed by the guest, call the
-     * pc-dimm unplug handler to cleanup up the pc-dimm device.
+     * unplug handler chain. This can never fail.
      */
-    pc_dimm_memory_unplug(dev, MACHINE(spapr));
+    hotplug_ctrl = qdev_get_hotplug_handler(dev);
+    hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);

hw/ppc/spapr.c +1 -0

+    pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev));
     object_unparent(OBJECT(dev));
     spapr_pending_dimm_unplugs_remove(spapr, ds);
 }

Let's handle it via hotplug_handler_unplug(). E.g. necessary to hotplug/ unplug memory devices (which a pc-dimm is) later.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

hw/ppc/spapr.c +2 -1

index 2f315f963b..286c38c842 100644
@@ -3291,7 +3291,8 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
 /* Callback to be called during DRC release. */
 void spapr_lmb_release(DeviceState *dev)
 {
-    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev));
+    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
+    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_ctrl);
     sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
 
     /* This information will get lost if a migration occurs

hw/ppc/spapr.c +15 -1

+    hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
+}
+
+static void spapr_memory_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                                Error **errp)
+{
+    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
+    sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev));
+
+    g_assert(ds);
+    g_assert(!ds->nr_lmbs);
+    pc_dimm_memory_unplug(dev, MACHINE(hotplug_dev));
     object_unparent(OBJECT(dev));
     spapr_pending_dimm_unplugs_remove(spapr, ds);
 }

@@ -3608,7 +3621,9 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
     Error *local_err = NULL;
 
     /* final stage hotplug handler */
-    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+        spapr_memory_unplug(hotplug_dev, dev, &local_err);
+    } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
         hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
                                &local_err);
     }

Let's handle it via hotplug_handler_unplug().

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

hw/ppc/spapr.c +12 -1

index 286c38c842..13d153b5a6 100644
@@ -3412,7 +3412,16 @@ static void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
 /* Callback to be called during DRC release. */
 void spapr_core_release(DeviceState *dev)
 {
-    MachineState *ms = MACHINE(qdev_get_hotplug_handler(dev));
+    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
+
+    /* Call the unplug handler chain. This can never fail. */
+    hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
+}
+
+static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                              Error **errp)
+{
+    MachineState *ms = MACHINE(hotplug_dev);
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
     CPUCore *cc = CPU_CORE(dev);
     CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);

@@ -3623,6 +3632,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
     /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         spapr_memory_unplug(hotplug_dev, dev, &local_err);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
+        spapr_core_unplug(hotplug_dev, dev, &local_err);
     } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
         hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
                                &local_err);

Let's handle it via hotplug_handler_unplug().

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/ppc/spapr.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

hw/ppc/spapr.c +12 -1

index 286c38c842..13d153b5a6 100644
@@ -3412,7 +3412,16 @@ static void *spapr_populate_hotplug_cpu_dt(CPUState *cs, int *fdt_offset,
 /* Callback to be called during DRC release. */
 void spapr_core_release(DeviceState *dev)
 {
-    MachineState *ms = MACHINE(qdev_get_hotplug_handler(dev));
+    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
+
+    /* Call the unplug handler chain. This can never fail. */
+    hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
+}
+
+static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
+                              Error **errp)
+{
+    MachineState *ms = MACHINE(hotplug_dev);
     sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
     CPUCore *cc = CPU_CORE(dev);
     CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);

@@ -3623,6 +3632,8 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
     /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         spapr_memory_unplug(hotplug_dev, dev, &local_err);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
+        spapr_core_unplug(hotplug_dev, dev, &local_err);
     } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
         hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev,
                                &local_err);

Let's move all pre-plug checks we can do without the device being realized into the applicable hotplug handler for pc and spapr.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/i386/pc.c                   | 11 +++++++
 hw/mem/memory-device.c         | 72 +++++++++++++++++++-----------------------
 hw/ppc/spapr.c                 | 11 +++++++
 include/hw/mem/memory-device.h |  2 ++
 4 files changed, 57 insertions(+), 39 deletions(-)

hw/i386/pc.c +11 -0

index 8bc41ef24b..61f1537e14 100644
@@ -2010,6 +2010,16 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
 {
     Error *local_err = NULL;
 
+    /* first stage hotplug handler */
+    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+        memory_device_pre_plug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev),
+                               &local_err);
+    }
+
+    if (local_err) {
+        goto out;
+    }
+
     /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_pre_plug(hotplug_dev, dev, &local_err);

@@ -2017,6 +2027,7 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
         hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
                                  &local_err);
     }
+out:
     error_propagate(errp, local_err);
 }
 

hw/mem/memory-device.c +21 -39

index 361d38bfc5..d22c91993f 100644
@@ -68,58 +68,26 @@ static int memory_device_used_region_size(Object *obj, void *opaque)
     return 0;
 }
 
-static void memory_device_check_addable(MachineState *ms, uint64_t size,
-                                        Error **errp)
-{
-    uint64_t used_region_size = 0;
-
-    /* we will need a new memory slot for kvm and vhost */
-    if (kvm_enabled() && !kvm_has_free_slot(ms)) {
-        error_setg(errp, "hypervisor has no free memory slots left");
-        return;
-    }
-    if (!vhost_has_free_slot()) {
-        error_setg(errp, "a used vhost backend has no free memory slots left");
-        return;
-    }
-
-    /* will we exceed the total amount of memory specified */
-    memory_device_used_region_size(OBJECT(ms), &used_region_size);
-    if (used_region_size + size > ms->maxram_size - ms->ram_size) {
-        error_setg(errp, "not enough space, currently 0x%" PRIx64
-                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
-                   used_region_size, ms->maxram_size - ms->ram_size);
-        return;
-    }
-
-}
-
 uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
                                      uint64_t align, uint64_t size,
                                      Error **errp)
 {
     uint64_t address_space_start, address_space_end;
+    uint64_t used_region_size = 0;
     GSList *list = NULL, *item;
     uint64_t new_addr = 0;
 
-    if (!ms->device_memory) {
-        error_setg(errp, "memory devices (e.g. for memory hotplug) are not "
-                         "supported by the machine");
-        return 0;
-    }
-
-    if (!memory_region_size(&ms->device_memory->mr)) {
-        error_setg(errp, "memory devices (e.g. for memory hotplug) are not "
-                         "enabled, please specify the maxmem option");
-        return 0;
-    }
     address_space_start = ms->device_memory->base;
     address_space_end = address_space_start +
                         memory_region_size(&ms->device_memory->mr);
     g_assert(address_space_end >= address_space_start);
 
-    memory_device_check_addable(ms, size, errp);
-    if (*errp) {
+    /* will we exceed the total amount of memory specified */
+    memory_device_used_region_size(OBJECT(ms), &used_region_size);
+    if (used_region_size + size > ms->maxram_size - ms->ram_size) {
+        error_setg(errp, "not enough space, currently 0x%" PRIx64
+                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
+                   used_region_size, ms->maxram_size - ms->ram_size);
         return 0;
     }
 

@@ -242,6 +210,32 @@ uint64_t get_plugged_memory_size(void)
     return size;
 }
 
+void memory_device_pre_plug(MachineState *ms, const MemoryDeviceState *md,
+                            Error **errp)
+{
+    if (!ms->device_memory) {
+        error_setg(errp, "memory devices (e.g. for memory hotplug) are not "
+                         "supported by the machine");
+        return;
+    }
+
+    if (!memory_region_size(&ms->device_memory->mr)) {
+        error_setg(errp, "memory devices (e.g. for memory hotplug) are not "
+                         "enabled, please specify the maxmem option");
+        return;
+    }

hw/mem/memory-device.c +2 -0

+}
+
 void memory_device_plug_region(MachineState *ms, MemoryRegion *mr,
                                uint64_t addr)
 {

hw/ppc/spapr.c +11 -0

index 13d153b5a6..562712def2 100644
@@ -3676,6 +3676,16 @@ static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
 {
     Error *local_err = NULL;
 
+    /* first stage hotplug handler */
+    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+        memory_device_pre_plug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev),
+                               &local_err);
+    }
+
+    if (local_err) {
+        goto out;
+    }
+
     /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         spapr_memory_pre_plug(hotplug_dev, dev, &local_err);

@@ -3685,6 +3695,7 @@ static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
         hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
                                  &local_err);
     }
+out:
     error_propagate(errp, local_err);
 }
 

include/hw/mem/memory-device.h +2 -0

index 62d906be50..3a4e9edc92 100644
@@ -51,6 +51,8 @@ typedef struct MemoryDeviceClass {
 
 MemoryDeviceInfoList *qmp_memory_device_list(void);
 uint64_t get_plugged_memory_size(void);
+void memory_device_pre_plug(MachineState *ms, const MemoryDeviceState *md,
+                            Error **errp);
 uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
                                      uint64_t align, uint64_t size,
                                      Error **errp);

Quoted 128 lines · On Thu, 17 May 2018 10:15:25 +0200 David Hildenbrand <[email protected]> wrote:

Let's move all pre-plug checks we can do without the device being realized into the applicable hotplug handler for pc and spapr.

Signed-off-by David Hildenbrand <[email protected]>

---
 hw/i386/pc.c                   | 11 +++++++
 hw/mem/memory-device.c         | 72 +++++++++++++++++++-----------------------
 hw/ppc/spapr.c                 | 11 +++++++
 include/hw/mem/memory-device.h |  2 ++
 4 files changed, 57 insertions(+), 39 deletions(-)

hw/i386/pc.c +11 -0

index 8bc41ef24b..61f1537e14 100644
@@ -2010,6 +2010,16 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
 {
     Error *local_err = NULL;
 
+    /* first stage hotplug handler */
+    if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
+        memory_device_pre_plug(MACHINE(hotplug_dev), MEMORY_DEVICE(dev),
+                               &local_err);
+    }
+
+    if (local_err) {
+        goto out;
+    }
+
     /* final stage hotplug handler */
     if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         pc_cpu_pre_plug(hotplug_dev, dev, &local_err);

@@ -2017,6 +2027,7 @@ static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
         hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev,
                                  &local_err);
     }
+out:
     error_propagate(errp, local_err);
 }
 

hw/mem/memory-device.c +21 -39

index 361d38bfc5..d22c91993f 100644
@@ -68,58 +68,26 @@ static int memory_device_used_region_size(Object *obj, void *opaque)
     return 0;
 }
 
-static void memory_device_check_addable(MachineState *ms, uint64_t size,
-                                        Error **errp)
-{
-    uint64_t used_region_size = 0;
-
-    /* we will need a new memory slot for kvm and vhost */
-    if (kvm_enabled() && !kvm_has_free_slot(ms)) {
-        error_setg(errp, "hypervisor has no free memory slots left");
-        return;
-    }
-    if (!vhost_has_free_slot()) {
-        error_setg(errp, "a used vhost backend has no free memory slots left");
-        return;
-    }
-
-    /* will we exceed the total amount of memory specified */
-    memory_device_used_region_size(OBJECT(ms), &used_region_size);
-    if (used_region_size + size > ms->maxram_size - ms->ram_size) {
-        error_setg(errp, "not enough space, currently 0x%" PRIx64
-                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
-                   used_region_size, ms->maxram_size - ms->ram_size);
-        return;
-    }
-
-}
-
 uint64_t memory_device_get_free_addr(MachineState *ms, const uint64_t *hint,
                                      uint64_t align, uint64_t size,
                                      Error **errp)
 {
     uint64_t address_space_start, address_space_end;
+    uint64_t used_region_size = 0;
     GSList *list = NULL, *item;
     uint64_t new_addr = 0;
 
-    if (!ms->device_memory) {
-        error_setg(errp, "memory devices (e.g. for memory hotplug) are not "
-                         "supported by the machine");
-        return 0;
-    }
-
-    if (!memory_region_size(&ms->device_memory->mr)) {
-        error_setg(errp, "memory devices (e.g. for memory hotplug) are not "
-                         "enabled, please specify the maxmem option");
-        return 0;
-    }
     address_space_start = ms->device_memory->base;
     address_space_end = address_space_start +
                         memory_region_size(&ms->device_memory->mr);
     g_assert(address_space_end >= address_space_start);
 
-    memory_device_check_addable(ms, size, errp);
-    if (*errp) {
+    /* will we exceed the total amount of memory specified */
+    memory_device_used_region_size(OBJECT(ms), &used_region_size);
+    if (used_region_size + size > ms->maxram_size - ms->ram_size) {
+        error_setg(errp, "not enough space, currently 0x%" PRIx64
+                   " in use of total hot pluggable 0x" RAM_ADDR_FMT,
+                   used_region_size, ms->maxram_size - ms->ram_size);
         return 0;
     }
 

@@ -242,6 +210,32 @@ uint64_t get_plugged_memory_size(void)
     return size;
 }
 
+void memory_device_pre_plug(MachineState *ms, const MemoryDeviceState *md,
+                            Error **errp)
+{
+    if (!ms->device_memory) {
+        error_setg(errp, "memory devices (e.g. for memory hotplug) are not "
+                         "supported by the machine");
+        return;
+    }
+
+    if (!memory_region_size(&ms->device_memory->mr)) {
+        error_setg(errp, "memory devices (e.g. for memory hotplug) are not "
+                         "enabled, please specify the maxmem option");
+        return;
+    }

hw/mem/memory-device.c uint64_t get_plugged_memory_size(void) · line 236

Quoted 10 lines

hw/mem/memory-device.c +10 -0

+
+    /* we will need a new memory slot for kvm and vhost */
+    if (kvm_enabled() && !kvm_has_free_slot(ms)) {
+        error_setg(errp, "hypervisor has no free memory slots left");
+        return;
+    }
+    if (!vhost_has_free_slot()) {
+        error_setg(errp, "a used vhost backend has no free memory slots left");
+        return;
+    }

thanks for extracting preparations steps into the right callback.