(missed to CC Paul on the original submission)
On 13.01.2023 09:47, 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).
>
> Signed-off-by: Jan Beulich <jbeul...@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.
>
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -571,7 +571,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/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -56,6 +56,13 @@ void __init acpi_iommu_init(void)
> if ( !acpi_disabled )
> {
> ret = acpi_dmar_init();
> +
> +#ifndef iommu_snoop
> + /* A command line override for snoop control affects VT-d only. */
> + if ( ret )
> + iommu_snoop = true;
> +#endif
> +
> if ( ret == -ENODEV )
> ret = acpi_ivrs_init();
> }
>
>
