| Message ID | 20220627125534.1035912-1-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | ACPI: VIOT: Fix ACS setup | expand |
Hi Eric, On Mon, Jun 27, 2022 at 02:55:34PM +0200, Eric Auger wrote: > Currently acpi_viot_init() gets called after the pci > device has been scanned and pci_enable_acs() has been called. > So pci_request_acs() fails to be taken into account leading > to wrong single iommu group topologies when dealing with > multi-function root ports for instance. > > We cannot simply move the acpi_viot_init() earlier, similarly > as the IORT init because the VIOT parsing relies on the pci > scan. However we can detect VIOT is present earlier and in > such a case, request ACS. Introduce a new acpi_viot_early_init() > routine that allows to call pci_request_acs() before the scan. > > Fixes: 3cf485540e7b ("ACPI: Add driver for the VIOT table") > Signed-off-by: Eric Auger <eric.auger@redhat.com> > Reported-by: Jin Liu <jinl@redhat.com> Thanks for the fix, the patch makes sense and fixes the issue. I wondered whether we should keep the logic where we only request ACS if an IOMMU is found to manage a PCI range, but I can't see any harm in requesting it regardless (plus there is a precedent with AMD IOMMU). I could imagine some VMM wanting to only put an IOMMU in front of its MMIO devices and leave PCI to roam free, but that seems like a stretch. There is another issue with the existing code, though: we can't call pci_request_acs() when CONFIG_PCI is disabled because no stub is defined. Could you wrap the call in an #ifdef? > --- > drivers/acpi/bus.c | 1 + > drivers/acpi/viot.c | 23 +++++++++++++++++------ > include/linux/acpi_viot.h | 2 ++ > 3 files changed, 20 insertions(+), 6 deletions(-) > > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c > index 86fa61a21826..906ad8153fd9 100644 > --- a/drivers/acpi/bus.c > +++ b/drivers/acpi/bus.c > @@ -1400,6 +1400,7 @@ static int __init acpi_init(void) > > pci_mmcfg_late_init(); > acpi_iort_init(); > + acpi_viot_early_init(); > acpi_hest_init(); > acpi_ghes_init(); > acpi_scan_init(); > diff --git a/drivers/acpi/viot.c b/drivers/acpi/viot.c > index d2256326c73a..3c1be123e4d6 100644 > --- a/drivers/acpi/viot.c > +++ b/drivers/acpi/viot.c > @@ -248,6 +248,23 @@ static int __init viot_parse_node(const struct acpi_viot_header *hdr) > return ret; > } > > +/** > + * acpi_viot_early_init - Test the presence of VIOT and enable ACS > + * > + * If the VIOT does exist, ACS must be enabled. This cannot be > + * done in acpi_viot_init() which is called after the bus scan > + */ > +void __init acpi_viot_early_init(void) > +{ > + acpi_status status; > + struct acpi_table_header *hdr; > + > + status = acpi_get_table(ACPI_SIG_VIOT, 0, &hdr); > + if (!ACPI_FAILURE(status)) > + pci_request_acs(); > + acpi_put_table(hdr); I'd rather not call acpi_put_table() in case of failure. I know it is handled but it looks fragile and I couldn't find any other user of acpi_get_table() doing this. > +} > + > /** > * acpi_viot_init - Parse the VIOT table > * > @@ -319,12 +336,6 @@ static int viot_pci_dev_iommu_init(struct pci_dev *pdev, u16 dev_id, void *data) > epid = ((domain_nr - ep->segment_start) << 16) + > dev_id - ep->bdf_start + ep->endpoint_id; > > - /* > - * If we found a PCI range managed by the viommu, we're > - * the one that has to request ACS. > - */ > - pci_request_acs(); > - > return viot_dev_iommu_init(&pdev->dev, ep->viommu, > epid); > } > diff --git a/include/linux/acpi_viot.h b/include/linux/acpi_viot.h > index 1eb8ee5b0e5f..e58d60f8ff2e 100644 > --- a/include/linux/acpi_viot.h > +++ b/include/linux/acpi_viot.h > @@ -6,10 +6,12 @@ > #include <linux/acpi.h> > > #ifdef CONFIG_ACPI_VIOT > +void __init acpi_viot_early_init(void); > void __init acpi_viot_init(void); > int viot_iommu_configure(struct device *dev); > #else > static inline void acpi_viot_init(void) {} > +static inline void acpi_viot_early_init(void) {} nit: different declaration order Thanks, Jean > static inline int viot_iommu_configure(struct device *dev) > { > return -ENODEV; > -- > 2.35.3 >
Hi Jean On 6/29/22 11:14, Jean-Philippe Brucker wrote: > Hi Eric, > > On Mon, Jun 27, 2022 at 02:55:34PM +0200, Eric Auger wrote: >> Currently acpi_viot_init() gets called after the pci >> device has been scanned and pci_enable_acs() has been called. >> So pci_request_acs() fails to be taken into account leading >> to wrong single iommu group topologies when dealing with >> multi-function root ports for instance. >> >> We cannot simply move the acpi_viot_init() earlier, similarly >> as the IORT init because the VIOT parsing relies on the pci >> scan. However we can detect VIOT is present earlier and in >> such a case, request ACS. Introduce a new acpi_viot_early_init() >> routine that allows to call pci_request_acs() before the scan. >> >> Fixes: 3cf485540e7b ("ACPI: Add driver for the VIOT table") >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> Reported-by: Jin Liu <jinl@redhat.com> > > Thanks for the fix, the patch makes sense and fixes the issue. > > I wondered whether we should keep the logic where we only request ACS if > an IOMMU is found to manage a PCI range, but I can't see any harm in > requesting it regardless (plus there is a precedent with AMD IOMMU). Yes that's what I saw too > I could imagine some VMM wanting to only put an IOMMU in front of its MMIO > devices and leave PCI to roam free, but that seems like a stretch. > > There is another issue with the existing code, though: we can't call > pci_request_acs() when CONFIG_PCI is disabled because no stub is defined. > Could you wrap the call in an #ifdef? sure > >> --- >> drivers/acpi/bus.c | 1 + >> drivers/acpi/viot.c | 23 +++++++++++++++++------ >> include/linux/acpi_viot.h | 2 ++ >> 3 files changed, 20 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c >> index 86fa61a21826..906ad8153fd9 100644 >> --- a/drivers/acpi/bus.c >> +++ b/drivers/acpi/bus.c >> @@ -1400,6 +1400,7 @@ static int __init acpi_init(void) >> >> pci_mmcfg_late_init(); >> acpi_iort_init(); >> + acpi_viot_early_init(); >> acpi_hest_init(); >> acpi_ghes_init(); >> acpi_scan_init(); >> diff --git a/drivers/acpi/viot.c b/drivers/acpi/viot.c >> index d2256326c73a..3c1be123e4d6 100644 >> --- a/drivers/acpi/viot.c >> +++ b/drivers/acpi/viot.c >> @@ -248,6 +248,23 @@ static int __init viot_parse_node(const struct acpi_viot_header *hdr) >> return ret; >> } >> >> +/** >> + * acpi_viot_early_init - Test the presence of VIOT and enable ACS >> + * >> + * If the VIOT does exist, ACS must be enabled. This cannot be >> + * done in acpi_viot_init() which is called after the bus scan >> + */ >> +void __init acpi_viot_early_init(void) >> +{ >> + acpi_status status; >> + struct acpi_table_header *hdr; >> + >> + status = acpi_get_table(ACPI_SIG_VIOT, 0, &hdr); >> + if (!ACPI_FAILURE(status)) >> + pci_request_acs(); >> + acpi_put_table(hdr); > > I'd rather not call acpi_put_table() in case of failure. I know it is > handled but it looks fragile and I couldn't find any other user of > acpi_get_table() doing this. OK > >> +} >> + >> /** >> * acpi_viot_init - Parse the VIOT table >> * >> @@ -319,12 +336,6 @@ static int viot_pci_dev_iommu_init(struct pci_dev *pdev, u16 dev_id, void *data) >> epid = ((domain_nr - ep->segment_start) << 16) + >> dev_id - ep->bdf_start + ep->endpoint_id; >> >> - /* >> - * If we found a PCI range managed by the viommu, we're >> - * the one that has to request ACS. >> - */ >> - pci_request_acs(); >> - >> return viot_dev_iommu_init(&pdev->dev, ep->viommu, >> epid); >> } >> diff --git a/include/linux/acpi_viot.h b/include/linux/acpi_viot.h >> index 1eb8ee5b0e5f..e58d60f8ff2e 100644 >> --- a/include/linux/acpi_viot.h >> +++ b/include/linux/acpi_viot.h >> @@ -6,10 +6,12 @@ >> #include <linux/acpi.h> >> >> #ifdef CONFIG_ACPI_VIOT >> +void __init acpi_viot_early_init(void); >> void __init acpi_viot_init(void); >> int viot_iommu_configure(struct device *dev); >> #else >> static inline void acpi_viot_init(void) {} >> +static inline void acpi_viot_early_init(void) {} > > nit: different declaration order OK Thanks Eric > > Thanks, > Jean > > >> static inline int viot_iommu_configure(struct device *dev) >> { >> return -ENODEV; >> -- >> 2.35.3 >>
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index 86fa61a21826..906ad8153fd9 100644 --- a/drivers/acpi/bus.c +++ b/drivers/acpi/bus.c @@ -1400,6 +1400,7 @@ static int __init acpi_init(void) pci_mmcfg_late_init(); acpi_iort_init(); + acpi_viot_early_init(); acpi_hest_init(); acpi_ghes_init(); acpi_scan_init(); diff --git a/drivers/acpi/viot.c b/drivers/acpi/viot.c index d2256326c73a..3c1be123e4d6 100644 --- a/drivers/acpi/viot.c +++ b/drivers/acpi/viot.c @@ -248,6 +248,23 @@ static int __init viot_parse_node(const struct acpi_viot_header *hdr) return ret; } +/** + * acpi_viot_early_init - Test the presence of VIOT and enable ACS + * + * If the VIOT does exist, ACS must be enabled. This cannot be + * done in acpi_viot_init() which is called after the bus scan + */ +void __init acpi_viot_early_init(void) +{ + acpi_status status; + struct acpi_table_header *hdr; + + status = acpi_get_table(ACPI_SIG_VIOT, 0, &hdr); + if (!ACPI_FAILURE(status)) + pci_request_acs(); + acpi_put_table(hdr); +} + /** * acpi_viot_init - Parse the VIOT table * @@ -319,12 +336,6 @@ static int viot_pci_dev_iommu_init(struct pci_dev *pdev, u16 dev_id, void *data) epid = ((domain_nr - ep->segment_start) << 16) + dev_id - ep->bdf_start + ep->endpoint_id; - /* - * If we found a PCI range managed by the viommu, we're - * the one that has to request ACS. - */ - pci_request_acs(); - return viot_dev_iommu_init(&pdev->dev, ep->viommu, epid); } diff --git a/include/linux/acpi_viot.h b/include/linux/acpi_viot.h index 1eb8ee5b0e5f..e58d60f8ff2e 100644 --- a/include/linux/acpi_viot.h +++ b/include/linux/acpi_viot.h @@ -6,10 +6,12 @@ #include <linux/acpi.h> #ifdef CONFIG_ACPI_VIOT +void __init acpi_viot_early_init(void); void __init acpi_viot_init(void); int viot_iommu_configure(struct device *dev); #else static inline void acpi_viot_init(void) {} +static inline void acpi_viot_early_init(void) {} static inline int viot_iommu_configure(struct device *dev) { return -ENODEV;
Currently acpi_viot_init() gets called after the pci device has been scanned and pci_enable_acs() has been called. So pci_request_acs() fails to be taken into account leading to wrong single iommu group topologies when dealing with multi-function root ports for instance. We cannot simply move the acpi_viot_init() earlier, similarly as the IORT init because the VIOT parsing relies on the pci scan. However we can detect VIOT is present earlier and in such a case, request ACS. Introduce a new acpi_viot_early_init() routine that allows to call pci_request_acs() before the scan. Fixes: 3cf485540e7b ("ACPI: Add driver for the VIOT table") Signed-off-by: Eric Auger <eric.auger@redhat.com> Reported-by: Jin Liu <jinl@redhat.com> --- drivers/acpi/bus.c | 1 + drivers/acpi/viot.c | 23 +++++++++++++++++------ include/linux/acpi_viot.h | 2 ++ 3 files changed, 20 insertions(+), 6 deletions(-)