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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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.

~Andrew