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

[dmukhin@xxxxxxxx]

On Tue, May 19, 2026 at 08:00:07PM -0700, dmukhin@xxxxxxxx wrote:
> 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.
> 
> >  
> >          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)

Oh, cool, that is exactly patch 3/5

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