Re: [PATCH v2 14/15] ehci-dbgp: Use pci_sbdf_t instead of (bus, slot, func)

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/06/2026 3:50 pm, Teddy Astie wrote:
> We also take the opportunity to allow the device to exist outside
> of segment 0 (only when specified with pci@ syntax), since it's
> not hardcoded anymore.
>
> Signed-off-by: Teddy Astie <teddy.astie@xxxxxxxxxx>

$ bloat-o-meter -c xen-syms-{before,after}
add/remove: 0/0 grow/shrink: 0/9 up/down: 0/-560 (-560)
Function                                     old     new   delta
ehci_dbgp_check_release.cold                  72      64      -8
dbgp_op                                      260     244     -16
ehci_dbgp_init_postirq                       121      97     -24
ehci_dbgp_suspend                             87      62     -25
ehci_dbgp_resume                             150     102     -48
nvidia_set_debug_port                        100      50     -50
ehci_dbgp_init_preirq                        344     239    -105
ehci_dbgp_setup                              692     568    -124
ehci_dbgp_init                               778     618    -160
Total: Before=2016873, After=2016313, chg -0.03%
add/remove: 0/0 grow/shrink: 1/0 up/down: 8/0 (8)
Data                                         old     new   delta
ehci_dbgp                                    144     152      +8
Total: Before=2209383, After=2209391, chg +0.00%


It's a very clear demonstration of how expensive the split parameter
passing is in practice.

> ---
>  xen/drivers/char/ehci-dbgp.c | 71 ++++++++++++------------------------
>  1 file changed, 24 insertions(+), 47 deletions(-)
>
> diff --git a/xen/drivers/char/ehci-dbgp.c b/xen/drivers/char/ehci-dbgp.c
> index a9bf7dbadb..6d0a6c2ef0 100644
> --- a/xen/drivers/char/ehci-dbgp.c
> +++ b/xen/drivers/char/ehci-dbgp.c
> @@ -335,7 +335,8 @@ struct ehci_dbgp {
>      struct timer timer;
>      spinlock_t *lock;
>      bool reset_run;
> -    u8 bus, slot, func, bar;
> +    u8 bar;
> +    pci_sbdf_t sbdf;

This struct is horribly packed, but putting sbdf between lock and
reset_run will pack better than having it here.

> @@ -1009,20 +1008,18 @@ static set_debug_port_t __read_mostly set_debug_port 
> = default_set_debug_port;
>  static void cf_check nvidia_set_debug_port(
>      struct ehci_dbgp *dbgp, unsigned int port)
>  {
> -    uint32_t dword = pci_conf_read32(PCI_SBDF(0, dbgp->bus, dbgp->slot,
> -                                              dbgp->func), 0x74);
> +    uint32_t dword = pci_conf_read32(dbgp->sbdf, 0x74);
>  
>      dword &= ~(0x0f << 12);
>      dword |= (port & 0x0f) << 12;
> -    pci_conf_write32(PCI_SBDF(0, dbgp->bus, dbgp->slot, dbgp->func), 0x74,
> +    pci_conf_write32(dbgp->sbdf, 0x74,
>                       dword);

This can be folded onto one line.

> @@ -1570,8 +1548,7 @@ int dbgp_op(const struct physdev_dbgp_op *op)
>      case PHYSDEVOP_DBGP_BUS_UNKNOWN:
>          break;
>      case PHYSDEVOP_DBGP_BUS_PCI:
> -        if ( op->u.pci.seg || ehci_dbgp.bus != op->u.pci.bus ||
> -            PCI_DEVFN(ehci_dbgp.slot, ehci_dbgp.func) != op->u.pci.devfn )
> +        if ( PCI_SBDF(op->u.pci.seg, op->u.pci.bus, op->u.pci.devfn).sbdf != 
> ehci_dbgp.sbdf.sbdf )
>

This wants wrapping, somehow.

With suitable adjustments, Reviewed-by: Andrew Cooper
<andrew.cooper3@xxxxxxxxxx>