On Mon, 16 Jun 2025 11:46:55 +0200
Eric Auger <eric.au...@redhat.com> wrote:
Maybe needs a little bit of description.
> Signed-off-by: Eric Auger <eric.au...@redhat.com>
A few trivial things inline.
Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>
>
> ---
> v2 -> v3:
> - fix cohabitation with virtio-mem-pci device and tested
> hotplug/unplug of this latter (Igor)
> ---
> hw/arm/virt.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 69 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 8c882e0794..06b87e1050 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1747,6 +1747,22 @@ static void virt_build_smbios(VirtMachineState *vms)
> }
> }
>
> +static AcpiPciHpState *get_acpi_pcihp_state(VirtMachineState *vms)
> +{
> + AcpiGedState *acpi_ged_state;
> + AcpiPciHpState *pcihp_state;
> +
> + if (!vms->acpi_dev) {
> + return NULL;
> + }
> + acpi_ged_state = ACPI_GED(vms->acpi_dev);
> + pcihp_state = &acpi_ged_state->pcihp_state;
> + if (pcihp_state->use_acpi_hotplug_bridge) {
> + return pcihp_state;
> + }
> + return NULL;
Really trivial but if this isn't going to gain extra stuff I'd consider
the NULL return the 'error' path and prefer that out of line for consistency.
That is.
if(!pcihp_state->use_acpi_hotplug_bridge) {
return NULL;
}
return pcihp_state;
That just ends up a tiny bit more consistent with the !vms->acpi_dev test
earlier in the function.
> +}
> +
> static
> void virt_machine_done(Notifier *notifier, void *data)
> {
> @@ -2907,6 +2923,13 @@ static void
> virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> {
> VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>
Maybe add a comment on why this is not in the if/else stack. I assume
because a few of those are also TYPE_PCI_DEVICE.
Something in the patch description would also work for me.
> + if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> + if (get_acpi_pcihp_state(vms)) {
> + acpi_pcihp_device_pre_plug_cb(HOTPLUG_HANDLER(vms->acpi_dev),
> + dev, errp);
> + }
> + }
> +
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> virt_memory_pre_plug(hotplug_dev, dev, errp);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
> @@ -2961,6 +2984,15 @@ static void virt_machine_device_plug_cb(HotplugHandler
> *hotplug_dev,
> }
> }
>
> + if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> + AcpiPciHpState *pcihp_state = get_acpi_pcihp_state(vms);
> +
> + if (pcihp_state) {
> + acpi_pcihp_device_plug_cb(HOTPLUG_HANDLER(vms->acpi_dev),
> + pcihp_state, dev, errp);
> + }
> + }
> +
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> virt_memory_plug(hotplug_dev, dev, errp);
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
> @@ -3017,12 +3049,27 @@ out:
> static void virt_machine_device_unplug_request_cb(HotplugHandler
> *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> + bool supported = false;
> +
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> virt_dimm_unplug_request(hotplug_dev, dev, errp);
> + supported = true;
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
> virtio_md_pci_unplug_request(VIRTIO_MD_PCI(dev),
> MACHINE(hotplug_dev),
> errp);
> - } else {
> + supported = true;
> + }
> + if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> + AcpiPciHpState *pcihp_state = get_acpi_pcihp_state(vms);
> +
> + if (pcihp_state) {
> +
> acpi_pcihp_device_unplug_request_cb(HOTPLUG_HANDLER(vms->acpi_dev),
> + pcihp_state, dev, errp);
> + supported = true;
> + }
> + }
> + if (!supported) {
> error_setg(errp, "device unplug request for unsupported device"
> " type: %s", object_get_typename(OBJECT(dev)));
> }
> @@ -3031,11 +3078,27 @@ static void
> virt_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> static void virt_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> + bool supported = false;
> +
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> virt_dimm_unplug(hotplug_dev, dev, errp);
> + supported = true;
> } else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MD_PCI)) {
> virtio_md_pci_unplug(VIRTIO_MD_PCI(dev), MACHINE(hotplug_dev), errp);
> - } else {
> + supported = true;
> + }
> + if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> +
> + AcpiPciHpState *pcihp_state = get_acpi_pcihp_state(vms);
> +
> + if (pcihp_state) {
> + acpi_pcihp_device_unplug_cb(HOTPLUG_HANDLER(vms->acpi_dev),
> + pcihp_state, dev, errp);
> + supported = true;
> + }
> + }
> + if (!supported) {
> error_setg(errp, "virt: device unplug for unsupported device"
> " type: %s", object_get_typename(OBJECT(dev)));
> }
