Re: PCIe: SLT attribute mismatch: 0xFF020100 instead of 0x20100

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Aug 29, 2023 at 10:31:25PM +0200, Marcin Juszkiewicz wrote:
> W dniu 29.08.2023 o 22:18, Michael S. Tsirkin pisze:
> > On Tue, Aug 29, 2023 at 10:05:47PM +0200, Marcin Juszkiewicz wrote:
> > > W dniu 29.08.2023 o 19:07, Michael S. Tsirkin pisze:
> > > 
> > > > No - it depends on secondart bus type and only applies to bridges.
> > > > Also we need compat machinery.
> > > > Marcin could you pls test the following?
> > > 
> > > Works fine:
> > > 
> > >   822 : Check Type 1 config header rules           : Result:  PASS
> > 
> > Thanks! Now if possible I'd like to ask you to run the following test
> > with both default machine and 8.1 machine types.
> > With default should pass with 8.1 should fail as before.
> 
> It passes with sbsa-ref (which is not using QEMU versioning).
> 
> Fails (as expected) when used new property for each pcie-root-port
> (ignore line breaks):
> 
> "-device pcie-root-port,
>   x-pci-express-writeable-slt-bug=true,
>   id=root30,chassis=30,slot=0"


could you also check with -machine pc-q35-8.1 instead of this
property?



> > Thanks!
> 
> Thanks for sorting it out. I may have some more PCIe related
> questions in future.
> > diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
> > index ea54a81a15..5cd452115a 100644
> > --- a/include/hw/pci/pci_bridge.h
> > +++ b/include/hw/pci/pci_bridge.h
> > @@ -77,6 +77,9 @@ struct PCIBridge {
> >       pci_map_irq_fn map_irq;
> >       const char *bus_name;
> > +
> > +    /* SLT is RO for PCIE to PCIE bridges, but old QEMU versions had it RW */
> > +    bool pcie_writeable_slt_bug;
> >   };
> >   #define PCI_BRIDGE_DEV_PROP_CHASSIS_NR "chassis_nr"
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index da699cf4e1..d665c79de3 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -39,7 +39,9 @@
> >   #include "hw/virtio/virtio.h"
> >   #include "hw/virtio/virtio-pci.h"
> > -GlobalProperty hw_compat_8_1[] = {};
> > +GlobalProperty hw_compat_8_1[] = {
> > +    { TYPE_PCI_BRIDGE, "x-pci-express-writeable-slt-bug", "true" },
> 
> ../hw/core/machine.c:43:7: error: ‘TYPE_PCI_BRIDGE’ undeclared here (not in a function); did you mean ‘TYPE_PCI_BUS’?
>    43 |     { TYPE_PCI_BRIDGE, "x-pci-express-writeable-slt-bug", "true" },
>       |       ^~~~~~~~~~~~~~~
>       |       TYPE_PCI_BUS
> 
> Works with TYPE_PCI_BUS.
> 
> > +};
> >   const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
> >   GlobalProperty hw_compat_8_0[] = {
> > diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> > index e7b9345615..6a4e38856d 100644
> > --- a/hw/pci/pci_bridge.c
> > +++ b/hw/pci/pci_bridge.c
> > @@ -38,6 +38,7 @@
> >   #include "qapi/error.h"
> >   #include "hw/acpi/acpi_aml_interface.h"
> >   #include "hw/acpi/pci.h"
> > +#include "hw/qdev-properties.h"
> >   /* PCI bridge subsystem vendor ID helper functions */
> >   #define PCI_SSVID_SIZEOF        8
> > @@ -385,6 +386,11 @@ void pci_bridge_initfn(PCIDevice *dev, const char *typename)
> >       pci_bridge_region_init(br);
> >       QLIST_INIT(&sec_bus->child);
> >       QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
> > +
> > +    /* For express secondary buses, secondary latency timer is RO 0 */
> > +    if (pci_bus_is_express(sec_bus) && !br->pcie_writeable_slt_bug) {
> > +        dev->wmask[PCI_SEC_LATENCY_TIMER] = 0;
> > +    }
> >   }
> >   /* default qdev clean up function for PCI-to-PCI bridge */
> > @@ -466,10 +472,18 @@ int pci_bridge_qemu_reserve_cap_init(PCIDevice *dev, int cap_offset,
> >       return 0;
> >   }
> > +static Property pci_bridge_properties[] = {
> > +    DEFINE_PROP_BOOL("x-pci-express-writeable-slt-bug", PCIBridge,
> > +                     pcie_writeable_slt_bug, false),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> >   static void pci_bridge_class_init(ObjectClass *klass, void *data)
> >   {
> >       AcpiDevAmlIfClass *adevc = ACPI_DEV_AML_IF_CLASS(klass);
> > +    DeviceClass *k = DEVICE_CLASS(klass);
> > +    device_class_set_props(k, pci_bridge_properties);
> >       adevc->build_dev_aml = build_pci_bridge_aml;
> >   }
> >