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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.01.2023 14:53, Xenia Ragiadakou wrote:
> On 1/13/23 15:24, Jan Beulich wrote:
>> 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.
> 
> You are right. I misunderstood.
> 
>>
>>> 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.
> 
> Ok I see, so this change is specific to AMD iommu and when iommu_snoop 
> becomes a #define, this change won't be needed anymore, right?

Well the (source) code change will still be needed; it'll simply be
compiled out for the case where iommu_snoop is a #define (which it
looks like we agree it will be when !INTEL_IOMMU).

Jan