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

hshoexer <[email protected]>
Newsgroups gmane.os.openbsd.tech
Message-ID <[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?

> 
> 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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.