Re: [PATCH v8 4/6] AMD/IOMMU: respect AtsDisabled device flag

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.09.2021 09:34, Durrant, Paul wrote:
> On 22/09/2021 15:37, Jan Beulich wrote:
>> IVHD entries may specify that ATS is to be blocked for a device or range
>> of devices. Honor firmware telling us so.
>>
>> While adding respective checks I noticed that the 2nd conditional in
>> amd_iommu_setup_domain_device() failed to check the IOMMU's capability.
>> Add the missing part of the condition there, as no good can come from
>> enabling ATS on a device when the IOMMU is not capable of dealing with
>> ATS requests.
>>
>> For actually using ACPI_IVHD_ATS_DISABLED, make its expansion no longer
>> exhibit UB.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> TBD: As an alternative to adding the missing IOMMU capability check, we
>>       may want to consider simply using dte->i in the 2nd conditional in
>>       amd_iommu_setup_domain_device().
>> Note that while ATS enabling/disabling gets invoked without any locks
>> held, the two functions should not be possible to race with one another
>> for any individual device (or else we'd be in trouble already, as ATS
>> might then get re-enabled immediately after it was disabled, with the
>> DTE out of sync with this setting).
>> ---
>> v7: New.
>>
>> --- a/xen/drivers/passthrough/amd/iommu.h
>> +++ b/xen/drivers/passthrough/amd/iommu.h
>> @@ -120,6 +120,7 @@ struct ivrs_mappings {
>>       uint16_t dte_requestor_id;
>>       bool valid:1;
>>       bool dte_allow_exclusion:1;
>> +    bool block_ats:1;
>>   
>>       /* ivhd device data settings */
>>       uint8_t device_flags;
>> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
>> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>> @@ -55,8 +55,8 @@ union acpi_ivhd_device {
>>   };
>>   
>>   static void __init add_ivrs_mapping_entry(
>> -    uint16_t bdf, uint16_t alias_id, uint8_t flags, bool alloc_irt,
>> -    struct amd_iommu *iommu)
>> +    uint16_t bdf, uint16_t alias_id, uint8_t flags, unsigned int ext_flags,
>> +    bool alloc_irt, struct amd_iommu *iommu)
>>   {
>>       struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(iommu->seg);
>>   
>> @@ -66,6 +66,7 @@ static void __init add_ivrs_mapping_entr
>>       ivrs_mappings[bdf].dte_requestor_id = alias_id;
>>   
>>       /* override flags for range of devices */
>> +    ivrs_mappings[bdf].block_ats = ext_flags & ACPI_IVHD_ATS_DISABLED;
>>       ivrs_mappings[bdf].device_flags = flags;
> 
> I'm assuming the above indentation problem (which appears to be repeated 
> in a few places below) is more to do with patch email generation rather 
> than the actual code modifications so...

Hmm, I first suspected a Thunderbird regression, but checking the list
archives I don't see any corruption. I'm therefore now suspecting the
problem may be at your end ...

> Reviewed-by: Paul Durrant <paul@xxxxxxx>

Thanks.

Jan