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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.01.2023 14:07, Xenia Ragiadakou wrote:
> 
> On 1/13/23 14:12, Jan Beulich wrote:
>> On 13.01.2023 12:55, Xenia Ragiadakou wrote:
>>> On 1/13/23 11:53, Jan Beulich wrote:
>>>> On 13.01.2023 10:34, Xenia Ragiadakou wrote:
>>>>> On 1/13/23 10:47, Jan Beulich wrote:
>>>>>> --- 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.
>>>
>>> I think I got confused... AFAIU with disabled iommu snooping cannot be
>>> enforced i.e iommu_snoop=true translates to snooping is enforced by the
>>> iommu (that's why we need to check that the iommu is enabled for the
>>> guest). So if the iommu is disabled how can iommu_snoop be true?
>>
>> For a non-existent (or disabled) IOMMU the value of this boolean simply
>> is irrelevant. Or to put it in other words, when there's no active
>> IOMMU, it doesn't matter whether it would actually snoop.
> 
> The variable iommu_snoop is initialized to true.
> Also, the above change does not prevent it from being overwritten 
> through the cmdline iommu param in iommu_setup().

Command line parsing happens earlier (and in parse_iommu_param(), not in
iommu_setup()). iommu_setup() can further overwrite it on its error path,
but as said that's benign then.

> I m afraid I still cannot understand why the change above is needed.

When using an AMD IOMMU, with how things work right now the variable ought
to always be true (hence why I've suggested that when !INTEL_IOMMU, this
simply become a #define to true). See also Andrew's comments here and/or
on your patch.

Jan