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

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

[Marcin Juszkiewicz]

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?

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

[Peter Maydell]

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

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

[Michael S. Tsirkin]

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;


-- 
MST

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

[Philippe Mathieu-Daudé]

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);
+        }
      }
---

?

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

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

[Marcin Juszkiewicz]

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

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

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

[Michael S. Tsirkin]

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.

Thanks!

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" },
+};
 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;
 }
 

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

[Marcin Juszkiewicz]

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"

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

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

[Michael S. Tsirkin]

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

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

[Marcin Juszkiewicz]

W dniu 29.08.2023 o 22:40, Michael S. Tsirkin pisze:
>> 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?

BSA ACS is AArch64 only.

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

[Michael S. Tsirkin]

On Tue, Aug 29, 2023 at 10:44:08PM +0200, Marcin Juszkiewicz wrote:
> W dniu 29.08.2023 o 22:40, Michael S. Tsirkin pisze:
> > > 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?
> 
> BSA ACS is AArch64 only.

virt-8.1 then

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

[Marcin Juszkiewicz]

W dniu 29.08.2023 o 22:46, Michael S. Tsirkin pisze:
> On Tue, Aug 29, 2023 at 10:44:08PM +0200, Marcin Juszkiewicz wrote:
>> W dniu 29.08.2023 o 22:40, Michael S. Tsirkin pisze:
>>>> 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?
>>
>> BSA ACS is AArch64 only.
> 
> virt-8.1 then

Passes for virt, virt-8.1 and virt-8.0 machines.

./code/qemu/build/aarch64-softmmu/qemu-system-aarch64 \
  -machine virt-8.0 \
  -m 4096  \
  -cpu neoverse-n1 \
  -smp 2 \
  -drive if=pflash,format=raw,file=QEMU_EFI.fd \
  -drive if=pflash,format=raw,file=QEMU_EFI-vars.fd \
-drive file=fat:rw:$PWD/disks/virtual/,format=raw \
-device pcie-root-port,id=root30,chassis=30,slot=0 \
-device igb,bus=root30 \
  -device qemu-xhci,id=usb \
  -device bochs-display \
  -device e1000e \
-nographic \
  -usb \
  -device usb-kbd \
  -device usb-tablet \
  -monitor telnet::45454,server,nowait \
  -serial stdio

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

[Michael S. Tsirkin]

On Wed, Aug 30, 2023 at 10:22:53AM +0200, Marcin Juszkiewicz wrote:
> W dniu 29.08.2023 o 22:46, Michael S. Tsirkin pisze:
> > On Tue, Aug 29, 2023 at 10:44:08PM +0200, Marcin Juszkiewicz wrote:
> > > W dniu 29.08.2023 o 22:40, Michael S. Tsirkin pisze:
> > > > > 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?
> > > 
> > > BSA ACS is AArch64 only.
> > 
> > virt-8.1 then
> 
> Passes for virt, virt-8.1 and virt-8.0 machines.
> 
> ./code/qemu/build/aarch64-softmmu/qemu-system-aarch64 \
>  -machine virt-8.0 \
>  -m 4096  \
>  -cpu neoverse-n1 \
>  -smp 2 \
>  -drive if=pflash,format=raw,file=QEMU_EFI.fd \
>  -drive if=pflash,format=raw,file=QEMU_EFI-vars.fd \
> -drive file=fat:rw:$PWD/disks/virtual/,format=raw \
> -device pcie-root-port,id=root30,chassis=30,slot=0 \
> -device igb,bus=root30 \
>  -device qemu-xhci,id=usb \
>  -device bochs-display \
>  -device e1000e \
> -nographic \
>  -usb \
>  -device usb-kbd \
>  -device usb-tablet \
>  -monitor telnet::45454,server,nowait \
>  -serial stdio
> 


Weird. OK let me post it properly and you can then test and confirm
it fails on 8.1 and 8.0 and passes on latest.

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

[Philippe Mathieu-Daudé]

On 29/8/23 13:39, Marcin Juszkiewicz 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?

Commit fb23162885 ("pci: initialize pci config headers depending it pci
header type.") sets PCI_SEC_LATENCY_TIMER writable; it seems to be a
mistake (and bsa-acs is doing the correct testing).

Can you try this quick fix?

-- >8 --
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 881d774fb6..e43aa0fb36 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -894,5 +895,4 @@ 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);
+    /* PCI_PRIMARY_BUS, PCI_SECONDARY_BUS, PCI_SUBORDINATE_BUS */
+    memset(d->wmask + PCI_PRIMARY_BUS, 0xff, 3);

---

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

[Marcin Juszkiewicz]

W dniu 29.08.2023 o 15:57, Philippe Mathieu-Daudé pisze:
> On 29/8/23 13:39, Marcin Juszkiewicz wrote:

>> Does someone know where the problem may be? And how to fix it?
> 
> Commit fb23162885 ("pci: initialize pci config headers depending it pci
> header type.") sets PCI_SEC_LATENCY_TIMER writable; it seems to be a
> mistake (and bsa-acs is doing the correct testing).
> 
> Can you try this quick fix?
> 
> -- >8 --
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 881d774fb6..e43aa0fb36 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -894,5 +895,4 @@ 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);
> +    /* PCI_PRIMARY_BUS, PCI_SECONDARY_BUS, PCI_SUBORDINATE_BUS */
> +    memset(d->wmask + PCI_PRIMARY_BUS, 0xff, 3);

It made test pass:

  822 : Check Type 1 config header rules           : Result:  PASS