Re: [PATCH for-12.0 v2] pcie: Model DLLLA transitions on Secondary Bus Reset

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

On Thu, Nov 12, 2026 at 11:28:34AM +0100, Cédric Le Goater wrote:
> On 11/11/26 13:48, Michael S. Tsirkin wrote:
> > On Tue, Nov 10, 2026 at 07:02:04AM +0100, Cédric Le Goater wrote:
> > > Per PCIe r6.5 sec 6.6.1 and 7.5.3.8, setting the Secondary Bus Reset
> > > bit on a downstream port drives the Data Link Layer to DL_Down,
> > > clearing the Data Link Layer Link Active bit (DLLLA). On de-assertion
> > > the link retrains and DLLLA is re-asserted.
> > > 
> > > Currently DLLLA is only managed during plug/unplug events, so a
> > > guest sees stale DLLLA=1 while SBR is in progress.
> > > 
> > > Handle both SBR transitions in pci_bridge_write_config(), the common
> > > config write path for all bridge types, guarded by
> > > pci_is_express_downstream_port(). Also restore DLLLA in
> > > pcie_cap_slot_reset() so that a system reset while SBR is asserted
> > > does not leave DLLLA stuck at 0.
> > > 
> > > Signed-off-by: Cédric Le Goater <clg@redhat.com>
> > > ---
> > >   include/hw/pci/pcie.h |  1 +
> > >   hw/pci/pci_bridge.c   | 13 ++++++++++++-
> > >   hw/pci/pcie.c         | 20 ++++++++++++++++++++
> > >   3 files changed, 33 insertions(+), 1 deletion(-)
> > > 
> > >   Changes in v2 :
> > > 
> > >   - Introduced pcie_cap_update_slot_link() as suggested by Michael.
> > > 
> > > diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> > > index ec25e7a7de99..50fbbacb2200 100644
> > > --- a/include/hw/pci/pcie.h
> > > +++ b/include/hw/pci/pcie.h
> > > @@ -112,6 +112,7 @@ void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slt_ctl, uint16_t *slt_sta);
> > >   void pcie_cap_slot_write_config(PCIDevice *dev,
> > >                                   uint16_t old_slt_ctl, uint16_t old_slt_sta,
> > >                                   uint32_t addr, uint32_t val, int len);
> > > +void pcie_cap_update_slot_link(PCIDevice *dev, uint16_t bctl);
> > >   int pcie_cap_slot_post_load(void *opaque, int version_id);
> > >   void pcie_cap_slot_push_attention_button(PCIDevice *dev);
> > >   void pcie_cap_slot_enable_power(PCIDevice *dev);
> > > diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> > > index e85932e41a92..a11cf2f20afa 100644
> > > --- a/hw/pci/pci_bridge.c
> > > +++ b/hw/pci/pci_bridge.c
> > > @@ -33,6 +33,7 @@
> > >   #include "qemu/units.h"
> > >   #include "hw/pci/pci_bridge.h"
> > >   #include "hw/pci/pci_bus.h"
> > > +#include "hw/pci/pcie.h"
> > >   #include "qemu/module.h"
> > >   #include "qemu/range.h"
> > >   #include "qapi/error.h"
> > > @@ -274,9 +275,19 @@ void pci_bridge_write_config(PCIDevice *d,
> > >       newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> > >       if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) {
> > > -        /* Trigger hot reset on 0->1 transition. */
> > > +        /* SBR assertion (0->1): bring link down, then reset devices. */
> > > +        if (pci_is_express_downstream_port(d)) {
> > > +            pcie_cap_update_slot_link(d, newctl);
> > > +        }
> > >           bus_cold_reset(BUS(&s->sec_bus));
> > >       }
> > > +
> > > +    if (oldctl & ~newctl & PCI_BRIDGE_CTL_BUS_RESET) {
> > > +        /* SBR de-assertion (1->0): bring link back up. */
> > > +        if (pci_is_express_downstream_port(d)) {
> > > +            pcie_cap_update_slot_link(d, newctl);
> > > +        }
> > > +    }
> > >   }
> > >   void pci_bridge_disable_base_limit(PCIDevice *dev)
> > 
> > 
> > I'd rather call it once, unconditionally: it's idempotent.
> > 
> > Or if you really want:
> > 	if ((oldctl ^ newctl) & PCI_BRIDGE_CTL_BUS_RESET)
> 
> yes. Looks better. How's that ?
> 
> @@ -273,6 +274,12 @@ void pci_bridge_write_config(PCIDevice *
>      }
>      newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> +    if ((oldctl ^ newctl) & PCI_BRIDGE_CTL_BUS_RESET) {
> +        if (pci_is_express_downstream_port(d)) {
> +            pcie_cap_update_slot_link(d, newctl);
> +        }
> +    }
> +
>      if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) {
>          /* Trigger hot reset on 0->1 transition. */
>          bus_cold_reset(BUS(&s->sec_bus));
> 
> 
> C.

LGTM

> > 
> > 
> > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > index bd00305ecca6..3b6dcd15aa44 100644
> > > --- a/hw/pci/pcie.c
> > > +++ b/hw/pci/pcie.c
> > > @@ -803,6 +803,8 @@ void pcie_cap_slot_reset(PCIDevice *dev)
> > >                                    PCI_EXP_SLTSTA_PDC |
> > >                                    PCI_EXP_SLTSTA_ABP);
> > > +    pcie_cap_update_slot_link(dev,
> > > +                              pci_get_word(dev->config + PCI_BRIDGE_CONTROL));
> > >       pcie_cap_update_power(dev);
> > >       hotplug_event_update_event_status(dev);
> > >   }
> > > @@ -946,6 +948,24 @@ void pcie_cap_slot_push_attention_button(PCIDevice *dev)
> > >       pcie_cap_slot_event(dev, PCI_EXP_HP_EV_ABP);
> > >   }
> > > +/*
> > > + * Set DLLLA to reflect the actual link state: up iff a device is
> > > + * present on the secondary bus and SBR is not asserted.
> > > + */
> > > +void pcie_cap_update_slot_link(PCIDevice *dev, uint16_t bctl)
> > > +{
> > > +    PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
> > > +    uint8_t *exp_cap = dev->config + dev->exp.exp_cap;
> > > +    uint32_t lnkcap = pci_get_long(exp_cap + PCI_EXP_LNKCAP);
> > > +    bool link_up = sec_bus->devices[0] && !(bctl & PCI_BRIDGE_CTL_BUS_RESET);
> > > +
> > > +    if (dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA ||
> > > +        (lnkcap & PCI_EXP_LNKCAP_DLLLARC)) {
> > > +        pci_set_word_by_mask(exp_cap + PCI_EXP_LNKSTA,
> > > +                             PCI_EXP_LNKSTA_DLLLA, link_up);
> > > +    }
> > > +}
> > > +
> > >   /* root control/capabilities/status. PME isn't emulated for now */
> > >   void pcie_cap_root_init(PCIDevice *dev)
> > >   {
> > > -- 
> > > 2.56.0
> >