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

[Peter Xu]

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.
> 
> 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 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.

> 
> 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