Re: [PATCH 1/2] x86/shadow: sanitize iommu_snoop usage

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 13/01/2023 8:47 am, Jan Beulich wrote:


As far as the subject goes, I really wouldn't call this "sanitise".  The
behaviour is crazy, and broken.

"Make shadow consistent with how HAP works" feels somewhat better.


> 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 <jbeulich@xxxxxxxx>
> ---
> 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.

That entire block looks suspect.  For one, I can't see why the ASSERT()
is correct; we have literally just (conditionally) added CACHE_ATTR to
pass_thru_flags and pulled everything across from gflags into sflags.

It can also half its number of external calls by rearranging the if/else
chain and making better use of the type variable.

>
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c

Just out of context here is a comment which says VT-d but really means
IOMMU.  It probably wants adjusting in the context of this change.

> @@ -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);

Hmm...  This is still one reasonably expensive nop; the PTE Flags for WB
are 0.

>                  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

I really don't think this is a good idea.  If nothing else, you're
reinforcing the notion that this logic is somehow acceptable.

If instead the comment read something like:

/* This logic is pretty bogus, but necessary for now.  iommu_snoop as a
control is only wired up for VT-d (which may be conditionally compiled
out), and while AMD can control coherency, Xen forces coherent accesses
unilaterally so iommu_snoop needs to report true on all AMD systems for
logic elsewhere in Xen to behave correctly. */

That at least highlights that it is a giant bodge.

~Andrew