Re: [PATCH v2] x86/shadow: make iommu_snoop usage consistent with HAP's

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

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@xxxxxxxx>
Reviewed-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx>

> ---
> 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