On Fri, Feb 09, 2024 at 08:06:07AM +0100, Jan Beulich wrote:
> 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 <jbeul...@suse.com>
> >
> > AFAICT the intention is that this is a non-functional change?
>
> No functional effect intended, yes. Added such a sentence.
Acked-by: Roger Pau Monné <roger....@citrix.com>
>
> >> --- 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.)
Would you mind adding a sentence to the commit message that the
vtd_iommu hooks are not modified in order to avoid the patch growing
too much? Otherwise it it's not clear why those are not also
converted.
Thanks, Roger.
