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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.01.2023 10:34, Xenia Ragiadakou wrote:
> 
> On 1/13/23 10: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
>> +
> 
> Why here iommu_snoop is forced when iommu is not enabled?
> This change is confusing because later on, in iommu_setup, iommu_snoop 
> will be set to false in the case of !iommu_enabled.

Counter question: Why is it being set to false there? I see no reason at
all. On the same basis as here, I'd actually expect it to be set back to
true in such a case. Which, however, would be a benign change now that
all uses of the variable are properly qualified. Which in turn is why I
thought I'd leave that other place alone.

Jan