Re: [PATCH] pci: SLT must be RO

["Michael S. Tsirkin" <mst@redhat.com>]

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 <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.

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