Re: [PATCH] pci: SLT must be RO
On Mon, Oct 02, 2023 at 01:39:16PM +0200, Marcin Juszkiewicz wrote: > W dniu 8.09.2023 o 15:29, Marcin Juszkiewicz pisze: > > W dniu 31.08.2023 o 12:05, Marcin Juszkiewicz pisze: > > > W dniu 30.08.2023 o 23:48, Michael S. Tsirkin pisze: > > > > current code sets PCI_SEC_LATENCY_TIMER to WO, but for > > > > pcie to pcie bridges it must be RO 0 according to > > > > pci express spec which 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. > > > > > > > > also, fix typo in comment where it's make writeable - this typo > > > > is likely what prevented us noticing we violate this requirement > > > > in the 1st place. > > > > > > > > Reported-by: Marcin Juszkiewicz > > > > Signed-off-by: Michael S. Tsirkin > > > > --- > > > > > > > Marcin, could you pls test this patch with virt-8.1 and latest? > > > > Thanks a lot! > > > > > > Tested-by: Marcin Juszkiewicz > > > > > > sbsa-ref: PASS > > > virt: PASS > > > virt-8.1: FAIL (as expected) > > > virt-8.0: FAIL (as expected) > > > > Can we get this patch refreshed and merged? > > ping? > yes, working on a pull request including this.
Re: [PATCH] pci: SLT must be RO
W dniu 8.09.2023 o 15:29, Marcin Juszkiewicz pisze: W dniu 31.08.2023 o 12:05, Marcin Juszkiewicz pisze: W dniu 30.08.2023 o 23:48, Michael S. Tsirkin pisze: current code sets PCI_SEC_LATENCY_TIMER to WO, but for pcie to pcie bridges it must be RO 0 according to pci express spec which 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. also, fix typo in comment where it's make writeable - this typo is likely what prevented us noticing we violate this requirement in the 1st place. Reported-by: Marcin Juszkiewicz Signed-off-by: Michael S. Tsirkin --- Marcin, could you pls test this patch with virt-8.1 and latest? Thanks a lot! Tested-by: Marcin Juszkiewicz sbsa-ref: PASS virt: PASS virt-8.1: FAIL (as expected) virt-8.0: FAIL (as expected) Can we get this patch refreshed and merged? ping?
Re: [PATCH] pci: SLT must be RO
W dniu 31.08.2023 o 12:05, Marcin Juszkiewicz pisze: W dniu 30.08.2023 o 23:48, Michael S. Tsirkin pisze: current code sets PCI_SEC_LATENCY_TIMER to WO, but for pcie to pcie bridges it must be RO 0 according to pci express spec which 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. also, fix typo in comment where it's make writeable - this typo is likely what prevented us noticing we violate this requirement in the 1st place. Reported-by: Marcin Juszkiewicz Signed-off-by: Michael S. Tsirkin --- Marcin, could you pls test this patch with virt-8.1 and latest? Thanks a lot! Tested-by: Marcin Juszkiewicz sbsa-ref: PASS virt: PASS virt-8.1: FAIL (as expected) virt-8.0: FAIL (as expected) Can we get this patch refreshed and merged?
Re: [PATCH] pci: SLT must be RO
W dniu 30.08.2023 o 23:48, Michael S. Tsirkin pisze:
current code sets PCI_SEC_LATENCY_TIMER to WO, but for
pcie to pcie bridges it must be RO 0 according to
pci express spec which 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.
also, fix typo in comment where it's make writeable - this typo
is likely what prevented us noticing we violate this requirement
in the 1st place.
Reported-by: Marcin Juszkiewicz
Signed-off-by: Michael S. Tsirkin
---
Marcin, could you pls test this patch with virt-8.1 and latest?
Thanks a lot!
Tested-by: Marcin Juszkiewicz
sbsa-ref: PASS
virt: PASS
virt-8.1: FAIL (as expected)
virt-8.0: FAIL (as expected)
./code/qemu/build/aarch64-softmmu/qemu-system-aarch64 \
-machine virt \
-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
include/hw/pci/pci_bridge.h | 3 +++
hw/core/machine.c | 5 -
hw/pci/pci.c| 2 +-
hw/pci/pci_bridge.c | 14 ++
4 files changed, 22 insertions(+), 2 deletions(-)
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..76743e3b31 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -32,6 +32,7 @@
#include "qemu/error-report.h"
#include "sysemu/qtest.h"
#include "hw/pci/pci.h"
+#include "hw/pci/pci_bridge.h"
#include "hw/mem/nvdimm.h"
#include "migration/global_state.h"
#include "migration/vmstate.h"
@@ -39,7 +40,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.c b/hw/pci/pci.c
index 881d774fb6..b0d21bf43a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -893,7 +893,7 @@ static void pci_init_w1cmask(PCIDevice *dev)
static void pci_init_mask_bridge(PCIDevice *d)
{
/* PCI_PRIMARY_BUS, PCI_SECONDARY_BUS, PCI_SUBORDINATE_BUS and
- PCI_SEC_LETENCY_TIMER */
+ PCI_SEC_LATENCY_TIMER */
memset(d->wmask + PCI_PRIMARY_BUS, 0xff, 4);
/* base and limit */
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_SIZEOF8
@@ -385,6 +386,11 @@ void pci_bridge_initfn(PCIDevice *dev, const char
*typename)
pci_bridge_region_init(br);
QLIST_INIT(_bus->child);
QLIST_INSERT_HEAD(>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: [PATCH] pci: SLT must be RO
On 31/8/23 08:45, Michael S. Tsirkin wrote:
On Thu, Aug 31, 2023 at 08:22:34AM +0200, Philippe Mathieu-Daudé wrote:
Hi Michael,
On 30/8/23 23:48, Michael S. Tsirkin wrote:
current code sets PCI_SEC_LATENCY_TIMER to WO, but for
pcie to pcie bridges it must be RO 0 according to
pci express spec which 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.
also, fix typo in comment where it's make writeable - this typo
is likely what prevented us noticing we violate this requirement
in the 1st place.
Reported-by: Marcin Juszkiewicz
Signed-off-by: Michael S. Tsirkin
---
Marcin, could you pls test this patch with virt-8.1 and latest?
Thanks a lot!
include/hw/pci/pci_bridge.h | 3 +++
hw/core/machine.c | 5 -
hw/pci/pci.c| 2 +-
hw/pci/pci_bridge.c | 14 ++
4 files changed, 22 insertions(+), 2 deletions(-)
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;
};
Patch LGTM, so:
Reviewed-by: Philippe Mathieu-Daudé
-GlobalProperty hw_compat_8_1[] = {};
+GlobalProperty hw_compat_8_1[] = {
+{ TYPE_PCI_BRIDGE, "x-pci-express-writeable-slt-bug", "true" },
However I don't understand why we can't clear the config register and
must use a compat flag, since per the spec it is hardwired to 0.
Because historically we didn't. If we make it RO and migrate from
VM where guest wrote into this register, migration will fail
as we check that RO fields are unchanged.
Another trick would be to pretend it's hardware driven
and set cmask. Compat machinery is more straight-forward though.
I see.
Do we need the "x-" compat prefix? This is not an experimental property.
It's an internal one, we don't want users to tweak it.
OK, thanks!
Re: [PATCH] pci: SLT must be RO
On Thu, Aug 31, 2023 at 08:22:34AM +0200, Philippe Mathieu-Daudé wrote:
> Hi Michael,
>
> On 30/8/23 23:48, Michael S. Tsirkin wrote:
> > current code sets PCI_SEC_LATENCY_TIMER to WO, but for
> > pcie to pcie bridges it must be RO 0 according to
> > pci express spec which 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.
> >
> > also, fix typo in comment where it's make writeable - this typo
> > is likely what prevented us noticing we violate this requirement
> > in the 1st place.
> >
> > Reported-by: Marcin Juszkiewicz
> > Signed-off-by: Michael S. Tsirkin
> > ---
> >
> > Marcin, could you pls test this patch with virt-8.1 and latest?
> > Thanks a lot!
> >
> >
> > include/hw/pci/pci_bridge.h | 3 +++
> > hw/core/machine.c | 5 -
> > hw/pci/pci.c| 2 +-
> > hw/pci/pci_bridge.c | 14 ++
> > 4 files changed, 22 insertions(+), 2 deletions(-)
> >
> > 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;
> > };
>
> Patch LGTM, so:
> Reviewed-by: Philippe Mathieu-Daudé
>
> > -GlobalProperty hw_compat_8_1[] = {};
> > +GlobalProperty hw_compat_8_1[] = {
> > +{ TYPE_PCI_BRIDGE, "x-pci-express-writeable-slt-bug", "true" },
>
> However I don't understand why we can't clear the config register and
> must use a compat flag, since per the spec it is hardwired to 0.
Because historically we didn't. If we make it RO and migrate from
VM where guest wrote into this register, migration will fail
as we check that RO fields are unchanged.
Another trick would be to pretend it's hardware driven
and set cmask. Compat machinery is more straight-forward though.
> Do we need the "x-" compat prefix? This is not an experimental property.
It's an internal one, we don't want users to tweak it.
> > +};
> > const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
>
>
> > 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_SIZEOF8
> > @@ -385,6 +386,11 @@ void pci_bridge_initfn(PCIDevice *dev, const char
> > *typename)
> > pci_bridge_region_init(br);
> > QLIST_INIT(_bus->child);
> > QLIST_INSERT_HEAD(>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;
> > +}
> > }
>
>
Re: [PATCH] pci: SLT must be RO
Hi Michael,
On 30/8/23 23:48, Michael S. Tsirkin wrote:
current code sets PCI_SEC_LATENCY_TIMER to WO, but for
pcie to pcie bridges it must be RO 0 according to
pci express spec which 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.
also, fix typo in comment where it's make writeable - this typo
is likely what prevented us noticing we violate this requirement
in the 1st place.
Reported-by: Marcin Juszkiewicz
Signed-off-by: Michael S. Tsirkin
---
Marcin, could you pls test this patch with virt-8.1 and latest?
Thanks a lot!
include/hw/pci/pci_bridge.h | 3 +++
hw/core/machine.c | 5 -
hw/pci/pci.c| 2 +-
hw/pci/pci_bridge.c | 14 ++
4 files changed, 22 insertions(+), 2 deletions(-)
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;
};
Patch LGTM, so:
Reviewed-by: Philippe Mathieu-Daudé
-GlobalProperty hw_compat_8_1[] = {};
+GlobalProperty hw_compat_8_1[] = {
+{ TYPE_PCI_BRIDGE, "x-pci-express-writeable-slt-bug", "true" },
However I don't understand why we can't clear the config register and
must use a compat flag, since per the spec it is hardwired to 0.
Do we need the "x-" compat prefix? This is not an experimental property.
+};
const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
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_SIZEOF8
@@ -385,6 +386,11 @@ void pci_bridge_initfn(PCIDevice *dev, const char
*typename)
pci_bridge_region_init(br);
QLIST_INIT(_bus->child);
QLIST_INSERT_HEAD(>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;
+}
}
