Re: acpidmar(4): Store PCI domain in softc for segment mapping

> Date: Mon, 10 Aug 2026 16:13:54 +0200
> From: hshoexer <[email protected]>
> 
> On Wed, Jul 29, 2026 at 11:12:52PM +0200, Mark Kettenis wrote:
> > > Date: Tue, 28 Jul 2026 15:39:46 +0200
> > > From: hshoexer <[email protected]>
> > > 
> > > Hi,
> > > 
> > > with this change I store the PCI domain in the softc.  Later I use
> > > that value when mapping domain to segment.  Instead of counting.
> > 
> > The way acpipci(4) works we already have a 1:1 mapping between PCI
> > segments and PCI domains.  Technically that isn't necessarily correct,
> > but in practice that seems to be the only workable approach.
> > 
> > > Also fall back to segment 0, when ACPI did not provide a segment.
> > 
> > If APCI doesn't provide a segment, we already set it to 0.  This is
> > required by the standard.  So...
> > 
> > > This actually enforces use of the IOMMU.
> > >     
> > > While there:
> > >  - tweak printf to use segment instead of PCI domain
> > >  - use ACPI provided segment instead of hardcoded 0 in ivhd_showpage()
> > > 
> > > ok?
> > > 
> > > Take care,
> > > HJ.
> > > 
> > > -----------------------------------------------------------------------------
> > > diff --git a/sys/arch/amd64/pci/acpipci.c b/sys/arch/amd64/pci/acpipci.c
> > > index e3121987654..e5e861662d7 100644
> > > --- a/sys/arch/amd64/pci/acpipci.c
> > > +++ b/sys/arch/amd64/pci/acpipci.c
> > > @@ -66,6 +66,7 @@ struct acpipci_softc {
> > >  	char		sc_memex_name[32];
> > >  	int		sc_bus;
> > >  	uint32_t	sc_seg;
> > > +	int		sc_domain;
> > >  };
> > >  
> > >  int	acpipci_match(struct device *, void *, void *);
> > > @@ -97,15 +98,14 @@ int
> > >  acpipci_domain_to_seg(int domain)
> > >  {
> > >  	struct acpipci_softc *sc;
> > > -	int i, d = 0;
> > > +	int i;
> > >  
> > >  	for (i = 0; i < acpipci_cd.cd_ndevs; i++) {
> > >  		sc = (struct acpipci_softc *)acpipci_cd.cd_devs[i];
> > >  		if (sc == NULL)
> > >  			continue;
> > > -		if (d == domain)
> > > +		if (sc->sc_domain == domain)
> > >  			return sc->sc_seg;
> > > -		d++;
> > >  	}
> > >  
> > >  	return -1;
> > > @@ -147,6 +147,9 @@ acpipci_attach(struct device *parent, struct device *self, void *aux)
> > >  	aml_evalinteger(sc->sc_acpi, sc->sc_node, "_SEG", 0, NULL, &seg);
> > >  	sc->sc_seg = seg;
> > >  
> > > +	/* Assigned when the PCI bus attaches. */
> > > +	sc->sc_domain = -1;
> > > +
> > >  	if (aml_evalname(sc->sc_acpi, sc->sc_node, "_CRS", 0, NULL, &res)) {
> > >  		printf(": can't find resources\n");
> > >  
> > > @@ -210,6 +213,7 @@ acpipci_attach_bus(struct device *parent, struct acpipci_softc *sc)
> > >  	pba.pba_pmemex = sc->sc_memex;
> > >  	pba.pba_domain = pci_ndomains++;
> > >  	pba.pba_bus = sc->sc_bus;
> > > +	sc->sc_domain = pba.pba_domain;
> > >  
> > >  	/* Enable MSI in ACPI 2.0 and above, unless we're told not to. */
> > >  	if (sc->sc_acpi->sc_fadt->hdr.revision >= 2 &&
> > > diff --git a/sys/dev/acpi/acpidmar.c b/sys/dev/acpi/acpidmar.c
> > > index 57b90ddb193..cf18265fd09 100644
> > > --- a/sys/dev/acpi/acpidmar.c
> > > +++ b/sys/dev/acpi/acpidmar.c
> > > @@ -2581,9 +2581,8 @@ acpidmar_pci_hook(pci_chipset_tag_t pc, struct pci_attach_args *pa)
> > >  
> > >  	segment = acpipci_domain_to_seg(pa->pa_domain);
> > >  	if (segment < 0) {
> > > -		DPRINTF(1, "acpidmar: no ACPI segment for pci domain %d\n",
> > > -		    pa->pa_domain);
> > > -		return;
> > > +		/* Fall back to segment 0. */
> > > +		segment = 0;
> > >  	}
> > 
> > So it we ever get a -1 here, somethings is really wrong.  So I'd
> > probably turn this into:
> > 
> >         KASSERT(segment >= 0);
> 
> good point!  Uupdated diff below.
> 
> ok?

ok kettenis@

> > Otherwise this looks good.
> > 
> > I'm still curious to see any x86 AML that actually uses non-zero _SEG
> > numbers.
> > 
> > >  	/* Record PCI-PCI bridge forwarding windows */
> > > @@ -2608,7 +2607,7 @@ acpidmar_pci_hook(pci_chipset_tag_t pc, struct pci_attach_args *pa)
> > >  	    PCI_SUBCLASS(reg) == PCI_SUBCLASS_BRIDGE_ISA) {
> > >  		/* For ISA Bridges, map 0-16Mb as 1:1 */
> > >  		printf("dmar: %.4x:%.2x:%.2x.%x mapping ISA\n",
> > > -		    pa->pa_domain, bus, dev, fun);
> > > +		    segment, bus, dev, fun);
> > >  		domain_map_pthru(dom, 0x00, 16*1024*1024);
> > >  
> > >  		/* Keep the identity mapped IOVA range out of the allocator */
> > > @@ -2917,7 +2916,7 @@ ivhd_showpage(struct iommu_softc *iommu, int sid, paddr_t paddr)
> > >  	if (show > 10)
> > >  		return;
> > >  	show++;
> > > -	dom = acpidmar_pci_attach(acpidmar_sc, 0, sid, 0);
> > > +	dom = acpidmar_pci_attach(acpidmar_sc, iommu->segment, sid, 0);
> > >  	if (!dom)
> > >  		return;
> > >  	printf("DTE: %.8x %.8x %.8x %.8x %.8x %.8x %.8x %.8x\n",
> > > 
> > > 
> ------------
> acpidmar(4): Store PCI domain in softc for segment mapping
> 
> Ensure we get a valid segement and enforce use of the IOMMU.
> 
> While there:
>  - tweak printf to use segment instead of PCI domain
>  - use ACPI provided segment instead of hardcoded 0 in ivhd_showpage()
> ---
>  sys/arch/amd64/pci/acpipci.c | 10 +++++++---
>  sys/dev/acpi/acpidmar.c      | 10 +++-------
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/sys/arch/amd64/pci/acpipci.c b/sys/arch/amd64/pci/acpipci.c
> index e3121987654..e5e861662d7 100644
> --- a/sys/arch/amd64/pci/acpipci.c
> +++ b/sys/arch/amd64/pci/acpipci.c
> @@ -66,6 +66,7 @@ struct acpipci_softc {
>  	char		sc_memex_name[32];
>  	int		sc_bus;
>  	uint32_t	sc_seg;
> +	int		sc_domain;
>  };
>  
>  int	acpipci_match(struct device *, void *, void *);
> @@ -97,15 +98,14 @@ int
>  acpipci_domain_to_seg(int domain)
>  {
>  	struct acpipci_softc *sc;
> -	int i, d = 0;
> +	int i;
>  
>  	for (i = 0; i < acpipci_cd.cd_ndevs; i++) {
>  		sc = (struct acpipci_softc *)acpipci_cd.cd_devs[i];
>  		if (sc == NULL)
>  			continue;
> -		if (d == domain)
> +		if (sc->sc_domain == domain)
>  			return sc->sc_seg;
> -		d++;
>  	}
>  
>  	return -1;
> @@ -147,6 +147,9 @@ acpipci_attach(struct device *parent, struct device *self, void *aux)
>  	aml_evalinteger(sc->sc_acpi, sc->sc_node, "_SEG", 0, NULL, &seg);
>  	sc->sc_seg = seg;
>  
> +	/* Assigned when the PCI bus attaches. */
> +	sc->sc_domain = -1;
> +
>  	if (aml_evalname(sc->sc_acpi, sc->sc_node, "_CRS", 0, NULL, &res)) {
>  		printf(": can't find resources\n");
>  
> @@ -210,6 +213,7 @@ acpipci_attach_bus(struct device *parent, struct acpipci_softc *sc)
>  	pba.pba_pmemex = sc->sc_memex;
>  	pba.pba_domain = pci_ndomains++;
>  	pba.pba_bus = sc->sc_bus;
> +	sc->sc_domain = pba.pba_domain;
>  
>  	/* Enable MSI in ACPI 2.0 and above, unless we're told not to. */
>  	if (sc->sc_acpi->sc_fadt->hdr.revision >= 2 &&
> diff --git a/sys/dev/acpi/acpidmar.c b/sys/dev/acpi/acpidmar.c
> index 5771a54ffc9..57581f0e48a 100644
> --- a/sys/dev/acpi/acpidmar.c
> +++ b/sys/dev/acpi/acpidmar.c
> @@ -2556,11 +2556,7 @@ acpidmar_pci_hook(pci_chipset_tag_t pc, struct pci_attach_args *pa)
>  	reg = pci_conf_read(pc, pa->pa_tag, PCI_CLASS_REG);
>  
>  	segment = acpipci_domain_to_seg(pa->pa_domain);
> -	if (segment < 0) {
> -		DPRINTF(1, "acpidmar: no ACPI segment for pci domain %d\n",
> -		    pa->pa_domain);
> -		return;
> -	}
> +	KASSERT(segment >= 0);
>  
>  	/* Record PCI-PCI bridge forwarding windows */
>  	bhlc = pci_conf_read(pc, pa->pa_tag, PCI_BHLC_REG);
> @@ -2584,7 +2580,7 @@ acpidmar_pci_hook(pci_chipset_tag_t pc, struct pci_attach_args *pa)
>  	    PCI_SUBCLASS(reg) == PCI_SUBCLASS_BRIDGE_ISA) {
>  		/* For ISA Bridges, map 0-16Mb as 1:1 */
>  		printf("dmar: %.4x:%.2x:%.2x.%x mapping ISA\n",
> -		    pa->pa_domain, bus, dev, fun);
> +		    segment, bus, dev, fun);
>  		domain_map_pthru(dom, 0x00, 16*1024*1024);
>  
>  		/* Keep the identity mapped IOVA range out of the allocator */
> @@ -2893,7 +2889,7 @@ ivhd_showpage(struct iommu_softc *iommu, int sid, paddr_t paddr)
>  	if (show > 10)
>  		return;
>  	show++;
> -	dom = acpidmar_pci_attach(acpidmar_sc, 0, sid, 0);
> +	dom = acpidmar_pci_attach(acpidmar_sc, iommu->segment, sid, 0);
>  	if (!dom)
>  		return;
>  	printf("DTE: %.8x %.8x %.8x %.8x %.8x %.8x %.8x %.8x\n",
> -- 
> 2.55.0
> 
>