| Message ID | 20230104084502.61734-4-burzalodowa@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Make x86 IOMMU driver support configurable | expand |
On 04.01.2023 09:44, Xenia Ragiadakou wrote: > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -82,11 +82,13 @@ static int __init cf_check parse_iommu_param(const char *s) > else if ( ss == s + 23 && !strncmp(s, "quarantine=scratch-page", 23) ) > iommu_quarantine = IOMMU_quarantine_scratch_page; > #endif > -#ifdef CONFIG_X86 > +#ifdef CONFIG_INTEL_IOMMU > else if ( (val = parse_boolean("igfx", s, ss)) >= 0 ) > iommu_igfx = val; > else if ( (val = parse_boolean("qinval", s, ss)) >= 0 ) > iommu_qinval = val; > +#endif You want to use no_config_param() here as well then. > --- a/xen/include/xen/iommu.h > +++ b/xen/include/xen/iommu.h > @@ -74,9 +74,13 @@ extern enum __packed iommu_intremap { > iommu_intremap_restricted, > iommu_intremap_full, > } iommu_intremap; > -extern bool iommu_igfx, iommu_qinval, iommu_snoop; > #else > # define iommu_intremap false > +#endif > + > +#ifdef CONFIG_INTEL_IOMMU > +extern bool iommu_igfx, iommu_qinval, iommu_snoop; > +#else > # define iommu_snoop false > #endif Do these declarations really need touching? In patch 2 you didn't move amd_iommu_perdev_intremap's either. Jan
On 1/12/23 13:31, Jan Beulich wrote: > On 04.01.2023 09:44, Xenia Ragiadakou wrote: >> --- a/xen/drivers/passthrough/iommu.c >> +++ b/xen/drivers/passthrough/iommu.c >> @@ -82,11 +82,13 @@ static int __init cf_check parse_iommu_param(const char *s) >> else if ( ss == s + 23 && !strncmp(s, "quarantine=scratch-page", 23) ) >> iommu_quarantine = IOMMU_quarantine_scratch_page; >> #endif >> -#ifdef CONFIG_X86 >> +#ifdef CONFIG_INTEL_IOMMU >> else if ( (val = parse_boolean("igfx", s, ss)) >= 0 ) >> iommu_igfx = val; >> else if ( (val = parse_boolean("qinval", s, ss)) >= 0 ) >> iommu_qinval = val; >> +#endif > > You want to use no_config_param() here as well then. Yes. I will fix it. > >> --- a/xen/include/xen/iommu.h >> +++ b/xen/include/xen/iommu.h >> @@ -74,9 +74,13 @@ extern enum __packed iommu_intremap { >> iommu_intremap_restricted, >> iommu_intremap_full, >> } iommu_intremap; >> -extern bool iommu_igfx, iommu_qinval, iommu_snoop; >> #else >> # define iommu_intremap false >> +#endif >> + >> +#ifdef CONFIG_INTEL_IOMMU >> +extern bool iommu_igfx, iommu_qinval, iommu_snoop; >> +#else >> # define iommu_snoop false >> #endif > > Do these declarations really need touching? In patch 2 you didn't move > amd_iommu_perdev_intremap's either. Ok, I will revert this change (as I did in v2 of patch 2) since it is not needed.
On 1/12/23 13:49, Xenia Ragiadakou wrote: > > On 1/12/23 13:31, Jan Beulich wrote: >> On 04.01.2023 09:44, Xenia Ragiadakou wrote: >>> --- a/xen/drivers/passthrough/iommu.c >>> +++ b/xen/drivers/passthrough/iommu.c >>> @@ -82,11 +82,13 @@ static int __init cf_check >>> parse_iommu_param(const char *s) >>> else if ( ss == s + 23 && !strncmp(s, >>> "quarantine=scratch-page", 23) ) >>> iommu_quarantine = IOMMU_quarantine_scratch_page; >>> #endif >>> -#ifdef CONFIG_X86 >>> +#ifdef CONFIG_INTEL_IOMMU >>> else if ( (val = parse_boolean("igfx", s, ss)) >= 0 ) >>> iommu_igfx = val; >>> else if ( (val = parse_boolean("qinval", s, ss)) >= 0 ) >>> iommu_qinval = val; >>> +#endif >> >> You want to use no_config_param() here as well then. > > Yes. I will fix it. > >> >>> --- a/xen/include/xen/iommu.h >>> +++ b/xen/include/xen/iommu.h >>> @@ -74,9 +74,13 @@ extern enum __packed iommu_intremap { >>> iommu_intremap_restricted, >>> iommu_intremap_full, >>> } iommu_intremap; >>> -extern bool iommu_igfx, iommu_qinval, iommu_snoop; >>> #else >>> # define iommu_intremap false >>> +#endif >>> + >>> +#ifdef CONFIG_INTEL_IOMMU >>> +extern bool iommu_igfx, iommu_qinval, iommu_snoop; >>> +#else >>> # define iommu_snoop false >>> #endif >> >> Do these declarations really need touching? In patch 2 you didn't move >> amd_iommu_perdev_intremap's either. > > Ok, I will revert this change (as I did in v2 of patch 2) since it is > not needed. Actually, my patch was altering the current behavior by defining iommu_snoop as false when !INTEL_IOMMU. IIUC, there is no control over snoop behavior when using the AMD iommu. Hence, iommu_snoop should evaluate to true for AMD iommu. However, when using the INTEL iommu the user can disable it via the "iommu" param, right? If that's the case then iommu_snoop needs to be moved from vtd/iommu.c to x86/iommu.c and iommu_snoop assignment via iommu param needs to be guarded by CONFIG_INTEL_IOMMU.
On 12.01.2023 16:43, Xenia Ragiadakou wrote: > On 1/12/23 13:49, Xenia Ragiadakou wrote: >> On 1/12/23 13:31, Jan Beulich wrote: >>> On 04.01.2023 09:44, Xenia Ragiadakou wrote: >>>> --- a/xen/include/xen/iommu.h >>>> +++ b/xen/include/xen/iommu.h >>>> @@ -74,9 +74,13 @@ extern enum __packed iommu_intremap { >>>> iommu_intremap_restricted, >>>> iommu_intremap_full, >>>> } iommu_intremap; >>>> -extern bool iommu_igfx, iommu_qinval, iommu_snoop; >>>> #else >>>> # define iommu_intremap false >>>> +#endif >>>> + >>>> +#ifdef CONFIG_INTEL_IOMMU >>>> +extern bool iommu_igfx, iommu_qinval, iommu_snoop; >>>> +#else >>>> # define iommu_snoop false >>>> #endif >>> >>> Do these declarations really need touching? In patch 2 you didn't move >>> amd_iommu_perdev_intremap's either. >> >> Ok, I will revert this change (as I did in v2 of patch 2) since it is >> not needed. > > Actually, my patch was altering the current behavior by defining > iommu_snoop as false when !INTEL_IOMMU. > > IIUC, there is no control over snoop behavior when using the AMD iommu. > Hence, iommu_snoop should evaluate to true for AMD iommu. > However, when using the INTEL iommu the user can disable it via the > "iommu" param, right? That's the intended behavior, yes, but right now we allow the option to also affect behavior on AMD - perhaps wrongly so, as there's one use outside of VT-x and VT-d code. But of course the option is documented to be there for VT-d only, so one can view it as user error if it's used on a non-VT-d system. > If that's the case then iommu_snoop needs to be moved from vtd/iommu.c > to x86/iommu.c and iommu_snoop assignment via iommu param needs to be > guarded by CONFIG_INTEL_IOMMU. Or #define to true when !INTEL_IOMMU and keep the variable where it is. Jan
On 12/01/2023 3:43 pm, Xenia Ragiadakou wrote: > > On 1/12/23 13:49, Xenia Ragiadakou wrote: >> >> On 1/12/23 13:31, Jan Beulich wrote: >>> On 04.01.2023 09:44, Xenia Ragiadakou wrote: >>> >>>> --- a/xen/include/xen/iommu.h >>>> +++ b/xen/include/xen/iommu.h >>>> @@ -74,9 +74,13 @@ extern enum __packed iommu_intremap { >>>> iommu_intremap_restricted, >>>> iommu_intremap_full, >>>> } iommu_intremap; >>>> -extern bool iommu_igfx, iommu_qinval, iommu_snoop; >>>> #else >>>> # define iommu_intremap false >>>> +#endif >>>> + >>>> +#ifdef CONFIG_INTEL_IOMMU >>>> +extern bool iommu_igfx, iommu_qinval, iommu_snoop; >>>> +#else >>>> # define iommu_snoop false >>>> #endif >>> >>> Do these declarations really need touching? In patch 2 you didn't move >>> amd_iommu_perdev_intremap's either. >> >> Ok, I will revert this change (as I did in v2 of patch 2) since it is >> not needed. > > Actually, my patch was altering the current behavior by defining > iommu_snoop as false when !INTEL_IOMMU. > > IIUC, there is no control over snoop behavior when using the AMD > iommu. Hence, iommu_snoop should evaluate to true for AMD iommu. > However, when using the INTEL iommu the user can disable it via the > "iommu" param, right? > > If that's the case then iommu_snoop needs to be moved from vtd/iommu.c > to x86/iommu.c and iommu_snoop assignment via iommu param needs to be > guarded by CONFIG_INTEL_IOMMU. > Pretty much everything Xen thinks it knows about iommu_snoop is broken. AMD IOMMUs have had this capability since the outset, but it's the FC bit (Force Coherent). On Intel, the capability is optional, and typically differs between IOMMUs in the same system. Treating iommu_snoop as a single global is buggy, because (when available) it's always a per-SBDF control. It is used to take a TLP and force it to be coherent even when the device was trying to issue a non-coherent access. Intel systems typically have a dedicated IOMMU for the IGD, which always issues coherent accesses (its memory access happens as an adjunct to the LLC, not as something that communicates with the memory controller directly), so the IOMMU doesn't offer snoop control, and Xen "levels" this down to "the system can't do snoop control". Xen is very confused when it comes to cacheability correctness. I still have a pile of post-XSA-402 work pending, and it needs to start with splitting Xen's idea of "domain can use reduced cacheability" from "domain has a device", and work incrementally from there. But in terms of snoop_control, it's strictly necessary for the cases where the guest kernel thinks it is using reduced cacheability, but it isn't because of something the hypervisor has done. But beyond that, forcing snoop behind the back of a guest which is using reduced cacheability is just a waste of performance. ~Andrew
On 1/12/23 17:53, Jan Beulich wrote: > On 12.01.2023 16:43, Xenia Ragiadakou wrote: >> On 1/12/23 13:49, Xenia Ragiadakou wrote: >>> On 1/12/23 13:31, Jan Beulich wrote: >>>> On 04.01.2023 09:44, Xenia Ragiadakou wrote: >>>>> --- a/xen/include/xen/iommu.h >>>>> +++ b/xen/include/xen/iommu.h >>>>> @@ -74,9 +74,13 @@ extern enum __packed iommu_intremap { >>>>> iommu_intremap_restricted, >>>>> iommu_intremap_full, >>>>> } iommu_intremap; >>>>> -extern bool iommu_igfx, iommu_qinval, iommu_snoop; >>>>> #else >>>>> # define iommu_intremap false >>>>> +#endif >>>>> + >>>>> +#ifdef CONFIG_INTEL_IOMMU >>>>> +extern bool iommu_igfx, iommu_qinval, iommu_snoop; >>>>> +#else >>>>> # define iommu_snoop false >>>>> #endif >>>> >>>> Do these declarations really need touching? In patch 2 you didn't move >>>> amd_iommu_perdev_intremap's either. >>> >>> Ok, I will revert this change (as I did in v2 of patch 2) since it is >>> not needed. >> >> Actually, my patch was altering the current behavior by defining >> iommu_snoop as false when !INTEL_IOMMU. >> >> IIUC, there is no control over snoop behavior when using the AMD iommu. >> Hence, iommu_snoop should evaluate to true for AMD iommu. >> However, when using the INTEL iommu the user can disable it via the >> "iommu" param, right? > > That's the intended behavior, yes, but right now we allow the option > to also affect behavior on AMD - perhaps wrongly so, as there's one > use outside of VT-x and VT-d code. But of course the option is > documented to be there for VT-d only, so one can view it as user > error if it's used on a non-VT-d system. > >> If that's the case then iommu_snoop needs to be moved from vtd/iommu.c >> to x86/iommu.c and iommu_snoop assignment via iommu param needs to be >> guarded by CONFIG_INTEL_IOMMU. > > Or #define to true when !INTEL_IOMMU and keep the variable where it > is. Given the current implementation, if defined to true, it will be true even when !iommu_enabled.
On 13.01.2023 09:10, Xenia Ragiadakou wrote: > > On 1/12/23 17:53, Jan Beulich wrote: >> On 12.01.2023 16:43, Xenia Ragiadakou wrote: >>> On 1/12/23 13:49, Xenia Ragiadakou wrote: >>>> On 1/12/23 13:31, Jan Beulich wrote: >>>>> On 04.01.2023 09:44, Xenia Ragiadakou wrote: >>>>>> --- a/xen/include/xen/iommu.h >>>>>> +++ b/xen/include/xen/iommu.h >>>>>> @@ -74,9 +74,13 @@ extern enum __packed iommu_intremap { >>>>>> iommu_intremap_restricted, >>>>>> iommu_intremap_full, >>>>>> } iommu_intremap; >>>>>> -extern bool iommu_igfx, iommu_qinval, iommu_snoop; >>>>>> #else >>>>>> # define iommu_intremap false >>>>>> +#endif >>>>>> + >>>>>> +#ifdef CONFIG_INTEL_IOMMU >>>>>> +extern bool iommu_igfx, iommu_qinval, iommu_snoop; >>>>>> +#else >>>>>> # define iommu_snoop false >>>>>> #endif >>>>> >>>>> Do these declarations really need touching? In patch 2 you didn't move >>>>> amd_iommu_perdev_intremap's either. >>>> >>>> Ok, I will revert this change (as I did in v2 of patch 2) since it is >>>> not needed. >>> >>> Actually, my patch was altering the current behavior by defining >>> iommu_snoop as false when !INTEL_IOMMU. >>> >>> IIUC, there is no control over snoop behavior when using the AMD iommu. >>> Hence, iommu_snoop should evaluate to true for AMD iommu. >>> However, when using the INTEL iommu the user can disable it via the >>> "iommu" param, right? >> >> That's the intended behavior, yes, but right now we allow the option >> to also affect behavior on AMD - perhaps wrongly so, as there's one >> use outside of VT-x and VT-d code. But of course the option is >> documented to be there for VT-d only, so one can view it as user >> error if it's used on a non-VT-d system. >> >>> If that's the case then iommu_snoop needs to be moved from vtd/iommu.c >>> to x86/iommu.c and iommu_snoop assignment via iommu param needs to be >>> guarded by CONFIG_INTEL_IOMMU. >> >> Or #define to true when !INTEL_IOMMU and keep the variable where it >> is. > > Given the current implementation, if defined to true, it will be true > even when !iommu_enabled. Which is supposed to be benign; I'm about to send a patch to actually make it benign in shadow code as well (which is the one place where I notice it isn't right now). Jan
On 12.01.2023 19:24, Andrew Cooper wrote: > On 12/01/2023 3:43 pm, Xenia Ragiadakou wrote: >> >> On 1/12/23 13:49, Xenia Ragiadakou wrote: >>> >>> On 1/12/23 13:31, Jan Beulich wrote: >>>> On 04.01.2023 09:44, Xenia Ragiadakou wrote: >>>> >>>>> --- a/xen/include/xen/iommu.h >>>>> +++ b/xen/include/xen/iommu.h >>>>> @@ -74,9 +74,13 @@ extern enum __packed iommu_intremap { >>>>> iommu_intremap_restricted, >>>>> iommu_intremap_full, >>>>> } iommu_intremap; >>>>> -extern bool iommu_igfx, iommu_qinval, iommu_snoop; >>>>> #else >>>>> # define iommu_intremap false >>>>> +#endif >>>>> + >>>>> +#ifdef CONFIG_INTEL_IOMMU >>>>> +extern bool iommu_igfx, iommu_qinval, iommu_snoop; >>>>> +#else >>>>> # define iommu_snoop false >>>>> #endif >>>> >>>> Do these declarations really need touching? In patch 2 you didn't move >>>> amd_iommu_perdev_intremap's either. >>> >>> Ok, I will revert this change (as I did in v2 of patch 2) since it is >>> not needed. >> >> Actually, my patch was altering the current behavior by defining >> iommu_snoop as false when !INTEL_IOMMU. >> >> IIUC, there is no control over snoop behavior when using the AMD >> iommu. Hence, iommu_snoop should evaluate to true for AMD iommu. >> However, when using the INTEL iommu the user can disable it via the >> "iommu" param, right? >> >> If that's the case then iommu_snoop needs to be moved from vtd/iommu.c >> to x86/iommu.c and iommu_snoop assignment via iommu param needs to be >> guarded by CONFIG_INTEL_IOMMU. >> > > Pretty much everything Xen thinks it knows about iommu_snoop is broken. > > AMD IOMMUs have had this capability since the outset, but it's the FC > bit (Force Coherent). On Intel, the capability is optional, and > typically differs between IOMMUs in the same system. > > Treating iommu_snoop as a single global is buggy, because (when > available) it's always a per-SBDF control. It is used to take a TLP and > force it to be coherent even when the device was trying to issue a > non-coherent access. > > Intel systems typically have a dedicated IOMMU for the IGD, which always > issues coherent accesses (its memory access happens as an adjunct to the > LLC, not as something that communicates with the memory controller > directly), so the IOMMU doesn't offer snoop control, and Xen "levels" > this down to "the system can't do snoop control". > > > Xen is very confused when it comes to cacheability correctness. I still > have a pile of post-XSA-402 work pending, and it needs to start with > splitting Xen's idea of "domain can use reduced cacheability" from > "domain has a device", and work incrementally from there. > > But in terms of snoop_control, it's strictly necessary for the cases > where the guest kernel thinks it is using reduced cacheability, but it > isn't because of something the hypervisor has done. But beyond that, > forcing snoop behind the back of a guest which is using reduced > cacheability is just a waste of performance. I guess I agree with most/all you say, but that's all orthogonal to Xenia's work (and also to the patch I'm about to send to address the one issue that I've spotted while reviewing Xenia's patch). Jan
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index 998dfaf20d..a2c67a17cd 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -82,11 +82,13 @@ static int __init cf_check parse_iommu_param(const char *s) else if ( ss == s + 23 && !strncmp(s, "quarantine=scratch-page", 23) ) iommu_quarantine = IOMMU_quarantine_scratch_page; #endif -#ifdef CONFIG_X86 +#ifdef CONFIG_INTEL_IOMMU else if ( (val = parse_boolean("igfx", s, ss)) >= 0 ) iommu_igfx = val; else if ( (val = parse_boolean("qinval", s, ss)) >= 0 ) iommu_qinval = val; +#endif +#ifdef CONFIG_X86 else if ( (val = parse_boolean("superpages", s, ss)) >= 0 ) iommu_superpages = val; #endif diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 4f22fc1bed..aa924541d5 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -74,9 +74,13 @@ extern enum __packed iommu_intremap { iommu_intremap_restricted, iommu_intremap_full, } iommu_intremap; -extern bool iommu_igfx, iommu_qinval, iommu_snoop; #else # define iommu_intremap false +#endif + +#ifdef CONFIG_INTEL_IOMMU +extern bool iommu_igfx, iommu_qinval, iommu_snoop; +#else # define iommu_snoop false #endif
Use CONFIG_INTEL_IOMMU to guard their usage in common code. No functional change intended. Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> --- Changes in v2: - replace CONFIG_INTEL_VTD with CONFIG_INTEL_IOMMU xen/drivers/passthrough/iommu.c | 4 +++- xen/include/xen/iommu.h | 6 +++++- 2 files changed, 8 insertions(+), 2 deletions(-)