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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> Also, in vmx_do_resume(), iommu_snoop is used without checking if the 
> iommu is enabled.

It only looks to be - a domain having a PCI device implies it having
IOMMU enabled for it. And indeed in that case we'd like to avoid the
effort for domains which have IOMMU support enabled for them, but which
have no devices assigned to them.

Jan