Re: [PATCH 4/7] VT-d: replace find_ats_dev_drhd()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.02.2024 18:31, Roger Pau Monné wrote:
> On Mon, Feb 05, 2024 at 02:56:36PM +0100, Jan Beulich wrote:
>> All callers only care about boolean outcome. For this there's no point
>> in allocating a duplicate of the respective DRHD structure; a simple
>> boolean suffices (which eventually may wantg to become a count, such
>                                          ^ want
>> that the "any ATS devices assigned state" can also clear again). With
>> that boolean, remove respective parameters from internal helper
>> functions right away, as those have access to the flag through another
>> parameter.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> AFAICT the intention is that this is a non-functional change?

No functional effect intended, yes. Added such a sentence.

>> --- a/xen/drivers/passthrough/vtd/extern.h
>> +++ b/xen/drivers/passthrough/vtd/extern.h
>> @@ -65,8 +65,6 @@ struct acpi_drhd_unit *ioapic_to_drhd(un
>>  struct acpi_drhd_unit *hpet_to_drhd(unsigned int hpet_id);
>>  struct acpi_rhsa_unit *drhd_to_rhsa(const struct acpi_drhd_unit *drhd);
>>  
>> -struct acpi_drhd_unit *find_ats_dev_drhd(struct vtd_iommu *iommu);
>> -
>>  int ats_device(const struct pci_dev *, const struct acpi_drhd_unit *);
>>  
>>  int dev_invalidate_iotlb(struct vtd_iommu *iommu, u16 did,
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -624,8 +624,7 @@ int cf_check vtd_flush_iotlb_reg(
>>  }
>>  
>>  static int __must_check iommu_flush_iotlb_global(struct vtd_iommu *iommu,
>> -                                                 bool 
>> flush_non_present_entry,
>> -                                                 bool flush_dev_iotlb)
>> +                                                 bool 
>> flush_non_present_entry)
>>  {
>>      int status;
>>  
>> @@ -633,7 +632,7 @@ static int __must_check iommu_flush_iotl
>>      vtd_ops_preamble_quirk(iommu);
>>  
>>      status = iommu->flush.iotlb(iommu, 0, 0, 0, DMA_TLB_GLOBAL_FLUSH,
>> -                                flush_non_present_entry, flush_dev_iotlb);
>> +                                flush_non_present_entry, 
>> iommu->flush_dev_iotlb);
> 
> Any reason to not also remove the parameter from here also?  As the handler
> gets iommu passed as the first parameter anyway.

Indeed, yet then the patch would have grown quite a bit. I think I
meant to have a respective post-commit-message remark, but then
forgot to actually put one there. Once (if) this change has gone in,
a follow-on patch could further tidy tings. (The "right away" in the
description was kind of meant to indicate that.)

Jan