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

[Jan Beulich <jbeulich@xxxxxxxx>]

(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 <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.
> 
> --- 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();
>      }
> 
>