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.juszkiew...@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;
}
