* [PATCH v2 0/3] x86/IOMMU: enabled / intremap handling @ 2021-10-21 9:57 Jan Beulich 2021-10-21 9:58 ` [PATCH v2 1/3] x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing Jan Beulich ` (3 more replies) 0 siblings, 4 replies; 24+ messages in thread From: Jan Beulich @ 2021-10-21 9:57 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, Paul Durrant In the course of reading the response to v1 (patch 1 only) I realized that not only that patch needs further adjustment, but that also further changes are needed (and there's likely yet more amiss). 1: x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing 2: x86/APIC: avoid iommu_supports_x2apic() on error path 3: AMD/IOMMU: iommu_enable vs iommu_intremap Jan ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 1/3] x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing 2021-10-21 9:57 [PATCH v2 0/3] x86/IOMMU: enabled / intremap handling Jan Beulich @ 2021-10-21 9:58 ` Jan Beulich 2021-10-22 5:59 ` Jan Beulich 2021-10-22 15:52 ` Roger Pau Monné 2021-10-21 9:58 ` [PATCH v2 2/3] x86/APIC: avoid iommu_supports_x2apic() on error path Jan Beulich ` (2 subsequent siblings) 3 siblings, 2 replies; 24+ messages in thread From: Jan Beulich @ 2021-10-21 9:58 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, Paul Durrant x2apic_bsp_setup() gets called ahead of iommu_setup(), and since x2APIC mode (physical vs clustered) depends on iommu_intremap, that variable needs to be set to off as soon as we know we can't / won't enable interrupt remapping, i.e. in particular when parsing of the respective ACPI tables failed. Move the turning off of iommu_intremap from AMD specific code into acpi_iommu_init(), accompanying it by clearing of iommu_enable. Take the opportunity and also fully skip ACPI table parsing logic on VT-d when both "iommu=off" and "iommu=no-intremap" are in effect anyway, like was already the case for AMD. The tag below only references the commit uncovering a pre-existing anomaly. Fixes: d8bd82327b0f ("AMD/IOMMU: obtain IVHD type to use earlier") Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- While the change here deals with apic_x2apic_probe() as called from x2apic_bsp_setup(), the check_x2apic_preenabled() path looks to be similarly affected. That call occurs before acpi_boot_init(), which is what calls acpi_iommu_init(). The ordering in setup.c is in part relatively fragile, which is why for the moment I'm still hesitant to move the generic_apic_probe() call down. Plus I don't have easy access to a suitable system to test this case. Thoughts? --- v2: Treat iommu_enable and iommu_intremap as separate options. --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c @@ -183,9 +183,6 @@ int __init acpi_ivrs_init(void) { int rc; - if ( !iommu_enable && !iommu_intremap ) - return 0; - rc = amd_iommu_get_supported_ivhd_type(); if ( rc < 0 ) return rc; @@ -193,10 +190,7 @@ int __init acpi_ivrs_init(void) ivhd_type = rc; if ( (amd_iommu_detect_acpi() !=0) || (iommu_found() == 0) ) - { - iommu_intremap = iommu_intremap_off; return -ENODEV; - } iommu_init_ops = &_iommu_init_ops; --- a/xen/drivers/passthrough/vtd/dmar.c +++ b/xen/drivers/passthrough/vtd/dmar.c @@ -777,11 +777,7 @@ static int __init acpi_parse_dmar(struct dmar = (struct acpi_table_dmar *)table; dmar_flags = dmar->flags; - if ( !iommu_enable && !iommu_intremap ) - { - ret = -EINVAL; - goto out; - } + ASSERT(iommu_enable || iommu_intremap); if ( !dmar->width ) { --- a/xen/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -41,6 +41,24 @@ enum iommu_intremap __read_mostly iommu_ bool __read_mostly iommu_intpost; #endif +void __init acpi_iommu_init(void) +{ + int ret; + + if ( !iommu_enable && !iommu_intremap ) + return; + + ret = acpi_dmar_init(); + if ( ret == -ENODEV ) + ret = acpi_ivrs_init(); + + if ( ret ) + { + iommu_enable = false; + iommu_intremap = iommu_intremap_off; + } +} + int __init iommu_hardware_setup(void) { struct IO_APIC_route_entry **ioapic_entries = NULL; --- a/xen/include/asm-x86/acpi.h +++ b/xen/include/asm-x86/acpi.h @@ -141,16 +141,10 @@ extern u32 x86_acpiid_to_apicid[]; extern u32 pmtmr_ioport; extern unsigned int pmtmr_width; +void acpi_iommu_init(void); int acpi_dmar_init(void); int acpi_ivrs_init(void); -static inline int acpi_iommu_init(void) -{ - int ret = acpi_dmar_init(); - - return ret == -ENODEV ? acpi_ivrs_init() : ret; -} - void acpi_mmcfg_init(void); /* Incremented whenever we transition through S3. Value is 1 during boot. */ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/3] x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing 2021-10-21 9:58 ` [PATCH v2 1/3] x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing Jan Beulich @ 2021-10-22 5:59 ` Jan Beulich 2021-10-29 8:44 ` Tian, Kevin 2021-10-22 15:52 ` Roger Pau Monné 1 sibling, 1 reply; 24+ messages in thread From: Jan Beulich @ 2021-10-22 5:59 UTC (permalink / raw) To: xen-devel Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, Paul Durrant, Kevin Tian On 21.10.2021 11:58, Jan Beulich wrote: > x2apic_bsp_setup() gets called ahead of iommu_setup(), and since x2APIC > mode (physical vs clustered) depends on iommu_intremap, that variable > needs to be set to off as soon as we know we can't / won't enable > interrupt remapping, i.e. in particular when parsing of the respective > ACPI tables failed. Move the turning off of iommu_intremap from AMD > specific code into acpi_iommu_init(), accompanying it by clearing of > iommu_enable. > > Take the opportunity and also fully skip ACPI table parsing logic on > VT-d when both "iommu=off" and "iommu=no-intremap" are in effect anyway, > like was already the case for AMD. > > The tag below only references the commit uncovering a pre-existing > anomaly. > > Fixes: d8bd82327b0f ("AMD/IOMMU: obtain IVHD type to use earlier") > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Ouch, forgot to Cc Kevin; now added. Jan > --- > While the change here deals with apic_x2apic_probe() as called from > x2apic_bsp_setup(), the check_x2apic_preenabled() path looks to be > similarly affected. That call occurs before acpi_boot_init(), which is > what calls acpi_iommu_init(). The ordering in setup.c is in part > relatively fragile, which is why for the moment I'm still hesitant to > move the generic_apic_probe() call down. Plus I don't have easy access > to a suitable system to test this case. Thoughts? > --- > v2: Treat iommu_enable and iommu_intremap as separate options. > > --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c > +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c > @@ -183,9 +183,6 @@ int __init acpi_ivrs_init(void) > { > int rc; > > - if ( !iommu_enable && !iommu_intremap ) > - return 0; > - > rc = amd_iommu_get_supported_ivhd_type(); > if ( rc < 0 ) > return rc; > @@ -193,10 +190,7 @@ int __init acpi_ivrs_init(void) > ivhd_type = rc; > > if ( (amd_iommu_detect_acpi() !=0) || (iommu_found() == 0) ) > - { > - iommu_intremap = iommu_intremap_off; > return -ENODEV; > - } > > iommu_init_ops = &_iommu_init_ops; > > --- a/xen/drivers/passthrough/vtd/dmar.c > +++ b/xen/drivers/passthrough/vtd/dmar.c > @@ -777,11 +777,7 @@ static int __init acpi_parse_dmar(struct > dmar = (struct acpi_table_dmar *)table; > dmar_flags = dmar->flags; > > - if ( !iommu_enable && !iommu_intremap ) > - { > - ret = -EINVAL; > - goto out; > - } > + ASSERT(iommu_enable || iommu_intremap); > > if ( !dmar->width ) > { > --- a/xen/drivers/passthrough/x86/iommu.c > +++ b/xen/drivers/passthrough/x86/iommu.c > @@ -41,6 +41,24 @@ enum iommu_intremap __read_mostly iommu_ > bool __read_mostly iommu_intpost; > #endif > > +void __init acpi_iommu_init(void) > +{ > + int ret; > + > + if ( !iommu_enable && !iommu_intremap ) > + return; > + > + ret = acpi_dmar_init(); > + if ( ret == -ENODEV ) > + ret = acpi_ivrs_init(); > + > + if ( ret ) > + { > + iommu_enable = false; > + iommu_intremap = iommu_intremap_off; > + } > +} > + > int __init iommu_hardware_setup(void) > { > struct IO_APIC_route_entry **ioapic_entries = NULL; > --- a/xen/include/asm-x86/acpi.h > +++ b/xen/include/asm-x86/acpi.h > @@ -141,16 +141,10 @@ extern u32 x86_acpiid_to_apicid[]; > extern u32 pmtmr_ioport; > extern unsigned int pmtmr_width; > > +void acpi_iommu_init(void); > int acpi_dmar_init(void); > int acpi_ivrs_init(void); > > -static inline int acpi_iommu_init(void) > -{ > - int ret = acpi_dmar_init(); > - > - return ret == -ENODEV ? acpi_ivrs_init() : ret; > -} > - > void acpi_mmcfg_init(void); > > /* Incremented whenever we transition through S3. Value is 1 during boot. */ > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH v2 1/3] x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing 2021-10-22 5:59 ` Jan Beulich @ 2021-10-29 8:44 ` Tian, Kevin 0 siblings, 0 replies; 24+ messages in thread From: Tian, Kevin @ 2021-10-29 8:44 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: Cooper, Andrew, Wei Liu, Roger Pau Monné, Paul Durrant > From: Jan Beulich <jbeulich@suse.com> > Sent: Friday, October 22, 2021 1:59 PM > > On 21.10.2021 11:58, Jan Beulich wrote: > > x2apic_bsp_setup() gets called ahead of iommu_setup(), and since x2APIC > > mode (physical vs clustered) depends on iommu_intremap, that variable > > needs to be set to off as soon as we know we can't / won't enable > > interrupt remapping, i.e. in particular when parsing of the respective > > ACPI tables failed. Move the turning off of iommu_intremap from AMD > > specific code into acpi_iommu_init(), accompanying it by clearing of > > iommu_enable. > > > > Take the opportunity and also fully skip ACPI table parsing logic on > > VT-d when both "iommu=off" and "iommu=no-intremap" are in effect > anyway, > > like was already the case for AMD. > > > > The tag below only references the commit uncovering a pre-existing > > anomaly. > > > > Fixes: d8bd82327b0f ("AMD/IOMMU: obtain IVHD type to use earlier") > > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Ouch, forgot to Cc Kevin; now added. Reviewed-by: Kevin Tian <kevin.tian@intel.com> > > Jan > > > --- > > While the change here deals with apic_x2apic_probe() as called from > > x2apic_bsp_setup(), the check_x2apic_preenabled() path looks to be > > similarly affected. That call occurs before acpi_boot_init(), which is > > what calls acpi_iommu_init(). The ordering in setup.c is in part > > relatively fragile, which is why for the moment I'm still hesitant to > > move the generic_apic_probe() call down. Plus I don't have easy access > > to a suitable system to test this case. Thoughts? > > --- > > v2: Treat iommu_enable and iommu_intremap as separate options. > > > > --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c > > +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c > > @@ -183,9 +183,6 @@ int __init acpi_ivrs_init(void) > > { > > int rc; > > > > - if ( !iommu_enable && !iommu_intremap ) > > - return 0; > > - > > rc = amd_iommu_get_supported_ivhd_type(); > > if ( rc < 0 ) > > return rc; > > @@ -193,10 +190,7 @@ int __init acpi_ivrs_init(void) > > ivhd_type = rc; > > > > if ( (amd_iommu_detect_acpi() !=0) || (iommu_found() == 0) ) > > - { > > - iommu_intremap = iommu_intremap_off; > > return -ENODEV; > > - } > > > > iommu_init_ops = &_iommu_init_ops; > > > > --- a/xen/drivers/passthrough/vtd/dmar.c > > +++ b/xen/drivers/passthrough/vtd/dmar.c > > @@ -777,11 +777,7 @@ static int __init acpi_parse_dmar(struct > > dmar = (struct acpi_table_dmar *)table; > > dmar_flags = dmar->flags; > > > > - if ( !iommu_enable && !iommu_intremap ) > > - { > > - ret = -EINVAL; > > - goto out; > > - } > > + ASSERT(iommu_enable || iommu_intremap); > > > > if ( !dmar->width ) > > { > > --- a/xen/drivers/passthrough/x86/iommu.c > > +++ b/xen/drivers/passthrough/x86/iommu.c > > @@ -41,6 +41,24 @@ enum iommu_intremap __read_mostly iommu_ > > bool __read_mostly iommu_intpost; > > #endif > > > > +void __init acpi_iommu_init(void) > > +{ > > + int ret; > > + > > + if ( !iommu_enable && !iommu_intremap ) > > + return; > > + > > + ret = acpi_dmar_init(); > > + if ( ret == -ENODEV ) > > + ret = acpi_ivrs_init(); > > + > > + if ( ret ) > > + { > > + iommu_enable = false; > > + iommu_intremap = iommu_intremap_off; > > + } > > +} > > + > > int __init iommu_hardware_setup(void) > > { > > struct IO_APIC_route_entry **ioapic_entries = NULL; > > --- a/xen/include/asm-x86/acpi.h > > +++ b/xen/include/asm-x86/acpi.h > > @@ -141,16 +141,10 @@ extern u32 x86_acpiid_to_apicid[]; > > extern u32 pmtmr_ioport; > > extern unsigned int pmtmr_width; > > > > +void acpi_iommu_init(void); > > int acpi_dmar_init(void); > > int acpi_ivrs_init(void); > > > > -static inline int acpi_iommu_init(void) > > -{ > > - int ret = acpi_dmar_init(); > > - > > - return ret == -ENODEV ? acpi_ivrs_init() : ret; > > -} > > - > > void acpi_mmcfg_init(void); > > > > /* Incremented whenever we transition through S3. Value is 1 during boot. > */ > > > > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/3] x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing 2021-10-21 9:58 ` [PATCH v2 1/3] x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing Jan Beulich 2021-10-22 5:59 ` Jan Beulich @ 2021-10-22 15:52 ` Roger Pau Monné 2021-11-02 10:07 ` Jan Beulich 1 sibling, 1 reply; 24+ messages in thread From: Roger Pau Monné @ 2021-10-22 15:52 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, Paul Durrant On Thu, Oct 21, 2021 at 11:58:18AM +0200, Jan Beulich wrote: > x2apic_bsp_setup() gets called ahead of iommu_setup(), and since x2APIC > mode (physical vs clustered) depends on iommu_intremap, that variable > needs to be set to off as soon as we know we can't / won't enable > interrupt remapping, i.e. in particular when parsing of the respective > ACPI tables failed. Move the turning off of iommu_intremap from AMD > specific code into acpi_iommu_init(), accompanying it by clearing of > iommu_enable. > > Take the opportunity and also fully skip ACPI table parsing logic on > VT-d when both "iommu=off" and "iommu=no-intremap" are in effect anyway, > like was already the case for AMD. > > The tag below only references the commit uncovering a pre-existing > anomaly. > > Fixes: d8bd82327b0f ("AMD/IOMMU: obtain IVHD type to use earlier") > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > --- > While the change here deals with apic_x2apic_probe() as called from > x2apic_bsp_setup(), the check_x2apic_preenabled() path looks to be > similarly affected. That call occurs before acpi_boot_init(), which is > what calls acpi_iommu_init(). The ordering in setup.c is in part > relatively fragile, which is why for the moment I'm still hesitant to > move the generic_apic_probe() call down. Plus I don't have easy access > to a suitable system to test this case. Thoughts? Indeed, that seems it could go quite wrong, as apic_x2apic_probe will see iommu_intremap == iommu_intremap_full (the default value) and thus could choose cluster mode without real interrupt remapping support. At first sight it would seem possible to move lower down, but as you say, this is all quite fragile. It's even made worse because we lack a strict ordering discipline or any kind of dependency checking, so even if we mess up the order it's likely to go unnoticed unless someone tests on an affected system. While we can try to solve this for the upcoming release, long term we need a stricter ordering, either as a comment, or even better enforced somehow in code. The x2APIC vs IOMMU ordering has bitten us multiple times and we should see about implementing a more robust solution. Thanks, Roger. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/3] x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing 2021-10-22 15:52 ` Roger Pau Monné @ 2021-11-02 10:07 ` Jan Beulich 2021-11-02 10:26 ` Roger Pau Monné 0 siblings, 1 reply; 24+ messages in thread From: Jan Beulich @ 2021-11-02 10:07 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu, Paul Durrant On 22.10.2021 17:52, Roger Pau Monné wrote: > On Thu, Oct 21, 2021 at 11:58:18AM +0200, Jan Beulich wrote: >> x2apic_bsp_setup() gets called ahead of iommu_setup(), and since x2APIC >> mode (physical vs clustered) depends on iommu_intremap, that variable >> needs to be set to off as soon as we know we can't / won't enable >> interrupt remapping, i.e. in particular when parsing of the respective >> ACPI tables failed. Move the turning off of iommu_intremap from AMD >> specific code into acpi_iommu_init(), accompanying it by clearing of >> iommu_enable. >> >> Take the opportunity and also fully skip ACPI table parsing logic on >> VT-d when both "iommu=off" and "iommu=no-intremap" are in effect anyway, >> like was already the case for AMD. >> >> The tag below only references the commit uncovering a pre-existing >> anomaly. >> >> Fixes: d8bd82327b0f ("AMD/IOMMU: obtain IVHD type to use earlier") >> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. >> --- >> While the change here deals with apic_x2apic_probe() as called from >> x2apic_bsp_setup(), the check_x2apic_preenabled() path looks to be >> similarly affected. That call occurs before acpi_boot_init(), which is >> what calls acpi_iommu_init(). The ordering in setup.c is in part >> relatively fragile, which is why for the moment I'm still hesitant to >> move the generic_apic_probe() call down. Plus I don't have easy access >> to a suitable system to test this case. Thoughts? > > Indeed, that seems it could go quite wrong, as apic_x2apic_probe will > see iommu_intremap == iommu_intremap_full (the default value) and thus > could choose cluster mode without real interrupt remapping support. > > At first sight it would seem possible to move lower down, but as you > say, this is all quite fragile. It's even made worse because we lack a > strict ordering discipline or any kind of dependency checking, so even > if we mess up the order it's likely to go unnoticed unless someone > tests on an affected system. > > While we can try to solve this for the upcoming release, long term we > need a stricter ordering, either as a comment, or even better enforced > somehow in code. The x2APIC vs IOMMU ordering has bitten us multiple > times and we should see about implementing a more robust solution. So what's your thought then: Make the change (in another patch), or rather leave the code as is? I'm slightly in favor of making the change seeing that you agree that the rearrangement looks to be correct. Jan ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/3] x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing 2021-11-02 10:07 ` Jan Beulich @ 2021-11-02 10:26 ` Roger Pau Monné 0 siblings, 0 replies; 24+ messages in thread From: Roger Pau Monné @ 2021-11-02 10:26 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, Paul Durrant On Tue, Nov 02, 2021 at 11:07:25AM +0100, Jan Beulich wrote: > On 22.10.2021 17:52, Roger Pau Monné wrote: > > On Thu, Oct 21, 2021 at 11:58:18AM +0200, Jan Beulich wrote: > >> x2apic_bsp_setup() gets called ahead of iommu_setup(), and since x2APIC > >> mode (physical vs clustered) depends on iommu_intremap, that variable > >> needs to be set to off as soon as we know we can't / won't enable > >> interrupt remapping, i.e. in particular when parsing of the respective > >> ACPI tables failed. Move the turning off of iommu_intremap from AMD > >> specific code into acpi_iommu_init(), accompanying it by clearing of > >> iommu_enable. > >> > >> Take the opportunity and also fully skip ACPI table parsing logic on > >> VT-d when both "iommu=off" and "iommu=no-intremap" are in effect anyway, > >> like was already the case for AMD. > >> > >> The tag below only references the commit uncovering a pre-existing > >> anomaly. > >> > >> Fixes: d8bd82327b0f ("AMD/IOMMU: obtain IVHD type to use earlier") > >> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > Thanks. > > >> --- > >> While the change here deals with apic_x2apic_probe() as called from > >> x2apic_bsp_setup(), the check_x2apic_preenabled() path looks to be > >> similarly affected. That call occurs before acpi_boot_init(), which is > >> what calls acpi_iommu_init(). The ordering in setup.c is in part > >> relatively fragile, which is why for the moment I'm still hesitant to > >> move the generic_apic_probe() call down. Plus I don't have easy access > >> to a suitable system to test this case. Thoughts? > > > > Indeed, that seems it could go quite wrong, as apic_x2apic_probe will > > see iommu_intremap == iommu_intremap_full (the default value) and thus > > could choose cluster mode without real interrupt remapping support. > > > > At first sight it would seem possible to move lower down, but as you > > say, this is all quite fragile. It's even made worse because we lack a > > strict ordering discipline or any kind of dependency checking, so even > > if we mess up the order it's likely to go unnoticed unless someone > > tests on an affected system. > > > > While we can try to solve this for the upcoming release, long term we > > need a stricter ordering, either as a comment, or even better enforced > > somehow in code. The x2APIC vs IOMMU ordering has bitten us multiple > > times and we should see about implementing a more robust solution. > > So what's your thought then: Make the change (in another patch), or rather > leave the code as is? I'm slightly in favor of making the change seeing > that you agree that the rearrangement looks to be correct. Sorry this wasn't clear, I've rambled too much: making the change in another patch would be my preferred option. I still wonder whether we could place some kind of checks to ensure the logic of interrupt remap vs APIC initialization is executed in the right order, but that's not release material. Thanks, Roger. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 2/3] x86/APIC: avoid iommu_supports_x2apic() on error path 2021-10-21 9:57 [PATCH v2 0/3] x86/IOMMU: enabled / intremap handling Jan Beulich 2021-10-21 9:58 ` [PATCH v2 1/3] x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing Jan Beulich @ 2021-10-21 9:58 ` Jan Beulich 2021-10-25 8:41 ` Roger Pau Monné 2021-10-21 9:59 ` [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap Jan Beulich 2021-11-02 10:17 ` [PATCH v2 0/3] x86/IOMMU: enabled / intremap handling Jan Beulich 3 siblings, 1 reply; 24+ messages in thread From: Jan Beulich @ 2021-10-21 9:58 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, Paul Durrant The value it returns may change from true to false in case iommu_enable_x2apic() fails and, as a side effect, clears iommu_intremap (as can happen at least on AMD). Latch the return value from the first invocation to replace the second one. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: New. --- a/xen/arch/x86/apic.c +++ b/xen/arch/x86/apic.c @@ -865,6 +865,7 @@ void x2apic_ap_setup(void) void __init x2apic_bsp_setup(void) { struct IO_APIC_route_entry **ioapic_entries = NULL; + bool iommu_x2apic; const char *orig_name; if ( !cpu_has_x2apic ) @@ -880,7 +881,8 @@ void __init x2apic_bsp_setup(void) printk("x2APIC: Already enabled by BIOS: Ignoring cmdline disable.\n"); } - if ( iommu_supports_x2apic() ) + iommu_x2apic = iommu_supports_x2apic(); + if ( iommu_x2apic ) { if ( (ioapic_entries = alloc_ioapic_entries()) == NULL ) { @@ -933,8 +935,11 @@ void __init x2apic_bsp_setup(void) printk("Switched to APIC driver %s\n", genapic.name); restore_out: - /* iommu_x2apic_enabled cannot be used here in the error case. */ - if ( iommu_supports_x2apic() ) + /* + * iommu_x2apic_enabled and iommu_supports_x2apic() cannot be used here + * in the error case. + */ + if ( iommu_x2apic ) { /* * NB: do not use raw mode when restoring entries if the iommu has ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/3] x86/APIC: avoid iommu_supports_x2apic() on error path 2021-10-21 9:58 ` [PATCH v2 2/3] x86/APIC: avoid iommu_supports_x2apic() on error path Jan Beulich @ 2021-10-25 8:41 ` Roger Pau Monné 0 siblings, 0 replies; 24+ messages in thread From: Roger Pau Monné @ 2021-10-25 8:41 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, Paul Durrant On Thu, Oct 21, 2021 at 11:58:37AM +0200, Jan Beulich wrote: > The value it returns may change from true to false in case > iommu_enable_x2apic() fails and, as a side effect, clears iommu_intremap > (as can happen at least on AMD). Latch the return value from the first > invocation to replace the second one. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap 2021-10-21 9:57 [PATCH v2 0/3] x86/IOMMU: enabled / intremap handling Jan Beulich 2021-10-21 9:58 ` [PATCH v2 1/3] x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing Jan Beulich 2021-10-21 9:58 ` [PATCH v2 2/3] x86/APIC: avoid iommu_supports_x2apic() on error path Jan Beulich @ 2021-10-21 9:59 ` Jan Beulich 2021-10-25 10:28 ` Roger Pau Monné 2021-11-02 10:17 ` [PATCH v2 0/3] x86/IOMMU: enabled / intremap handling Jan Beulich 3 siblings, 1 reply; 24+ messages in thread From: Jan Beulich @ 2021-10-21 9:59 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, Paul Durrant The two are really meant to be independent settings; iov_supports_xt() using || instead of && was simply wrong. The corrected check is, however, redundant, just like the (correct) one in iov_detect(): These hook functions are unreachable without acpi_ivrs_init() installing the iommu_init_ops pointer, which it does only upon success. (Unlike for VT-d there is no late clearing of iommu_enable due to quirks, and any possible clearing of iommu_intremap happens only after iov_supports_xt() has run.) Signed-off-by: Jan Beulich <jbeulich@suse.com> --- In fact in iov_detect() it could be iommu_enable alone which gets checked, but this felt overly aggressive to me. Instead I'm getting the impression that the function may wrongly not get called when "iommu=off" but interrupt remapping is in use: We'd not get the interrupt handler installed, and hence interrupt remapping related events would never get reported. (Same on VT-d, FTAOD.) For iov_supports_xt() the question is whether, like VT-d's intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap alone (in which case it would need to remain a check rather than getting converted to ASSERT()). --- v2: New. --- a/xen/drivers/passthrough/amd/iommu_intr.c +++ b/xen/drivers/passthrough/amd/iommu_intr.c @@ -731,8 +731,7 @@ bool __init iov_supports_xt(void) { unsigned int apic; - if ( !iommu_enable || !iommu_intremap ) - return false; + ASSERT(iommu_enable || iommu_intremap); if ( amd_iommu_prepare(true) ) return false; --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c @@ -199,8 +199,7 @@ int __init acpi_ivrs_init(void) static int __init iov_detect(void) { - if ( !iommu_enable && !iommu_intremap ) - return 0; + ASSERT(iommu_enable || iommu_intremap); if ( (init_done ? amd_iommu_init_late() : amd_iommu_init(false)) != 0 ) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap 2021-10-21 9:59 ` [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap Jan Beulich @ 2021-10-25 10:28 ` Roger Pau Monné 2021-11-02 10:13 ` Jan Beulich 0 siblings, 1 reply; 24+ messages in thread From: Roger Pau Monné @ 2021-10-25 10:28 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, Paul Durrant On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote: > The two are really meant to be independent settings; iov_supports_xt() > using || instead of && was simply wrong. The corrected check is, > however, redundant, just like the (correct) one in iov_detect(): These > hook functions are unreachable without acpi_ivrs_init() installing the > iommu_init_ops pointer, which it does only upon success. (Unlike for > VT-d there is no late clearing of iommu_enable due to quirks, and any > possible clearing of iommu_intremap happens only after iov_supports_xt() > has run.) > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > In fact in iov_detect() it could be iommu_enable alone which gets > checked, but this felt overly aggressive to me. Instead I'm getting the > impression that the function may wrongly not get called when "iommu=off" > but interrupt remapping is in use: We'd not get the interrupt handler > installed, and hence interrupt remapping related events would never get > reported. (Same on VT-d, FTAOD.) I've spend a non-trivial amount of time looking into this before reading this note. AFAICT you could set iommu=off and still get x2APIC enabled and relying on interrupt remapping. > For iov_supports_xt() the question is whether, like VT-d's > intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap > alone (in which case it would need to remain a check rather than getting > converted to ASSERT()). Hm, no, I don't think so. I think iommu_enable should take precedence over iommu_intremap, and having iommu_enable == false should force interrupt remapping to be reported as disabled. Note that disabling it in iommu_setup is too late, as the APIC initialization will have already taken place. It's my reading of the command line parameter documentation that setting iommu=off should disable all usage of the IOMMU, and that includes the interrupt remapping support (ie: a user should not need to set iommu=off,no-intremap) > --- > v2: New. > > --- a/xen/drivers/passthrough/amd/iommu_intr.c > +++ b/xen/drivers/passthrough/amd/iommu_intr.c > @@ -731,8 +731,7 @@ bool __init iov_supports_xt(void) > { > unsigned int apic; > > - if ( !iommu_enable || !iommu_intremap ) > - return false; > + ASSERT(iommu_enable || iommu_intremap); I think this should be && in order to match my comments above. Thanks, Roger. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap 2021-10-25 10:28 ` Roger Pau Monné @ 2021-11-02 10:13 ` Jan Beulich 2021-11-02 11:03 ` Roger Pau Monné 0 siblings, 1 reply; 24+ messages in thread From: Jan Beulich @ 2021-11-02 10:13 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu, Paul Durrant On 25.10.2021 12:28, Roger Pau Monné wrote: > On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote: >> The two are really meant to be independent settings; iov_supports_xt() >> using || instead of && was simply wrong. The corrected check is, >> however, redundant, just like the (correct) one in iov_detect(): These >> hook functions are unreachable without acpi_ivrs_init() installing the >> iommu_init_ops pointer, which it does only upon success. (Unlike for >> VT-d there is no late clearing of iommu_enable due to quirks, and any >> possible clearing of iommu_intremap happens only after iov_supports_xt() >> has run.) >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> In fact in iov_detect() it could be iommu_enable alone which gets >> checked, but this felt overly aggressive to me. Instead I'm getting the >> impression that the function may wrongly not get called when "iommu=off" >> but interrupt remapping is in use: We'd not get the interrupt handler >> installed, and hence interrupt remapping related events would never get >> reported. (Same on VT-d, FTAOD.) > > I've spend a non-trivial amount of time looking into this before > reading this note. AFAICT you could set iommu=off and still get x2APIC > enabled and relying on interrupt remapping. Right, contrary to ... >> For iov_supports_xt() the question is whether, like VT-d's >> intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap >> alone (in which case it would need to remain a check rather than getting >> converted to ASSERT()). > > Hm, no, I don't think so. I think iommu_enable should take precedence > over iommu_intremap, and having iommu_enable == false should force > interrupt remapping to be reported as disabled. Note that disabling it > in iommu_setup is too late, as the APIC initialization will have > already taken place. > > It's my reading of the command line parameter documentation that > setting iommu=off should disable all usage of the IOMMU, and that > includes the interrupt remapping support (ie: a user should not need > to set iommu=off,no-intremap) ... that documentation. But I think it's the documentation that wants fixing, such that iommu=off really only control DMA remap. With that ... >> --- >> v2: New. >> >> --- a/xen/drivers/passthrough/amd/iommu_intr.c >> +++ b/xen/drivers/passthrough/amd/iommu_intr.c >> @@ -731,8 +731,7 @@ bool __init iov_supports_xt(void) >> { >> unsigned int apic; >> >> - if ( !iommu_enable || !iommu_intremap ) >> - return false; >> + ASSERT(iommu_enable || iommu_intremap); > > I think this should be && in order to match my comments above. ... I think || is correct to use here. Jan ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap 2021-11-02 10:13 ` Jan Beulich @ 2021-11-02 11:03 ` Roger Pau Monné 2021-11-02 14:00 ` Jan Beulich 2021-11-03 9:46 ` Jan Beulich 0 siblings, 2 replies; 24+ messages in thread From: Roger Pau Monné @ 2021-11-02 11:03 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, Paul Durrant On Tue, Nov 02, 2021 at 11:13:08AM +0100, Jan Beulich wrote: > On 25.10.2021 12:28, Roger Pau Monné wrote: > > On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote: > >> The two are really meant to be independent settings; iov_supports_xt() > >> using || instead of && was simply wrong. The corrected check is, > >> however, redundant, just like the (correct) one in iov_detect(): These > >> hook functions are unreachable without acpi_ivrs_init() installing the > >> iommu_init_ops pointer, which it does only upon success. (Unlike for > >> VT-d there is no late clearing of iommu_enable due to quirks, and any > >> possible clearing of iommu_intremap happens only after iov_supports_xt() > >> has run.) > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> --- > >> In fact in iov_detect() it could be iommu_enable alone which gets > >> checked, but this felt overly aggressive to me. Instead I'm getting the > >> impression that the function may wrongly not get called when "iommu=off" > >> but interrupt remapping is in use: We'd not get the interrupt handler > >> installed, and hence interrupt remapping related events would never get > >> reported. (Same on VT-d, FTAOD.) > > > > I've spend a non-trivial amount of time looking into this before > > reading this note. AFAICT you could set iommu=off and still get x2APIC > > enabled and relying on interrupt remapping. > > Right, contrary to ... > > >> For iov_supports_xt() the question is whether, like VT-d's > >> intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap > >> alone (in which case it would need to remain a check rather than getting > >> converted to ASSERT()). > > > > Hm, no, I don't think so. I think iommu_enable should take precedence > > over iommu_intremap, and having iommu_enable == false should force > > interrupt remapping to be reported as disabled. Note that disabling it > > in iommu_setup is too late, as the APIC initialization will have > > already taken place. > > > > It's my reading of the command line parameter documentation that > > setting iommu=off should disable all usage of the IOMMU, and that > > includes the interrupt remapping support (ie: a user should not need > > to set iommu=off,no-intremap) > > ... that documentation. But I think it's the documentation that > wants fixing, such that iommu=off really only control DMA remap. IMO I think it's confusing to have sub-options that could be enabled when you set the global one to off. I would expect `iommu=off` to disable all the iommu related options, and I think it's fair for people to expect that behavior. I'm unsure whether it's fair to change the documentation now, we should instead fix the code, so that people using `iommu=off` get the expected behavior. Then we would likely need to introduce a way to disable just dma remapping (dmaremap, similar to intremap). That would make a much better and saner interface IMO. Thanks, Roger. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap 2021-11-02 11:03 ` Roger Pau Monné @ 2021-11-02 14:00 ` Jan Beulich 2021-11-02 14:59 ` Roger Pau Monné 2021-11-03 9:46 ` Jan Beulich 1 sibling, 1 reply; 24+ messages in thread From: Jan Beulich @ 2021-11-02 14:00 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu, Paul Durrant On 02.11.2021 12:03, Roger Pau Monné wrote: > On Tue, Nov 02, 2021 at 11:13:08AM +0100, Jan Beulich wrote: >> On 25.10.2021 12:28, Roger Pau Monné wrote: >>> On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote: >>>> The two are really meant to be independent settings; iov_supports_xt() >>>> using || instead of && was simply wrong. The corrected check is, >>>> however, redundant, just like the (correct) one in iov_detect(): These >>>> hook functions are unreachable without acpi_ivrs_init() installing the >>>> iommu_init_ops pointer, which it does only upon success. (Unlike for >>>> VT-d there is no late clearing of iommu_enable due to quirks, and any >>>> possible clearing of iommu_intremap happens only after iov_supports_xt() >>>> has run.) >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> --- >>>> In fact in iov_detect() it could be iommu_enable alone which gets >>>> checked, but this felt overly aggressive to me. Instead I'm getting the >>>> impression that the function may wrongly not get called when "iommu=off" >>>> but interrupt remapping is in use: We'd not get the interrupt handler >>>> installed, and hence interrupt remapping related events would never get >>>> reported. (Same on VT-d, FTAOD.) >>> >>> I've spend a non-trivial amount of time looking into this before >>> reading this note. AFAICT you could set iommu=off and still get x2APIC >>> enabled and relying on interrupt remapping. >> >> Right, contrary to ... >> >>>> For iov_supports_xt() the question is whether, like VT-d's >>>> intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap >>>> alone (in which case it would need to remain a check rather than getting >>>> converted to ASSERT()). >>> >>> Hm, no, I don't think so. I think iommu_enable should take precedence >>> over iommu_intremap, and having iommu_enable == false should force >>> interrupt remapping to be reported as disabled. Note that disabling it >>> in iommu_setup is too late, as the APIC initialization will have >>> already taken place. >>> >>> It's my reading of the command line parameter documentation that >>> setting iommu=off should disable all usage of the IOMMU, and that >>> includes the interrupt remapping support (ie: a user should not need >>> to set iommu=off,no-intremap) >> >> ... that documentation. But I think it's the documentation that >> wants fixing, such that iommu=off really only control DMA remap. > > IMO I think it's confusing to have sub-options that could be enabled > when you set the global one to off. I would expect `iommu=off` to > disable all the iommu related options, and I think it's fair for > people to expect that behavior. > > I'm unsure whether it's fair to change the documentation now, we > should instead fix the code, so that people using `iommu=off` get the > expected behavior. Then we would likely need to introduce a way to > disable just dma remapping (dmaremap, similar to intremap). That > would make a much better and saner interface IMO. But from an x2APIC perspective it is a problem to have "iommu=off" also turn off intremap. And indeed the option has never (fully) worked that way: It clears iommu_enable, but not iommu_intremap (nor any of the other sub-options, but there it's less of a problem because they're not used in isolation), and iommu_intremap only may have happened to either get turned off later or to not get evaluated in at least some of the case. Jan ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap 2021-11-02 14:00 ` Jan Beulich @ 2021-11-02 14:59 ` Roger Pau Monné 0 siblings, 0 replies; 24+ messages in thread From: Roger Pau Monné @ 2021-11-02 14:59 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, Paul Durrant On Tue, Nov 02, 2021 at 03:00:24PM +0100, Jan Beulich wrote: > On 02.11.2021 12:03, Roger Pau Monné wrote: > > On Tue, Nov 02, 2021 at 11:13:08AM +0100, Jan Beulich wrote: > >> On 25.10.2021 12:28, Roger Pau Monné wrote: > >>> On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote: > >>>> The two are really meant to be independent settings; iov_supports_xt() > >>>> using || instead of && was simply wrong. The corrected check is, > >>>> however, redundant, just like the (correct) one in iov_detect(): These > >>>> hook functions are unreachable without acpi_ivrs_init() installing the > >>>> iommu_init_ops pointer, which it does only upon success. (Unlike for > >>>> VT-d there is no late clearing of iommu_enable due to quirks, and any > >>>> possible clearing of iommu_intremap happens only after iov_supports_xt() > >>>> has run.) > >>>> > >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >>>> --- > >>>> In fact in iov_detect() it could be iommu_enable alone which gets > >>>> checked, but this felt overly aggressive to me. Instead I'm getting the > >>>> impression that the function may wrongly not get called when "iommu=off" > >>>> but interrupt remapping is in use: We'd not get the interrupt handler > >>>> installed, and hence interrupt remapping related events would never get > >>>> reported. (Same on VT-d, FTAOD.) > >>> > >>> I've spend a non-trivial amount of time looking into this before > >>> reading this note. AFAICT you could set iommu=off and still get x2APIC > >>> enabled and relying on interrupt remapping. > >> > >> Right, contrary to ... > >> > >>>> For iov_supports_xt() the question is whether, like VT-d's > >>>> intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap > >>>> alone (in which case it would need to remain a check rather than getting > >>>> converted to ASSERT()). > >>> > >>> Hm, no, I don't think so. I think iommu_enable should take precedence > >>> over iommu_intremap, and having iommu_enable == false should force > >>> interrupt remapping to be reported as disabled. Note that disabling it > >>> in iommu_setup is too late, as the APIC initialization will have > >>> already taken place. > >>> > >>> It's my reading of the command line parameter documentation that > >>> setting iommu=off should disable all usage of the IOMMU, and that > >>> includes the interrupt remapping support (ie: a user should not need > >>> to set iommu=off,no-intremap) > >> > >> ... that documentation. But I think it's the documentation that > >> wants fixing, such that iommu=off really only control DMA remap. > > > > IMO I think it's confusing to have sub-options that could be enabled > > when you set the global one to off. I would expect `iommu=off` to > > disable all the iommu related options, and I think it's fair for > > people to expect that behavior. > > > > I'm unsure whether it's fair to change the documentation now, we > > should instead fix the code, so that people using `iommu=off` get the > > expected behavior. Then we would likely need to introduce a way to > > disable just dma remapping (dmaremap, similar to intremap). That > > would make a much better and saner interface IMO. > > But from an x2APIC perspective it is a problem to have "iommu=off" > also turn off intremap. I think we could log a message in that case? (x2APIC could be enabled but iommu explicitly disabled) And maybe expand the documentation to notice that disabling the iommu or interrupt remapping will disable x2APIC support. > And indeed the option has never (fully) > worked that way: It clears iommu_enable, but not iommu_intremap > (nor any of the other sub-options, but there it's less of a problem > because they're not used in isolation), and iommu_intremap only > may have happened to either get turned off later or to not get > evaluated in at least some of the case. While I understand there's some baggage here, I'm not sure keeping the current behavior is correct. I would rather have iommu=off to cover all iommu functionality, and then we should add dmaremap sub-option to disable remapping only. I think that would be a sane and logic interface for users to understand. We should also note the implications of disabling interrupt remapping regarding x2APIC support in the documentation. Thanks, Roger. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap 2021-11-02 11:03 ` Roger Pau Monné 2021-11-02 14:00 ` Jan Beulich @ 2021-11-03 9:46 ` Jan Beulich 2021-11-03 15:06 ` Roger Pau Monné 1 sibling, 1 reply; 24+ messages in thread From: Jan Beulich @ 2021-11-03 9:46 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu, Paul Durrant On 02.11.2021 12:03, Roger Pau Monné wrote: > On Tue, Nov 02, 2021 at 11:13:08AM +0100, Jan Beulich wrote: >> On 25.10.2021 12:28, Roger Pau Monné wrote: >>> On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote: >>>> The two are really meant to be independent settings; iov_supports_xt() >>>> using || instead of && was simply wrong. The corrected check is, >>>> however, redundant, just like the (correct) one in iov_detect(): These >>>> hook functions are unreachable without acpi_ivrs_init() installing the >>>> iommu_init_ops pointer, which it does only upon success. (Unlike for >>>> VT-d there is no late clearing of iommu_enable due to quirks, and any >>>> possible clearing of iommu_intremap happens only after iov_supports_xt() >>>> has run.) >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> --- >>>> In fact in iov_detect() it could be iommu_enable alone which gets >>>> checked, but this felt overly aggressive to me. Instead I'm getting the >>>> impression that the function may wrongly not get called when "iommu=off" >>>> but interrupt remapping is in use: We'd not get the interrupt handler >>>> installed, and hence interrupt remapping related events would never get >>>> reported. (Same on VT-d, FTAOD.) >>> >>> I've spend a non-trivial amount of time looking into this before >>> reading this note. AFAICT you could set iommu=off and still get x2APIC >>> enabled and relying on interrupt remapping. >> >> Right, contrary to ... >> >>>> For iov_supports_xt() the question is whether, like VT-d's >>>> intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap >>>> alone (in which case it would need to remain a check rather than getting >>>> converted to ASSERT()). >>> >>> Hm, no, I don't think so. I think iommu_enable should take precedence >>> over iommu_intremap, and having iommu_enable == false should force >>> interrupt remapping to be reported as disabled. Note that disabling it >>> in iommu_setup is too late, as the APIC initialization will have >>> already taken place. >>> >>> It's my reading of the command line parameter documentation that >>> setting iommu=off should disable all usage of the IOMMU, and that >>> includes the interrupt remapping support (ie: a user should not need >>> to set iommu=off,no-intremap) >> >> ... that documentation. But I think it's the documentation that >> wants fixing, such that iommu=off really only control DMA remap. > > IMO I think it's confusing to have sub-options that could be enabled > when you set the global one to off. I would expect `iommu=off` to > disable all the iommu related options, and I think it's fair for > people to expect that behavior. It occurs to me that this reply of yours here contradicts your R-b on patch 1, in particular with its revision log saying: v2: Treat iommu_enable and iommu_intremap as separate options. Even in case I receive a release ack from Ian, I'll try to remember to hold off committing that until this apparent (to me) confusion was sorted. Jan ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap 2021-11-03 9:46 ` Jan Beulich @ 2021-11-03 15:06 ` Roger Pau Monné 2021-11-03 15:15 ` Jan Beulich 0 siblings, 1 reply; 24+ messages in thread From: Roger Pau Monné @ 2021-11-03 15:06 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, Paul Durrant On Wed, Nov 03, 2021 at 10:46:40AM +0100, Jan Beulich wrote: > On 02.11.2021 12:03, Roger Pau Monné wrote: > > On Tue, Nov 02, 2021 at 11:13:08AM +0100, Jan Beulich wrote: > >> On 25.10.2021 12:28, Roger Pau Monné wrote: > >>> On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote: > >>>> The two are really meant to be independent settings; iov_supports_xt() > >>>> using || instead of && was simply wrong. The corrected check is, > >>>> however, redundant, just like the (correct) one in iov_detect(): These > >>>> hook functions are unreachable without acpi_ivrs_init() installing the > >>>> iommu_init_ops pointer, which it does only upon success. (Unlike for > >>>> VT-d there is no late clearing of iommu_enable due to quirks, and any > >>>> possible clearing of iommu_intremap happens only after iov_supports_xt() > >>>> has run.) > >>>> > >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >>>> --- > >>>> In fact in iov_detect() it could be iommu_enable alone which gets > >>>> checked, but this felt overly aggressive to me. Instead I'm getting the > >>>> impression that the function may wrongly not get called when "iommu=off" > >>>> but interrupt remapping is in use: We'd not get the interrupt handler > >>>> installed, and hence interrupt remapping related events would never get > >>>> reported. (Same on VT-d, FTAOD.) > >>> > >>> I've spend a non-trivial amount of time looking into this before > >>> reading this note. AFAICT you could set iommu=off and still get x2APIC > >>> enabled and relying on interrupt remapping. > >> > >> Right, contrary to ... > >> > >>>> For iov_supports_xt() the question is whether, like VT-d's > >>>> intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap > >>>> alone (in which case it would need to remain a check rather than getting > >>>> converted to ASSERT()). > >>> > >>> Hm, no, I don't think so. I think iommu_enable should take precedence > >>> over iommu_intremap, and having iommu_enable == false should force > >>> interrupt remapping to be reported as disabled. Note that disabling it > >>> in iommu_setup is too late, as the APIC initialization will have > >>> already taken place. > >>> > >>> It's my reading of the command line parameter documentation that > >>> setting iommu=off should disable all usage of the IOMMU, and that > >>> includes the interrupt remapping support (ie: a user should not need > >>> to set iommu=off,no-intremap) > >> > >> ... that documentation. But I think it's the documentation that > >> wants fixing, such that iommu=off really only control DMA remap. > > > > IMO I think it's confusing to have sub-options that could be enabled > > when you set the global one to off. I would expect `iommu=off` to > > disable all the iommu related options, and I think it's fair for > > people to expect that behavior. > > It occurs to me that this reply of yours here contradicts your R-b > on patch 1, in particular with its revision log saying: > > v2: Treat iommu_enable and iommu_intremap as separate options. Right, I see. patch 1 uses if ( !iommu_enable && !iommu_intremap ) return; Which I think should be: if ( !iommu_enable ) return; Sorry I didn't realize in that context. I think we need to decide whether we want to fix the documentation to match the code, or whether we should fix the code to match the documentation. My preference would be for the latter, because I think the resulting interface would be clearer. That will require introducing a new dmaremap iommu suboption, but again I think this will result in a better interface overall. Thanks, Roger. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap 2021-11-03 15:06 ` Roger Pau Monné @ 2021-11-03 15:15 ` Jan Beulich 2021-11-03 17:00 ` Roger Pau Monné 0 siblings, 1 reply; 24+ messages in thread From: Jan Beulich @ 2021-11-03 15:15 UTC (permalink / raw) To: Roger Pau Monné, Paul Durrant; +Cc: xen-devel, Andrew Cooper, Wei Liu On 03.11.2021 16:06, Roger Pau Monné wrote: > On Wed, Nov 03, 2021 at 10:46:40AM +0100, Jan Beulich wrote: >> On 02.11.2021 12:03, Roger Pau Monné wrote: >>> On Tue, Nov 02, 2021 at 11:13:08AM +0100, Jan Beulich wrote: >>>> On 25.10.2021 12:28, Roger Pau Monné wrote: >>>>> On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote: >>>>>> The two are really meant to be independent settings; iov_supports_xt() >>>>>> using || instead of && was simply wrong. The corrected check is, >>>>>> however, redundant, just like the (correct) one in iov_detect(): These >>>>>> hook functions are unreachable without acpi_ivrs_init() installing the >>>>>> iommu_init_ops pointer, which it does only upon success. (Unlike for >>>>>> VT-d there is no late clearing of iommu_enable due to quirks, and any >>>>>> possible clearing of iommu_intremap happens only after iov_supports_xt() >>>>>> has run.) >>>>>> >>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>>> --- >>>>>> In fact in iov_detect() it could be iommu_enable alone which gets >>>>>> checked, but this felt overly aggressive to me. Instead I'm getting the >>>>>> impression that the function may wrongly not get called when "iommu=off" >>>>>> but interrupt remapping is in use: We'd not get the interrupt handler >>>>>> installed, and hence interrupt remapping related events would never get >>>>>> reported. (Same on VT-d, FTAOD.) >>>>> >>>>> I've spend a non-trivial amount of time looking into this before >>>>> reading this note. AFAICT you could set iommu=off and still get x2APIC >>>>> enabled and relying on interrupt remapping. >>>> >>>> Right, contrary to ... >>>> >>>>>> For iov_supports_xt() the question is whether, like VT-d's >>>>>> intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap >>>>>> alone (in which case it would need to remain a check rather than getting >>>>>> converted to ASSERT()). >>>>> >>>>> Hm, no, I don't think so. I think iommu_enable should take precedence >>>>> over iommu_intremap, and having iommu_enable == false should force >>>>> interrupt remapping to be reported as disabled. Note that disabling it >>>>> in iommu_setup is too late, as the APIC initialization will have >>>>> already taken place. >>>>> >>>>> It's my reading of the command line parameter documentation that >>>>> setting iommu=off should disable all usage of the IOMMU, and that >>>>> includes the interrupt remapping support (ie: a user should not need >>>>> to set iommu=off,no-intremap) >>>> >>>> ... that documentation. But I think it's the documentation that >>>> wants fixing, such that iommu=off really only control DMA remap. >>> >>> IMO I think it's confusing to have sub-options that could be enabled >>> when you set the global one to off. I would expect `iommu=off` to >>> disable all the iommu related options, and I think it's fair for >>> people to expect that behavior. >> >> It occurs to me that this reply of yours here contradicts your R-b >> on patch 1, in particular with its revision log saying: >> >> v2: Treat iommu_enable and iommu_intremap as separate options. > > Right, I see. patch 1 uses > > if ( !iommu_enable && !iommu_intremap ) > return; > > Which I think should be: > > if ( !iommu_enable ) > return; > > Sorry I didn't realize in that context. I think we need to decide > whether we want to fix the documentation to match the code, or whether > we should fix the code to match the documentation. Except that adjusting the conditional(s) in patch 1 would then be a functional change that's not really the purpose of that patch - it really only folds acpi_ivrs_init()'s and acpi_parse_dmar()'s into a vendor-independent instance in acpi_iommu_init(). Alternatively we could adjust the conditional here (in patch 3), but that would feel unrelated once again, as this change is supposed to be AMD-specific. > My preference would be for the latter, because I think the resulting > interface would be clearer. That will require introducing a new > dmaremap iommu suboption, but again I think this will result in a > better interface overall. I guess we could do with a 3rd opinion: Paul, any chance? In any event I hope that we can agree that patches 1 and 2 are okay for 4.16 in their present shape, and patch 3 (plus whichever further ones) would better wait for post-4.16? Jan ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap 2021-11-03 15:15 ` Jan Beulich @ 2021-11-03 17:00 ` Roger Pau Monné 0 siblings, 0 replies; 24+ messages in thread From: Roger Pau Monné @ 2021-11-03 17:00 UTC (permalink / raw) To: Jan Beulich; +Cc: Paul Durrant, xen-devel, Andrew Cooper, Wei Liu On Wed, Nov 03, 2021 at 04:15:52PM +0100, Jan Beulich wrote: > On 03.11.2021 16:06, Roger Pau Monné wrote: > > On Wed, Nov 03, 2021 at 10:46:40AM +0100, Jan Beulich wrote: > >> On 02.11.2021 12:03, Roger Pau Monné wrote: > >>> On Tue, Nov 02, 2021 at 11:13:08AM +0100, Jan Beulich wrote: > >>>> On 25.10.2021 12:28, Roger Pau Monné wrote: > >>>>> On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote: > >>>>>> The two are really meant to be independent settings; iov_supports_xt() > >>>>>> using || instead of && was simply wrong. The corrected check is, > >>>>>> however, redundant, just like the (correct) one in iov_detect(): These > >>>>>> hook functions are unreachable without acpi_ivrs_init() installing the > >>>>>> iommu_init_ops pointer, which it does only upon success. (Unlike for > >>>>>> VT-d there is no late clearing of iommu_enable due to quirks, and any > >>>>>> possible clearing of iommu_intremap happens only after iov_supports_xt() > >>>>>> has run.) > >>>>>> > >>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >>>>>> --- > >>>>>> In fact in iov_detect() it could be iommu_enable alone which gets > >>>>>> checked, but this felt overly aggressive to me. Instead I'm getting the > >>>>>> impression that the function may wrongly not get called when "iommu=off" > >>>>>> but interrupt remapping is in use: We'd not get the interrupt handler > >>>>>> installed, and hence interrupt remapping related events would never get > >>>>>> reported. (Same on VT-d, FTAOD.) > >>>>> > >>>>> I've spend a non-trivial amount of time looking into this before > >>>>> reading this note. AFAICT you could set iommu=off and still get x2APIC > >>>>> enabled and relying on interrupt remapping. > >>>> > >>>> Right, contrary to ... > >>>> > >>>>>> For iov_supports_xt() the question is whether, like VT-d's > >>>>>> intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap > >>>>>> alone (in which case it would need to remain a check rather than getting > >>>>>> converted to ASSERT()). > >>>>> > >>>>> Hm, no, I don't think so. I think iommu_enable should take precedence > >>>>> over iommu_intremap, and having iommu_enable == false should force > >>>>> interrupt remapping to be reported as disabled. Note that disabling it > >>>>> in iommu_setup is too late, as the APIC initialization will have > >>>>> already taken place. > >>>>> > >>>>> It's my reading of the command line parameter documentation that > >>>>> setting iommu=off should disable all usage of the IOMMU, and that > >>>>> includes the interrupt remapping support (ie: a user should not need > >>>>> to set iommu=off,no-intremap) > >>>> > >>>> ... that documentation. But I think it's the documentation that > >>>> wants fixing, such that iommu=off really only control DMA remap. > >>> > >>> IMO I think it's confusing to have sub-options that could be enabled > >>> when you set the global one to off. I would expect `iommu=off` to > >>> disable all the iommu related options, and I think it's fair for > >>> people to expect that behavior. > >> > >> It occurs to me that this reply of yours here contradicts your R-b > >> on patch 1, in particular with its revision log saying: > >> > >> v2: Treat iommu_enable and iommu_intremap as separate options. > > > > Right, I see. patch 1 uses > > > > if ( !iommu_enable && !iommu_intremap ) > > return; > > > > Which I think should be: > > > > if ( !iommu_enable ) > > return; > > > > Sorry I didn't realize in that context. I think we need to decide > > whether we want to fix the documentation to match the code, or whether > > we should fix the code to match the documentation. > > Except that adjusting the conditional(s) in patch 1 would then > be a functional change that's not really the purpose of that > patch - it really only folds acpi_ivrs_init()'s and > acpi_parse_dmar()'s into a vendor-independent instance in > acpi_iommu_init(). Right. > Alternatively we could adjust the conditional > here (in patch 3), but that would feel unrelated once again, as > this change is supposed to be AMD-specific. Depending on what we end up doing regarding interrupt remapping being disabled with iommu=off we might want to rework patch 3. > > My preference would be for the latter, because I think the resulting > > interface would be clearer. That will require introducing a new > > dmaremap iommu suboption, but again I think this will result in a > > better interface overall. > > I guess we could do with a 3rd opinion: Paul, any chance? > > In any event I hope that we can agree that patches 1 and 2 are > okay for 4.16 in their present shape, and patch 3 (plus whichever > further ones) would better wait for post-4.16? I consider the issues either a bug in the documentation or the code, so it's likely I would suggest whatever fix we end up doing to be backported. At which point it might make sense to add to the release. I don't think it should be a blocked though, as this hasn't been introduced in this release. Thanks, Roger. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/3] x86/IOMMU: enabled / intremap handling 2021-10-21 9:57 [PATCH v2 0/3] x86/IOMMU: enabled / intremap handling Jan Beulich ` (2 preceding siblings ...) 2021-10-21 9:59 ` [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap Jan Beulich @ 2021-11-02 10:17 ` Jan Beulich 2021-11-03 11:01 ` [PATCH v2 0/3][4.16] " Jan Beulich 3 siblings, 1 reply; 24+ messages in thread From: Jan Beulich @ 2021-11-02 10:17 UTC (permalink / raw) To: Ian Jackson, Andrew Cooper Cc: Wei Liu, Roger Pau Monné, Paul Durrant, xen-devel On 21.10.2021 11:57, Jan Beulich wrote: > In the course of reading the response to v1 (patch 1 only) I realized > that not only that patch needs further adjustment, but that also > further changes are needed (and there's likely yet more amiss). > > 1: x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing > 2: x86/APIC: avoid iommu_supports_x2apic() on error path > 3: AMD/IOMMU: iommu_enable vs iommu_intremap Ian, while we further discuss / refine patch 3, the first two have the needed R-b, but will now need you release-ack aiui. Andrew, did you perhaps have a chance to actually try v2 of patch 1? It works for me when suitably configuring the BIOS on my Skylake, so I wouldn't feel uncertain in committing without a Tested-by, but it would feel even better if I had one. Thanks, Jan ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/3][4.16] x86/IOMMU: enabled / intremap handling 2021-11-02 10:17 ` [PATCH v2 0/3] x86/IOMMU: enabled / intremap handling Jan Beulich @ 2021-11-03 11:01 ` Jan Beulich 2021-11-03 16:19 ` Ian Jackson 0 siblings, 1 reply; 24+ messages in thread From: Jan Beulich @ 2021-11-03 11:01 UTC (permalink / raw) To: Ian Jackson Cc: Wei Liu, Roger Pau Monné, Paul Durrant, xen-devel, Andrew Cooper On 02.11.2021 11:17, Jan Beulich wrote: > On 21.10.2021 11:57, Jan Beulich wrote: >> In the course of reading the response to v1 (patch 1 only) I realized >> that not only that patch needs further adjustment, but that also >> further changes are needed (and there's likely yet more amiss). >> >> 1: x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing >> 2: x86/APIC: avoid iommu_supports_x2apic() on error path >> 3: AMD/IOMMU: iommu_enable vs iommu_intremap > > Ian, while we further discuss / refine patch 3, the first two have the > needed R-b, but will now need you release-ack aiui. Seeing your reply on IRC, here an attempt at a release justification (the patches were ready by Oct 29, but no-one cared to commit them in my absence, so I thought I'd get away without such a write-up): Patch 1 addresses a regression identified by Andrew. The main risk I see here (which has turned up only very recently) is disagreement on patch 3 which imo has an effect also on what patch 1 does, as to the (non-)effects of "iommu=off" on the hypervisor command line. This, however, is not an effect of the patch, but pre-existing behavior. The behavioral change (in this regard) is in patch 3, which is still under discussion. Patch 2 corrects an (unlikely but not impossible to be taken) error path, supposedly making systems functional again in case they would in fact cause that error path to be taken. The risk looks low to me, given that two function calls with previously assumed to be identical results now get folded into one with the result latched. Jan > Andrew, did you perhaps have a chance to actually try v2 of patch 1? It > works for me when suitably configuring the BIOS on my Skylake, so I > wouldn't feel uncertain in committing without a Tested-by, but it would > feel even better if I had one. > > Thanks, Jan > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/3][4.16] x86/IOMMU: enabled / intremap handling 2021-11-03 11:01 ` [PATCH v2 0/3][4.16] " Jan Beulich @ 2021-11-03 16:19 ` Ian Jackson 2021-11-04 8:15 ` Jan Beulich 0 siblings, 1 reply; 24+ messages in thread From: Ian Jackson @ 2021-11-03 16:19 UTC (permalink / raw) To: Jan Beulich Cc: Wei Liu, Roger Pau Monné, Paul Durrant, xen-devel, Andrew Cooper Jan Beulich writes ("Re: [PATCH v2 0/3][4.16] x86/IOMMU: enabled / intremap handling"): > On 02.11.2021 11:17, Jan Beulich wrote: > > On 21.10.2021 11:57, Jan Beulich wrote: > >> In the course of reading the response to v1 (patch 1 only) I realized > >> that not only that patch needs further adjustment, but that also > >> further changes are needed (and there's likely yet more amiss). > >> > >> 1: x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing > >> 2: x86/APIC: avoid iommu_supports_x2apic() on error path > >> 3: AMD/IOMMU: iommu_enable vs iommu_intremap > > > > Ian, while we further discuss / refine patch 3, the first two have the > > needed R-b, but will now need you release-ack aiui. > > Seeing your reply on IRC, here an attempt at a release justification > (the patches were ready by Oct 29, but no-one cared to commit them > in my absence, so I thought I'd get away without such a write-up): > > Patch 1 addresses a regression identified by Andrew. The main risk I > see here (which has turned up only very recently) is disagreement on > patch 3 which imo has an effect also on what patch 1 does, as to the > (non-)effects of "iommu=off" on the hypervisor command line. This, > however, is not an effect of the patch, but pre-existing behavior. > The behavioral change (in this regard) is in patch 3, which is still > under discussion. Thank you. I also went to the list and read the thread there. Patch 1: Reviewed-by: Ian Jackson <iwj@xenproject.org> > Patch 2 corrects an (unlikely but not impossible to be taken) error > path, supposedly making systems functional again in case they would > in fact cause that error path to be taken. The risk looks low to me, > given that two function calls with previously assumed to be > identical results now get folded into one with the result latched. This one also: Release-Acked-by: Ian Jackson <iwj@xenproject.org> I think, from reading the thread, that patch 3 is not targeting 4.16. Thanks, Ian. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/3][4.16] x86/IOMMU: enabled / intremap handling 2021-11-03 16:19 ` Ian Jackson @ 2021-11-04 8:15 ` Jan Beulich 2021-11-04 11:10 ` Ian Jackson 0 siblings, 1 reply; 24+ messages in thread From: Jan Beulich @ 2021-11-04 8:15 UTC (permalink / raw) To: Ian Jackson Cc: Wei Liu, Roger Pau Monné, Paul Durrant, xen-devel, Andrew Cooper On 03.11.2021 17:19, Ian Jackson wrote: > Jan Beulich writes ("Re: [PATCH v2 0/3][4.16] x86/IOMMU: enabled / intremap handling"): >> On 02.11.2021 11:17, Jan Beulich wrote: >>> On 21.10.2021 11:57, Jan Beulich wrote: >>>> In the course of reading the response to v1 (patch 1 only) I realized >>>> that not only that patch needs further adjustment, but that also >>>> further changes are needed (and there's likely yet more amiss). >>>> >>>> 1: x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing >>>> 2: x86/APIC: avoid iommu_supports_x2apic() on error path >>>> 3: AMD/IOMMU: iommu_enable vs iommu_intremap >>> >>> Ian, while we further discuss / refine patch 3, the first two have the >>> needed R-b, but will now need you release-ack aiui. >> >> Seeing your reply on IRC, here an attempt at a release justification >> (the patches were ready by Oct 29, but no-one cared to commit them >> in my absence, so I thought I'd get away without such a write-up): >> >> Patch 1 addresses a regression identified by Andrew. The main risk I >> see here (which has turned up only very recently) is disagreement on >> patch 3 which imo has an effect also on what patch 1 does, as to the >> (non-)effects of "iommu=off" on the hypervisor command line. This, >> however, is not an effect of the patch, but pre-existing behavior. >> The behavioral change (in this regard) is in patch 3, which is still >> under discussion. > > Thank you. I also went to the list and read the thread there. > > Patch 1: > > Reviewed-by: Ian Jackson <iwj@xenproject.org> > >> Patch 2 corrects an (unlikely but not impossible to be taken) error >> path, supposedly making systems functional again in case they would >> in fact cause that error path to be taken. The risk looks low to me, >> given that two function calls with previously assumed to be >> identical results now get folded into one with the result latched. > > This one also: > > Release-Acked-by: Ian Jackson <iwj@xenproject.org> Thanks, but your reply leaves me a little confused: Your use of "also" may mean R-b for both patches but R-a-b only for patch 2. But I could also find a variety of other interpretations, including that the first R-b really was meant to be R-a-b (which otherwise I'd need on top of the R-b anyway). Please clarify. > I think, from reading the thread, that patch 3 is not targeting 4.16. Correct. The other related one now targeting 4.16 is the separately submitted "x86/x2APIC: defer probe until after IOMMU ACPI table parsing". But I can see reasons for you to prefer to have that deferred. Jan ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 0/3][4.16] x86/IOMMU: enabled / intremap handling 2021-11-04 8:15 ` Jan Beulich @ 2021-11-04 11:10 ` Ian Jackson 0 siblings, 0 replies; 24+ messages in thread From: Ian Jackson @ 2021-11-04 11:10 UTC (permalink / raw) To: Jan Beulich Cc: Wei Liu, Roger Pau Monné, Paul Durrant, xen-devel, Andrew Cooper Jan Beulich writes ("Re: [PATCH v2 0/3][4.16] x86/IOMMU: enabled / intremap handling"): > > Reviewed-by: Ian Jackson <iwj@xenproject.org> > > > >> Patch 2 corrects an (unlikely but not impossible to be taken) error > >> path, supposedly making systems functional again in case they would > >> in fact cause that error path to be taken. The risk looks low to me, > >> given that two function calls with previously assumed to be > >> identical results now get folded into one with the result latched. > > > > This one also: > > > > Release-Acked-by: Ian Jackson <iwj@xenproject.org> > > Thanks, but your reply leaves me a little confused: Your use of "also" > may mean R-b for both patches but R-a-b only for patch 2. But I could > also find a variety of other interpretations, including that the > first R-b really was meant to be R-a-b (which otherwise I'd need on > top of the R-b anyway). Please clarify. Um. Well spotted. I meant release-acked-by for both. I botched the keystroke in my MUA. Sorry for the confusion. So FTAAD I have *not* "Reviewed" either patch (although I did read it, I don't consider myself qualified to give an R-b). > > I think, from reading the thread, that patch 3 is not targeting 4.16. > > Correct. The other related one now targeting 4.16 is the separately > submitted "x86/x2APIC: defer probe until after IOMMU ACPI table > parsing". But I can see reasons for you to prefer to have that > deferred. Thanks, Ian. ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2021-11-04 11:11 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-21 9:57 [PATCH v2 0/3] x86/IOMMU: enabled / intremap handling Jan Beulich 2021-10-21 9:58 ` [PATCH v2 1/3] x86/IOMMU: mark IOMMU / intremap not in use when ACPI tables are missing Jan Beulich 2021-10-22 5:59 ` Jan Beulich 2021-10-29 8:44 ` Tian, Kevin 2021-10-22 15:52 ` Roger Pau Monné 2021-11-02 10:07 ` Jan Beulich 2021-11-02 10:26 ` Roger Pau Monné 2021-10-21 9:58 ` [PATCH v2 2/3] x86/APIC: avoid iommu_supports_x2apic() on error path Jan Beulich 2021-10-25 8:41 ` Roger Pau Monné 2021-10-21 9:59 ` [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap Jan Beulich 2021-10-25 10:28 ` Roger Pau Monné 2021-11-02 10:13 ` Jan Beulich 2021-11-02 11:03 ` Roger Pau Monné 2021-11-02 14:00 ` Jan Beulich 2021-11-02 14:59 ` Roger Pau Monné 2021-11-03 9:46 ` Jan Beulich 2021-11-03 15:06 ` Roger Pau Monné 2021-11-03 15:15 ` Jan Beulich 2021-11-03 17:00 ` Roger Pau Monné 2021-11-02 10:17 ` [PATCH v2 0/3] x86/IOMMU: enabled / intremap handling Jan Beulich 2021-11-03 11:01 ` [PATCH v2 0/3][4.16] " Jan Beulich 2021-11-03 16:19 ` Ian Jackson 2021-11-04 8:15 ` Jan Beulich 2021-11-04 11:10 ` Ian Jackson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.