Re: [PATCH v2 1/1] pcie: Add hotplug detect state register to cmask

[Leonardo Bras Soares Passos <leobras@redhat.com>]

On Thu, Jul 6, 2023 at 11:35 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Thu, Jul 06, 2023 at 01:55:47AM -0300, Leonardo Bras wrote:
> > When trying to migrate a machine type pc-q35-6.0 or lower, with this
> > cmdline options,
> >
> > -device driver=pcie-root-port,port=18,chassis=19,id=pcie-root-port18,bus=pcie.0,addr=0x12 \
> > -device driver=nec-usb-xhci,p2=4,p3=4,id=nex-usb-xhci0,bus=pcie-root-port18,addr=0x12.0x1
> >
> > the following bug happens after all ram pages were sent:
> >
> > qemu-kvm: get_pci_config_device: Bad config data: i=0x6e read: 0 device: 40 cmask: ff wmask: 0 w1cmask:19
> > qemu-kvm: Failed to load PCIDevice:config
> > qemu-kvm: Failed to load pcie-root-port:parent_obj.parent_obj.parent_obj
> > qemu-kvm: error while loading state for instance 0x0 of device '0000:00:12.0/pcie-root-port'
> > qemu-kvm: load of migration failed: Invalid argument
> >
> > This happens on pc-q35-6.0 or lower because of:
> > { "ICH9-LPC", ACPI_PM_PROP_ACPI_PCIHP_BRIDGE, "off" }
> >
> > In this scenario, hotplug_handler_plug() calls pcie_cap_slot_plug_cb(),
> > which sets dev->config byte 0x6e with bit PCI_EXP_SLTSTA_PDS to signal PCI
> > hotplug for the guest. After a while the guest will deal with this hotplug
> > and qemu will clear the above bit.
>
> Do you mean that the bit will be cleared after this point for the whole
> lifecycle of the VM, as long as the pcie topology doesn't change again?
>
> "This bit indicates the presence of an adapter in the slot"
>
> IIUC the adapter in the slot is there, why it's cleared rather than set?

Fort some reason the guest is powering down the device, and we have in qemu:

 /*
     * If the slot is populated, power indicator is off and power
     * controller is off, it is safe to detach the devices.
     *
     * Note: don't detach if condition was already true:
     * this is a work around for guests that overwrite
     * control of powered off slots before powering them on.
     */
    if ((sltsta & PCI_EXP_SLTSTA_PDS) && pcie_sltctl_powered_off(val) &&
        !pcie_sltctl_powered_off(old_slt_ctl))
    {
        pcie_cap_slot_do_unplug(dev);  // clears PCI_EXP_SLTSTA_PDS
    }


>
> >
> > Then, during migration, get_pci_config_device() will compare the
> > configs of both the freshly created device and the one that is being
> > received via migration, which will differ due to the PCI_EXP_SLTSTA_PDS bit
> > and cause the bug to reproduce.
> >
> > To avoid this fake incompatibility, there are tree fields in PCIDevice that
> > can help:
> >
> > - wmask: Used to implement R/W bytes, and
> > - w1cmask: Used to implement RW1C(Write 1 to Clear) bytes
> > - cmask: Used to enable config checks on load.
> >
> > According to PCI Express® Base Specification Revision 5.0 Version 1.0,
> > table 7-27 (Slot Status Register) bit 6, the "Presence Detect State" is
> > listed as RO (read-only), so it only makes sense to make use of the cmask
> > field.
> >
> > So, clear PCI_EXP_SLTSTA_PDS bit on cmask, so the fake incompatibility on
> > get_pci_config_device() does not abort the migration.
>
> Yes, using cmask makes more sense to me, but we'd need some pci developer
> to ack it at last I guess, anyway.

Agree! I am waiting for Michael's opinion on this.

>
> >
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2215819
> > Signed-off-by: Leonardo Bras <leobras@redhat.com>
>
> I asked the same question, and I still keep confused: whether there's a
> first bad commit?  Starting from when it fails?
>
> For example, is this broken on 6.0 binaries too with pc-q35-6.0?

I tested for qemu 6.0, and it still reproduces, but have not pursued
this any further.

>
> Thanks,


Thank you!
Leo

>
> > ---
> >  hw/pci/pcie.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index b8c24cf45f..cae56bf1c8 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -659,6 +659,10 @@ void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s)
> >      pci_word_test_and_set_mask(dev->w1cmask + pos + PCI_EXP_SLTSTA,
> >                                 PCI_EXP_HP_EV_SUPPORTED);
> >
> > +    /* Avoid migration abortion when this device hot-removed by guest */
> > +    pci_word_test_and_clear_mask(dev->cmask + pos + PCI_EXP_SLTSTA,
> > +                                 PCI_EXP_SLTSTA_PDS);
> > +
> >      dev->exp.hpev_notified = false;
> >
> >      qbus_set_hotplug_handler(BUS(pci_bridge_get_sec_bus(PCI_BRIDGE(dev))),
> > --
> > 2.41.0
> >
>
> --
> Peter Xu
>