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

[Michael S. Tsirkin]

On Tue, Aug 29, 2023 at 04:04:27PM +0200, Philippe Mathieu-Daudé wrote:
> On 29/8/23 15:48, Michael S. Tsirkin wrote:
> > On Tue, Aug 29, 2023 at 02:14:51PM +0100, Peter Maydell wrote:
> > > On Tue, 29 Aug 2023 at 12:40, Marcin Juszkiewicz
> > > <marcin.juszkiewicz@linaro.org> wrote:
> > > > 
> > > > I am working on aarch64/sbsa-ref machine so people can have virtual
> > > > machine to test their OS against something reminding standards compliant
> > > > system.
> > > > 
> > > > One of tools I use is BSA ACS (Base System Architecture - Architecture
> > > > Compliance Suite) [1] written by Arm. It runs set of tests to check does
> > > > system conforms to BSA specification.
> > > > 
> > > > 1. https://github.com/ARM-software/bsa-acs
> > > > 
> > > > 
> > > > SBSA-ref goes better and better, yet still we have some issues. One of
> > > > them is test 822 ("Check Type 1 config header rules") which fails on
> > > > each PCIe root port device:
> > > > 
> > > > BDF 0x400 : SLT attribute mismatch: 0xFF020100 instead of 0x20100
> > > > BDF 0x500 : SLT attribute mismatch: 0xFF030300 instead of 0x30300
> > > > BDF 0x600 : SLT attribute mismatch: 0xFF040400 instead of 0x40400
> > > > 
> > > > I reported it as an issue [2] and got response that it may be QEMU
> > > > fault. My pcie knowledge is not good enough to know where the problem 
> > > > is.
> > > > 
> > > > 2. https://github.com/ARM-software/bsa-acs/issues/193
> > > > 
> > > > 
> > > > In the comment Gowtham Siddarth wrote:
> > > > 
> > > > > Regarding the SLT (Secondary Latency Timer) register, the expected
> > > > > values align with the ACS specifications, registering as 0. However,
> > > > > a discrepancy arises in the register's attribute, intended to be set
> > > > > as Read-Only. Contrary to this intent, the bit field seems to
> > > > > function as> Read-Write. Ordinarily, when attempting to write to the
> > > > > register by configuring all bits to 1, the anticipated behaviour
> > > > > should involve rejecting the write operation, maintaining the value
> > > > > at 0 to uphold the register's designated Read-Only nature. However,
> > > > > in this scenario, the write action takes effect, leading to a
> > > > > transformation of the register's value to FFs. This anomaly could
> > > > > potentially stem from an issue within the emulator.
> > > > 
> > > > Does someone know where the problem may be? And how to fix it?
> > > 
> > > I don't know enough about PCI to be sure here, but it sounds like
> > > what you're saying is happening is that the test case writes all-1s
> > > to some PCI register for the root port device, and expects that where
> > > some bits in it are defined in the spec as read-only they don't get set?
> > > 
> > > Which registers exactly is the test case trying to write in this way ?
> > > 
> > > I've cc'd the QEMU PCI maintainers.
> > > 
> > > thanks
> > > -- PMM
> > 
> > 
> > yes, this is a bug.
> > 
> > 
> > static void pci_init_mask_bridge(PCIDevice *d)
> > {
> >      /* PCI_PRIMARY_BUS, PCI_SECONDARY_BUS, PCI_SUBORDINATE_BUS and
> >         PCI_SEC_LETENCY_TIMER */
> >      memset(d->wmask + PCI_PRIMARY_BUS, 0xff, 4);
> > 
> > 
> > note there's a typo: PCI_SEC_LETENCY_TIMER should be
> > PCI_SEC_LATENCY_TIMER.
> > 
> > But the express spec says:
> >     This register does not apply to PCI Express. It must be read-only and 
> > hardwired to 00h. For PCI Express to PCI/PCI-X
> >     Bridges, refer to the [PCIe-to-PCI-PCI-X-Bridge] for requirements for 
> > this register.
> > 
> > 
> > So, only for the pci express to pci express bridges, and only for new
> > machine types, we need to override the mask to 0:
> > 
> >     d->wmask[PCI_SEC_LATENCY_TIMER] = 0;
> 
> Ah right. So smth like:
> 
> -- >8 --
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 881d774fb6..c73de617e0 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1241,2 +1241,5 @@ static PCIDevice *do_pci_register_device(PCIDevice
> *pci_dev,
>          pci_init_mask_bridge(pci_dev);
> +        if (pci_is_express(pci_dev)) {
> +            pci_set_byte(d->wmask + PCI_SEC_LATENCY_TIMER, 0);
> +        }
>      }
> ---
> 
> ?



No - it depends on secondart bus type and only applies to bridges.
Also we need compat machinery.
Marcin could you pls test the following?


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/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;
 }