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

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

On Mon, Nov 09, 2026 at 10:12:25AM +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 |  2 ++
>  hw/pci/pci_bridge.c   | 13 ++++++++++++-
>  hw/pci/pcie.c         | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> index ec25e7a7de99..c362f55f9b58 100644
> --- a/include/hw/pci/pcie.h
> +++ b/include/hw/pci/pcie.h
> @@ -112,6 +112,8 @@ 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_slot_link_down(PCIDevice *dev);
> +void pcie_cap_slot_link_up(PCIDevice *dev);
>  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..747d90e1ca46 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_slot_link_down(d);
> +        }
>          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_slot_link_up(d);
> +        }
> +    }
>  }
>  
>  void pci_bridge_disable_base_limit(PCIDevice *dev)
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index bd00305ecca6..e89a65eea37b 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -803,6 +803,7 @@ void pcie_cap_slot_reset(PCIDevice *dev)
>                                   PCI_EXP_SLTSTA_PDC |
>                                   PCI_EXP_SLTSTA_ABP);
>  
> +    pcie_cap_slot_link_up(dev);
>      pcie_cap_update_power(dev);
>      hotplug_event_update_event_status(dev);
>  }
> @@ -946,6 +947,38 @@ void pcie_cap_slot_push_attention_button(PCIDevice *dev)
>      pcie_cap_slot_event(dev, PCI_EXP_HP_EV_ABP);
>  }
>  
> +/*
> + * PCIe r6.5, sec 7.5.3.8: setting the SBR bit eventually transitions
> + * the port to DL_Down, clearing DLLLA.
> + */
> +void pcie_cap_slot_link_down(PCIDevice *dev)
> +{
> +    assert(pci_is_express_downstream_port(dev));
> +    uint8_t *exp_cap = dev->config + dev->exp.exp_cap;
> +
> +    pci_word_test_and_clear_mask(exp_cap + PCI_EXP_LNKSTA,
> +                                 PCI_EXP_LNKSTA_DLLLA);
> +}
> +
> +/*
> + * PCIe r6.5, sec 7.5.3.8: on SBR de-assertion the link retrains and
> + * DLLLA is re-asserted if a device is still present.
> + */
> +void pcie_cap_slot_link_up(PCIDevice *dev)
> +{
> +    assert(pci_is_express_downstream_port(dev));
> +    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);
> +
> +    if (sec_bus->devices[0] &&
> +        (dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA ||
> +         (lnkcap & PCI_EXP_LNKCAP_DLLLARC))) {
> +        pci_word_test_and_set_mask(exp_cap + PCI_EXP_LNKSTA,
> +                                   PCI_EXP_LNKSTA_DLLLA);
> +    }
> +}
> +

It would be slightly simpler if we just have a function that
sets DLLLA to the correct value.

Something like (completely untested):


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);
}

Then we just call it everywhere.


>  /* root control/capabilities/status. PME isn't emulated for now */
>  void pcie_cap_root_init(PCIDevice *dev)
>  {
> -- 
> 2.56.0