Re: [PATCH] hw/pci/pcie: Move hot plug capability check to pre_plug callback
On Thu, Jun 04, 2020 at 12:57:55PM +0200, Igor Mammedov wrote:
> On Mon, 1 Jun 2020 18:29:34 +0200
> Julia Suvorova wrote:
>
> > Check for hot plug capability earlier to avoid removing devices attached
> > during the initialization process.
> >
> > Run qemu with an unattached drive:
> > -drive file=$FILE,if=none,id=drive0 \
> > -device pcie-root-port,id=rp0,slot=3,bus=pcie.0,hotplug=off
> > Hotplug a block device:
> > device_add virtio-blk-pci,id=blk0,drive=drive0,bus=rp0
> > If hotplug fails on plug_cb, drive0 will be deleted.
> >
> > Signed-off-by: Julia Suvorova
> > ---
> > Hard to say if it's a bug or generally acceptable behaviour, but seems like
> > hotplug_handler_plug should never fail.
>
> _unplug shouldn't fail the rest are allowed to, but it's hard to unwind
> intialization cleanly to _plug stage so if it's possible to do checks
> at _pre_plug time (i.e. before device's realize() is called) we should do so.
>
> Acked-by: Igor Mammedov
Makes sense. Julia could you repost with a tweaked commit log
and I'll apply. Thanks!
> >
> > hw/pci/pcie.c | 19 +++
> > 1 file changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index f50e10b8fb..5b9c022d91 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -407,6 +407,17 @@ static void pcie_cap_slot_plug_common(PCIDevice
> > *hotplug_dev, DeviceState *dev,
> > void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState
> > *dev,
> > Error **errp)
> > {
> > +PCIDevice *hotplug_pdev = PCI_DEVICE(hotplug_dev);
> > +uint8_t *exp_cap = hotplug_pdev->config + hotplug_pdev->exp.exp_cap;
> > +uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> > +
> > +/* Check if hot-plug is disabled on the slot */
> > +if (dev->hotplugged && (sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > +error_setg(errp, "Hot-plug failed: unsupported by the port device
> > '%s'",
> > + DEVICE(hotplug_pdev)->id);
> > +return;
> > +}
> > +
> > pcie_cap_slot_plug_common(PCI_DEVICE(hotplug_dev), dev, errp);
> > }
> >
> > @@ -415,7 +426,6 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev,
> > DeviceState *dev,
> > {
> > PCIDevice *hotplug_pdev = PCI_DEVICE(hotplug_dev);
> > uint8_t *exp_cap = hotplug_pdev->config + hotplug_pdev->exp.exp_cap;
> > -uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> > PCIDevice *pci_dev = PCI_DEVICE(dev);
> >
> > /* Don't send event when device is enabled during qemu machine
> > creation:
> > @@ -431,13 +441,6 @@ void pcie_cap_slot_plug_cb(HotplugHandler
> > *hotplug_dev, DeviceState *dev,
> > return;
> > }
> >
> > -/* Check if hot-plug is disabled on the slot */
> > -if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > -error_setg(errp, "Hot-plug failed: unsupported by the port device
> > '%s'",
> > - DEVICE(hotplug_pdev)->id);
> > -return;
> > -}
> > -
> > /* To enable multifunction hot-plug, we just ensure the function
> > * 0 added last. When function 0 is added, we set the sltsta and
> > * inform OS via event notification.
Re: [PATCH] hw/pci/pcie: Move hot plug capability check to pre_plug callback
On Mon, 1 Jun 2020 18:29:34 +0200
Julia Suvorova wrote:
> Check for hot plug capability earlier to avoid removing devices attached
> during the initialization process.
>
> Run qemu with an unattached drive:
> -drive file=$FILE,if=none,id=drive0 \
> -device pcie-root-port,id=rp0,slot=3,bus=pcie.0,hotplug=off
> Hotplug a block device:
> device_add virtio-blk-pci,id=blk0,drive=drive0,bus=rp0
> If hotplug fails on plug_cb, drive0 will be deleted.
>
> Signed-off-by: Julia Suvorova
> ---
> Hard to say if it's a bug or generally acceptable behaviour, but seems like
> hotplug_handler_plug should never fail.
_unplug shouldn't fail the rest are allowed to, but it's hard to unwind
intialization cleanly to _plug stage so if it's possible to do checks
at _pre_plug time (i.e. before device's realize() is called) we should do so.
Acked-by: Igor Mammedov
>
> hw/pci/pcie.c | 19 +++
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index f50e10b8fb..5b9c022d91 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -407,6 +407,17 @@ static void pcie_cap_slot_plug_common(PCIDevice
> *hotplug_dev, DeviceState *dev,
> void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> Error **errp)
> {
> +PCIDevice *hotplug_pdev = PCI_DEVICE(hotplug_dev);
> +uint8_t *exp_cap = hotplug_pdev->config + hotplug_pdev->exp.exp_cap;
> +uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> +
> +/* Check if hot-plug is disabled on the slot */
> +if (dev->hotplugged && (sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> +error_setg(errp, "Hot-plug failed: unsupported by the port device
> '%s'",
> + DEVICE(hotplug_pdev)->id);
> +return;
> +}
> +
> pcie_cap_slot_plug_common(PCI_DEVICE(hotplug_dev), dev, errp);
> }
>
> @@ -415,7 +426,6 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev,
> DeviceState *dev,
> {
> PCIDevice *hotplug_pdev = PCI_DEVICE(hotplug_dev);
> uint8_t *exp_cap = hotplug_pdev->config + hotplug_pdev->exp.exp_cap;
> -uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> PCIDevice *pci_dev = PCI_DEVICE(dev);
>
> /* Don't send event when device is enabled during qemu machine creation:
> @@ -431,13 +441,6 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev,
> DeviceState *dev,
> return;
> }
>
> -/* Check if hot-plug is disabled on the slot */
> -if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> -error_setg(errp, "Hot-plug failed: unsupported by the port device
> '%s'",
> - DEVICE(hotplug_pdev)->id);
> -return;
> -}
> -
> /* To enable multifunction hot-plug, we just ensure the function
> * 0 added last. When function 0 is added, we set the sltsta and
> * inform OS via event notification.
Re: [PATCH] hw/pci/pcie: Move hot plug capability check to pre_plug callback
On Tue, Jun 2, 2020 at 5:54 AM Michael S. Tsirkin wrote:
>
> On Mon, Jun 01, 2020 at 06:29:34PM +0200, Julia Suvorova wrote:
> > Check for hot plug capability earlier to avoid removing devices attached
> > during the initialization process.
> >
> > Run qemu with an unattached drive:
> > -drive file=$FILE,if=none,id=drive0 \
> > -device pcie-root-port,id=rp0,slot=3,bus=pcie.0,hotplug=off
> > Hotplug a block device:
> > device_add virtio-blk-pci,id=blk0,drive=drive0,bus=rp0
> > If hotplug fails on plug_cb, drive0 will be deleted.
> >
> > Signed-off-by: Julia Suvorova
>
>
> Fixes: 0501e1aa1d32a6 ("hw/pci/pcie: Forbid hot-plug if it's disabled on the
> slot")
>
> correct?
Yes.
> > ---
> > Hard to say if it's a bug or generally acceptable behaviour, but seems like
> > hotplug_handler_plug should never fail.
> >
> > hw/pci/pcie.c | 19 +++
> > 1 file changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index f50e10b8fb..5b9c022d91 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -407,6 +407,17 @@ static void pcie_cap_slot_plug_common(PCIDevice
> > *hotplug_dev, DeviceState *dev,
> > void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState
> > *dev,
> > Error **errp)
> > {
> > +PCIDevice *hotplug_pdev = PCI_DEVICE(hotplug_dev);
> > +uint8_t *exp_cap = hotplug_pdev->config + hotplug_pdev->exp.exp_cap;
> > +uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> > +
> > +/* Check if hot-plug is disabled on the slot */
> > +if (dev->hotplugged && (sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > +error_setg(errp, "Hot-plug failed: unsupported by the port device
> > '%s'",
> > + DEVICE(hotplug_pdev)->id);
> > +return;
> > +}
> > +
> > pcie_cap_slot_plug_common(PCI_DEVICE(hotplug_dev), dev, errp);
> > }
> >
> > @@ -415,7 +426,6 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev,
> > DeviceState *dev,
> > {
> > PCIDevice *hotplug_pdev = PCI_DEVICE(hotplug_dev);
> > uint8_t *exp_cap = hotplug_pdev->config + hotplug_pdev->exp.exp_cap;
> > -uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> > PCIDevice *pci_dev = PCI_DEVICE(dev);
> >
> > /* Don't send event when device is enabled during qemu machine
> > creation:
> > @@ -431,13 +441,6 @@ void pcie_cap_slot_plug_cb(HotplugHandler
> > *hotplug_dev, DeviceState *dev,
> > return;
> > }
> >
> > -/* Check if hot-plug is disabled on the slot */
> > -if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> > -error_setg(errp, "Hot-plug failed: unsupported by the port device
> > '%s'",
> > - DEVICE(hotplug_pdev)->id);
> > -return;
> > -}
> > -
> > /* To enable multifunction hot-plug, we just ensure the function
> > * 0 added last. When function 0 is added, we set the sltsta and
> > * inform OS via event notification.
> > --
> > 2.25.4
>
Re: [PATCH] hw/pci/pcie: Move hot plug capability check to pre_plug callback
On Mon, Jun 01, 2020 at 06:29:34PM +0200, Julia Suvorova wrote:
> Check for hot plug capability earlier to avoid removing devices attached
> during the initialization process.
>
> Run qemu with an unattached drive:
> -drive file=$FILE,if=none,id=drive0 \
> -device pcie-root-port,id=rp0,slot=3,bus=pcie.0,hotplug=off
> Hotplug a block device:
> device_add virtio-blk-pci,id=blk0,drive=drive0,bus=rp0
> If hotplug fails on plug_cb, drive0 will be deleted.
>
> Signed-off-by: Julia Suvorova
Fixes: 0501e1aa1d32a6 ("hw/pci/pcie: Forbid hot-plug if it's disabled on the
slot")
correct?
> ---
> Hard to say if it's a bug or generally acceptable behaviour, but seems like
> hotplug_handler_plug should never fail.
>
> hw/pci/pcie.c | 19 +++
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index f50e10b8fb..5b9c022d91 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -407,6 +407,17 @@ static void pcie_cap_slot_plug_common(PCIDevice
> *hotplug_dev, DeviceState *dev,
> void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> Error **errp)
> {
> +PCIDevice *hotplug_pdev = PCI_DEVICE(hotplug_dev);
> +uint8_t *exp_cap = hotplug_pdev->config + hotplug_pdev->exp.exp_cap;
> +uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> +
> +/* Check if hot-plug is disabled on the slot */
> +if (dev->hotplugged && (sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> +error_setg(errp, "Hot-plug failed: unsupported by the port device
> '%s'",
> + DEVICE(hotplug_pdev)->id);
> +return;
> +}
> +
> pcie_cap_slot_plug_common(PCI_DEVICE(hotplug_dev), dev, errp);
> }
>
> @@ -415,7 +426,6 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev,
> DeviceState *dev,
> {
> PCIDevice *hotplug_pdev = PCI_DEVICE(hotplug_dev);
> uint8_t *exp_cap = hotplug_pdev->config + hotplug_pdev->exp.exp_cap;
> -uint32_t sltcap = pci_get_word(exp_cap + PCI_EXP_SLTCAP);
> PCIDevice *pci_dev = PCI_DEVICE(dev);
>
> /* Don't send event when device is enabled during qemu machine creation:
> @@ -431,13 +441,6 @@ void pcie_cap_slot_plug_cb(HotplugHandler *hotplug_dev,
> DeviceState *dev,
> return;
> }
>
> -/* Check if hot-plug is disabled on the slot */
> -if ((sltcap & PCI_EXP_SLTCAP_HPC) == 0) {
> -error_setg(errp, "Hot-plug failed: unsupported by the port device
> '%s'",
> - DEVICE(hotplug_pdev)->id);
> -return;
> -}
> -
> /* To enable multifunction hot-plug, we just ensure the function
> * 0 added last. When function 0 is added, we set the sltsta and
> * inform OS via event notification.
> --
> 2.25.4
