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 <jbeul...@suse.com>
Reviewed-by: Xenia Ragiadakou <burzalod...@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();
}
--
Xenia
