| Message ID | 8d41b8ca-decb-e175-c77a-1c8f91fd2046@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] x86/shadow: make iommu_snoop usage consistent with HAP's | expand |
On 1/20/23 10:44, Jan Beulich wrote: > First of all the variable is meaningful only when an IOMMU is in use for > a guest. Qualify the check accordingly, like done elsewhere. Furthermore > the controlling command line option is supposed to take effect on VT-d > only. Since command line parsing happens before we know whether we're > going to use VT-d, force the variable back to set when instead running > with AMD IOMMU(s). > > Since it may end up misleading, also remove the clearing of the flag in > iommu_setup() and vtd_setup()'s error path. The variable simply is > meaningless with IOMMU(s) disabled, so there's no point touching it > there. > > Finally also correct a relevant nearby comment. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Xenia Ragiadakou <burzalodowa@gmail.com> > --- > I was first considering to add the extra check to the outermost > enclosing if(), but I guess that would break the (questionable) case of > assigning MMIO ranges directly by address. The way it's done now also > better fits the existing checks, in particular the ones in p2m-ept.c. > > Note that the #ifndef is put there in anticipation of iommu_snoop > becoming a #define when !IOMMU_INTEL (see > https://lists.xen.org/archives/html/xen-devel/2023-01/msg00103.html > and replies). > > In _sh_propagate() I'm further puzzled: The iomem_access_permitted() > certainly suggests very bad things could happen if it returned false > (i.e. in the implicit "else" case). The assumption looks to be that no > bad "target_mfn" can make it there. But overall things might end up > looking more sane (and being cheaper) when simply using "mmio_mfn" > instead. > --- > v2: Change title. Extend comment in acpi_iommu_init(). Purge clearing > of the variable from iommu_setup() and vtd_setup()'s error path. > > --- a/xen/arch/x86/mm/shadow/multi.c > +++ b/xen/arch/x86/mm/shadow/multi.c > @@ -556,8 +556,8 @@ _sh_propagate(struct vcpu *v, > > ASSERT(!(sflags & PAGE_CACHE_ATTRS)); > > - /* compute the PAT index for shadow page entry when VT-d is enabled > - * and device assigned. > + /* > + * Compute the PAT index for shadow page entry when IOMMU is enabled. > * 1) direct MMIO: compute the PAT index with gMTRR=UC and gPAT. > * 2) if enables snoop control, compute the PAT index as WB. > * 3) if disables snoop control, compute the PAT index with > @@ -577,7 +577,7 @@ _sh_propagate(struct vcpu *v, > gfn_to_paddr(target_gfn), > mfn_to_maddr(target_mfn), > X86_MT_UC); > - else if ( iommu_snoop ) > + else if ( is_iommu_enabled(d) && iommu_snoop ) > sflags |= pat_type_2_pte_flags(X86_MT_WB); > else > sflags |= get_pat_flags(v, > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -587,9 +587,6 @@ int __init iommu_setup(void) > printk("I/O virtualisation %sabled\n", iommu_enabled ? "en" : "dis"); > if ( !iommu_enabled ) > { > -#ifndef iommu_snoop > - iommu_snoop = false; > -#endif > iommu_hwdom_passthrough = false; > iommu_hwdom_strict = false; > } > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -2746,9 +2746,6 @@ static int __init cf_check vtd_setup(voi > > error: > iommu_enabled = 0; > -#ifndef iommu_snoop > - iommu_snoop = false; > -#endif > iommu_hwdom_passthrough = false; > iommu_qinval = 0; > iommu_intremap = iommu_intremap_off; > --- a/xen/drivers/passthrough/x86/iommu.c > +++ b/xen/drivers/passthrough/x86/iommu.c > @@ -56,6 +56,17 @@ void __init acpi_iommu_init(void) > if ( !acpi_disabled ) > { > ret = acpi_dmar_init(); > + > +#ifndef iommu_snoop > + /* > + * As long as there's no per-domain snoop control, and as long as on > + * AMD we uniformly force coherent accesses, a possible command line > + * override should affect VT-d only. > + */ > + if ( ret ) > + iommu_snoop = true; > +#endif > + > if ( ret == -ENODEV ) > ret = acpi_ivrs_init(); > }
> From: Jan Beulich <jbeulich@suse.com> > Sent: Friday, January 20, 2023 4:44 PM > > First of all the variable is meaningful only when an IOMMU is in use for > a guest. Qualify the check accordingly, like done elsewhere. Furthermore > the controlling command line option is supposed to take effect on VT-d > only. Since command line parsing happens before we know whether we're > going to use VT-d, force the variable back to set when instead running > with AMD IOMMU(s). > > Since it may end up misleading, also remove the clearing of the flag in > iommu_setup() and vtd_setup()'s error path. The variable simply is > meaningless with IOMMU(s) disabled, so there's no point touching it > there. > > Finally also correct a relevant nearby comment. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
--- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -556,8 +556,8 @@ _sh_propagate(struct vcpu *v, ASSERT(!(sflags & PAGE_CACHE_ATTRS)); - /* compute the PAT index for shadow page entry when VT-d is enabled - * and device assigned. + /* + * Compute the PAT index for shadow page entry when IOMMU is enabled. * 1) direct MMIO: compute the PAT index with gMTRR=UC and gPAT. * 2) if enables snoop control, compute the PAT index as WB. * 3) if disables snoop control, compute the PAT index with @@ -577,7 +577,7 @@ _sh_propagate(struct vcpu *v, gfn_to_paddr(target_gfn), mfn_to_maddr(target_mfn), X86_MT_UC); - else if ( iommu_snoop ) + else if ( is_iommu_enabled(d) && iommu_snoop ) sflags |= pat_type_2_pte_flags(X86_MT_WB); else sflags |= get_pat_flags(v, --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -587,9 +587,6 @@ int __init iommu_setup(void) printk("I/O virtualisation %sabled\n", iommu_enabled ? "en" : "dis"); if ( !iommu_enabled ) { -#ifndef iommu_snoop - iommu_snoop = false; -#endif iommu_hwdom_passthrough = false; iommu_hwdom_strict = false; } --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -2746,9 +2746,6 @@ static int __init cf_check vtd_setup(voi error: iommu_enabled = 0; -#ifndef iommu_snoop - iommu_snoop = false; -#endif iommu_hwdom_passthrough = false; iommu_qinval = 0; iommu_intremap = iommu_intremap_off; --- a/xen/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -56,6 +56,17 @@ void __init acpi_iommu_init(void) if ( !acpi_disabled ) { ret = acpi_dmar_init(); + +#ifndef iommu_snoop + /* + * As long as there's no per-domain snoop control, and as long as on + * AMD we uniformly force coherent accesses, a possible command line + * override should affect VT-d only. + */ + if ( ret ) + iommu_snoop = true; +#endif + if ( ret == -ENODEV ) ret = acpi_ivrs_init(); }
First of all the variable is meaningful only when an IOMMU is in use for a guest. Qualify the check accordingly, like done elsewhere. Furthermore the controlling command line option is supposed to take effect on VT-d only. Since command line parsing happens before we know whether we're going to use VT-d, force the variable back to set when instead running with AMD IOMMU(s). Since it may end up misleading, also remove the clearing of the flag in iommu_setup() and vtd_setup()'s error path. The variable simply is meaningless with IOMMU(s) disabled, so there's no point touching it there. Finally also correct a relevant nearby comment. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- I was first considering to add the extra check to the outermost enclosing if(), but I guess that would break the (questionable) case of assigning MMIO ranges directly by address. The way it's done now also better fits the existing checks, in particular the ones in p2m-ept.c. Note that the #ifndef is put there in anticipation of iommu_snoop becoming a #define when !IOMMU_INTEL (see https://lists.xen.org/archives/html/xen-devel/2023-01/msg00103.html and replies). In _sh_propagate() I'm further puzzled: The iomem_access_permitted() certainly suggests very bad things could happen if it returned false (i.e. in the implicit "else" case). The assumption looks to be that no bad "target_mfn" can make it there. But overall things might end up looking more sane (and being cheaper) when simply using "mmio_mfn" instead. --- v2: Change title. Extend comment in acpi_iommu_init(). Purge clearing of the variable from iommu_setup() and vtd_setup()'s error path.