Re: [PATCH] pci: SLT must be RO

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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 <marcin.juszkiewicz@linaro.org>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> 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é <philmd@linaro.org>

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