Re: [PATCH 2/5] vtd: Use pci_sbdf_t in acpi_parse_dev_scope()

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Le 20/05/2026 à 05:03, dmukhin@xxxxxxxx a écrit :
> On Mon, May 18, 2026 at 05:21:26PM +0200, Teddy Astie wrote:
> > Use a dedicated pci_sbdf_t struct that we update instead of recreating
> > one each time we need it.
> >
> > Signed-off-by: Teddy Astie <teddy.astie@xxxxxxxxxx>
> > ---
> >   xen/drivers/passthrough/vtd/dmar.c | 42 ++++++++++++------------------
> >   1 file changed, 16 insertions(+), 26 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/vtd/dmar.c 
> > b/xen/drivers/passthrough/vtd/dmar.c
> > index 2a756831a6..c36f4bbd7b 100644
> > --- a/xen/drivers/passthrough/vtd/dmar.c
> > +++ b/xen/drivers/passthrough/vtd/dmar.c
> > @@ -310,7 +310,7 @@ static int __init acpi_parse_dev_scope(
> >   {
> >       struct acpi_ioapic_unit *acpi_ioapic_unit;
> >       const struct acpi_dmar_device_scope *acpi_scope;
> > -    u16 bus, sub_bus, sec_bus;
> > +    u16 sub_bus, sec_bus;
> >       const struct acpi_dmar_pci_path *path;
> >       struct acpi_drhd_unit *drhd = type == DMAR_TYPE ?
> >           container_of(scope, struct acpi_drhd_unit, scope) : NULL;
> > @@ -332,29 +332,26 @@ static int __init acpi_parse_dev_scope(
> >   
> >       while ( start < end )
> >       {
> > +        pci_sbdf_t dev_sbdf;
> >           acpi_scope = start;
> >           path = (const void *)(acpi_scope + 1);
> >           depth = (acpi_scope->length - sizeof(*acpi_scope)) / sizeof(*path);
> > -        bus = acpi_scope->bus;
> > +        dev_sbdf = PCI_SBDF(seg, acpi_scope->bus, path->dev, path->fn);
> `dev_sbdf` calculation depends on `path` which is updated in `while()` loop
> below.
Ah yes indeed, good catch.

Fixed locally.

> >   
> >           while ( --depth > 0 )
> >           {
> > -            bus = pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
> > -                                 PCI_SECONDARY_BUS);
> > +            dev_sbdf.bus = pci_conf_read8(dev_sbdf, PCI_SECONDARY_BUS);
> >               path++;
> >           }
> >   
> >           switch ( acpi_scope->entry_type )
> >           {
> >           case ACPI_DMAR_SCOPE_TYPE_BRIDGE:
> > -            sec_bus = pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
> > -                                     PCI_SECONDARY_BUS);
> > -            sub_bus = pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
> > -                                     PCI_SUBORDINATE_BUS);
> > +            sec_bus = pci_conf_read8(dev_sbdf, PCI_SECONDARY_BUS);
> > +            sub_bus = pci_conf_read8(dev_sbdf, PCI_SUBORDINATE_BUS);
> >               if ( iommu_verbose )
> >                   printk(VTDPREFIX " bridge: %pp start=%x sec=%x sub=%x\n",
> > -                       &PCI_SBDF(seg, bus, path->dev, path->fn),
> > -                       acpi_scope->bus, sec_bus, sub_bus);
> > +                       &dev_sbdf, acpi_scope->bus, sec_bus, sub_bus);
> >   
> >               dmar_scope_add_buses(scope, sec_bus, sub_bus);
> >               gfx_only = false;
> > @@ -362,8 +359,7 @@ static int __init acpi_parse_dev_scope(
> >   
> >           case ACPI_DMAR_SCOPE_TYPE_HPET:
> >               if ( iommu_verbose )
> > -                printk(VTDPREFIX " MSI HPET: %pp\n",
> > -                       &PCI_SBDF(seg, bus, path->dev, path->fn));
> > +                printk(VTDPREFIX " MSI HPET: %pp\n", &dev_sbdf);
> >   
> >               if ( drhd )
> >               {
> > @@ -374,9 +370,7 @@ static int __init acpi_parse_dev_scope(
> >                   if ( !acpi_hpet_unit )
> >                       goto out;
> >                   acpi_hpet_unit->id = acpi_scope->enumeration_id;
> > -                acpi_hpet_unit->bus = bus;
> > -                acpi_hpet_unit->dev = path->dev;
> > -                acpi_hpet_unit->func = path->fn;
> > +                acpi_hpet_unit->bdf = dev_sbdf.bdf;
> >                   list_add(&acpi_hpet_unit->list, &drhd->hpet_list);
> >   
> >                   gfx_only = false;
> > @@ -386,16 +380,15 @@ static int __init acpi_parse_dev_scope(
> >   
> >           case ACPI_DMAR_SCOPE_TYPE_ENDPOINT:
> >               if ( iommu_verbose )
> > -                printk(VTDPREFIX " endpoint: %pp\n",
> > -                       &PCI_SBDF(seg, bus, path->dev, path->fn));
> > +                printk(VTDPREFIX " endpoint: %pp\n", &dev_sbdf);
> >   
> > -            if ( drhd && pci_device_detect(seg, bus, path->dev, path->fn) )
> > +            if ( drhd && pci_device_detect(seg, dev_sbdf.bus, dev_sbdf.dev, 
> > dev_sbdf.fn) )
> Looks like `pci_device_detect()` also needs some refactoring...
> (Probably out of scope for this series, though)
>
> >               {
> > -                if ( pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
> > +                if ( pci_conf_read8(dev_sbdf,
> >                                       PCI_CLASS_DEVICE + 1) != 0x03
> >                                       /* PCI_BASE_CLASS_DISPLAY */ )
> >                       gfx_only = false;
> > -                else if ( !seg && !bus && path->dev == 2 && !path->fn )
> > +                else if ( !seg && !dev_sbdf.bus && path->dev == 2 && !path->fn 
> > )
> >                       igd_drhd_address = drhd->address;
> >               }
> >   
> > @@ -403,8 +396,7 @@ static int __init acpi_parse_dev_scope(
> >   
> >           case ACPI_DMAR_SCOPE_TYPE_IOAPIC:
> >               if ( iommu_verbose )
> > -                printk(VTDPREFIX " IOAPIC: %pp\n",
> > -                       &PCI_SBDF(seg, bus, path->dev, path->fn));
> > +                printk(VTDPREFIX " IOAPIC: %pp\n", &dev_sbdf);
> >   
> >               if ( drhd )
> >               {
> > @@ -413,9 +405,7 @@ static int __init acpi_parse_dev_scope(
> >                   if ( !acpi_ioapic_unit )
> >                       goto out;
> >                   acpi_ioapic_unit->apic_id = acpi_scope->enumeration_id;
> > -                acpi_ioapic_unit->ioapic.bdf.bus = bus;
> > -                acpi_ioapic_unit->ioapic.bdf.dev = path->dev;
> > -                acpi_ioapic_unit->ioapic.bdf.func = path->fn;
> > +                acpi_ioapic_unit->ioapic.info = dev_sbdf.bdf;
> >                   list_add(&acpi_ioapic_unit->list, &drhd->ioapic_list);
> >   
> >                   gfx_only = false;
> > @@ -431,7 +421,7 @@ static int __init acpi_parse_dev_scope(
> >               gfx_only = false;
> >               continue;
> >           }
> > -        scope->devices[didx++] = PCI_BDF(bus, path->dev, path->fn);
> > +        scope->devices[didx++] = dev_sbdf.bdf;
> >           start += acpi_scope->length;
> >       }
> >   
> > --
> > 2.52.0
> >
> >
> >
> > --
> > Teddy Astie | Vates XCP-ng Developer
> >
> > XCP-ng & Xen Orchestra - Vates solutions
> >
> > web: https://vates.tech
Teddy

Attachment: OpenPGP_0x660FA9D102CBCFD0.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature