[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/1] pcie: Add hotplug detect state register to w1cmask
From: |
Peter Xu |
Subject: |
Re: [PATCH 1/1] pcie: Add hotplug detect state register to w1cmask |
Date: |
Thu, 29 Jun 2023 16:01:41 -0400 |
On Thu, Jun 29, 2023 at 03:33:06PM -0400, Michael S. Tsirkin wrote:
> On Thu, Jun 29, 2023 at 01:01:53PM -0400, Peter Xu wrote:
> > Hi, Leo,
> >
> > Thanks for figuring this out. Let me copy a few more potential reviewers
> > from commit 17858a1695 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on
> > Q35").
> >
> > On Thu, Jun 29, 2023 at 06:05:00AM -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 the bus 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.
>
> Presence Detect State – This bit indicates the presence of an
> adapter in the slot, reflected by the logical “OR” of the Physical
> Layer in-band presence detect mechanism and, if present, any
> out-of-band presence detect mechanism defined for the slot’s
> corresponding form factor. Note that the in-band presence
> detect mechanism requires that power be applied to an adapter
> for its presence to be detected. Consequently, form factors that
> require a power controller for hot-plug must implement a
> physical pin presence detect mechanism.
> RO
> Defined encodings are:
> 0b Slot Empty
> 1b Card Present in slot
> This bit must be implemented on all Downstream Ports that
> implement slots. For Downstream Ports not connected to slots
> (where the Slot Implemented bit of the PCI Express Capabilities
> register is 0b), this bit must be hardwired to 1b.
>
>
> And this seems to match what QEMU is doing: it clears on unplug
> not after guest deals with hotplug.
>
>
> > > 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.
>
> So bit is set on source.
> But why is the bit cleared on destination? This is the part I don't get.
My understanding is that when ACPI_PM_PROP_ACPI_PCIHP_BRIDGE is off for the
device, we just won't ever PCI_EXP_SLTSTA_PDS bit?
>
>
> > > To avoid this fake incompatibility, there are two fields in PCIDevice that
> > > can help:
> > >
> > > .wmask: Used to implement R/W bytes, and
> > > .w1cmask: Used to implement RW1C(Write 1 to Clear) bytes
> >
> > Is there one more option to clear the bit in cmask?
> >
> > IIUC w1cmask means the guest can now write to this bit, but afaiu from the
> > pcie spec it's RO.
>
> Yes this bit must be RO.
>
> > >
> > > According to pcie_cap_slot_init() the slot status register
> > > (PCI_EXP_SLTSTA), in which PCI_EXP_SLTSTA_PDS is a flag, seems to fall
> > > under w1cmask field, with makes sense due to the way signaling the hotplug
> > > works.
> > >
> > > So, add PCI_EXP_SLTSTA_PDS bit to w1cmask, so the fake incompatibility on
> > > get_pci_config_device() does not abort the migration.
> > >
> > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2215819
> > > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> >
> > Do we need a Fixes: and also the need to copy stable?
> >
> > > ---
> > > hw/pci/pcie.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > index b8c24cf45f..2def1765a5 100644
> > > --- a/hw/pci/pcie.c
> > > +++ b/hw/pci/pcie.c
> > > @@ -657,7 +657,7 @@ void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s)
> > > PCI_EXP_SLTCTL_EIC);
> > >
> > > pci_word_test_and_set_mask(dev->w1cmask + pos + PCI_EXP_SLTSTA,
> > > - PCI_EXP_HP_EV_SUPPORTED);
> > > + PCI_EXP_HP_EV_SUPPORTED |
> > > PCI_EXP_SLTSTA_PDS);
> > >
> > > dev->exp.hpev_notified = false;
> > >
> > > --
> > > 2.41.0
> > >
> >
> > --
> > Peter Xu
>
--
Peter Xu