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 <jbeul...@suse.com>
>> ---
>> 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 <p...@xen.org>
Thanks.
Jan
