Re: [PATCH v2 3/8] x86/iommu: iommu_igfx, iommu_qinval and iommu_snoop are VT-d specific

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.01.2023 19:24, Andrew Cooper wrote:
> On 12/01/2023 3:43 pm, Xenia Ragiadakou wrote:
>>
>> On 1/12/23 13:49, Xenia Ragiadakou wrote:
>>>
>>> On 1/12/23 13:31, Jan Beulich wrote:
>>>> On 04.01.2023 09:44, Xenia Ragiadakou wrote:
>>>>
>>>>> --- a/xen/include/xen/iommu.h
>>>>> +++ b/xen/include/xen/iommu.h
>>>>> @@ -74,9 +74,13 @@ extern enum __packed iommu_intremap {
>>>>>      iommu_intremap_restricted,
>>>>>      iommu_intremap_full,
>>>>>   } iommu_intremap;
>>>>> -extern bool iommu_igfx, iommu_qinval, iommu_snoop;
>>>>>   #else
>>>>>   # define iommu_intremap false
>>>>> +#endif
>>>>> +
>>>>> +#ifdef CONFIG_INTEL_IOMMU
>>>>> +extern bool iommu_igfx, iommu_qinval, iommu_snoop;
>>>>> +#else
>>>>>   # define iommu_snoop false
>>>>>   #endif
>>>>
>>>> Do these declarations really need touching? In patch 2 you didn't move
>>>> amd_iommu_perdev_intremap's either.
>>>
>>> Ok, I will revert this change (as I did in v2 of patch 2) since it is
>>> not needed.
>>
>> Actually, my patch was altering the current behavior by defining
>> iommu_snoop as false when !INTEL_IOMMU.
>>
>> IIUC, there is no control over snoop behavior when using the AMD
>> iommu. Hence, iommu_snoop should evaluate to true for AMD iommu.
>> However, when using the INTEL iommu the user can disable it via the
>> "iommu" param, right?
>>
>> If that's the case then iommu_snoop needs to be moved from vtd/iommu.c
>> to x86/iommu.c and iommu_snoop assignment via iommu param needs to be
>> guarded by CONFIG_INTEL_IOMMU.
>>
> 
> Pretty much everything Xen thinks it knows about iommu_snoop is broken.
> 
> AMD IOMMUs have had this capability since the outset, but it's the FC
> bit (Force Coherent).  On Intel, the capability is optional, and
> typically differs between IOMMUs in the same system.
> 
> Treating iommu_snoop as a single global is buggy, because (when
> available) it's always a per-SBDF control.  It is used to take a TLP and
> force it to be coherent even when the device was trying to issue a
> non-coherent access.
> 
> Intel systems typically have a dedicated IOMMU for the IGD, which always
> issues coherent accesses (its memory access happens as an adjunct to the
> LLC, not as something that communicates with the memory controller
> directly), so the IOMMU doesn't offer snoop control, and Xen "levels"
> this down to "the system can't do snoop control".
> 
> 
> Xen is very confused when it comes to cacheability correctness.  I still
> have a pile of post-XSA-402 work pending, and it needs to start with
> splitting Xen's idea of "domain can use reduced cacheability" from
> "domain has a device", and work incrementally from there.
> 
> But in terms of snoop_control, it's strictly necessary for the cases
> where the guest kernel thinks it is using reduced cacheability, but it
> isn't because of something the hypervisor has done.  But beyond that,
> forcing snoop behind the back of a guest which is using reduced
> cacheability is just a waste of performance.

I guess I agree with most/all you say, but that's all orthogonal to
Xenia's work (and also to the patch I'm about to send to address the
one issue that I've spotted while reviewing Xenia's patch).

Jan