Re: [PATCH 2/7] IOMMU: rename and re-type ats_enabled

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Feb 12, 2024 at 11:45:33AM +0100, Jan Beulich wrote:
> On 12.02.2024 10:39, Roger Pau Monné wrote:
> > On Thu, Feb 08, 2024 at 04:49:46PM +0100, Jan Beulich wrote:
> >> On 08.02.2024 12:49, Roger Pau Monné wrote:
> >>> On Mon, Feb 05, 2024 at 02:55:43PM +0100, Jan Beulich wrote:
> >>>> Make the variable a tristate, with (as done elsewhere) a negative value
> >>>> meaning "default". Since all use sites need looking at, also rename it
> >>>> to match our usual "opt_*" pattern. While touching it, also move it to
> >>>> .data.ro_after_init.
> >>>>
> >>>> The only place it retains boolean nature is pci_ats_device(), for now.
> >>>
> >>> Why does it retain the boolean nature in pci_ats_device()?
> >>>
> >>> I assume this is to avoid having to touch the line again in a further
> >>> patch, as given the current logic pci_ats_device() would also want to
> >>> treat -1 as ATS disabled.
> >>
> >> No, then I would need to touch the line. The function wants to treat
> >> -1 as "maybe enabled", so the caller can know whether a device is an
> >> ATS device regardless of whether ATS use is fully off, or only
> >> "soft-off".
> > 
> > I have to admit I'm slightly concerned about this soft-off.  Given the
> > current status of ATS itself in Xen, and the technology itself, I
> > think a user should always opt-in to ATS usage.
> 
> The plan is to follow your suggestion in patch 3 and require explicit
> enabling for passing through of such devices. For Dom0, however, I
> think it is important that we respect the firmware request by default.
> The only viable(?!) alternative would be to panic() instead.

Or assign to domIO?

> >>>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> >>>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> >>>> @@ -185,10 +185,11 @@ static int __must_check amd_iommu_setup_
> >>>>          dte->ex = ivrs_dev->dte_allow_exclusion;
> >>>>          dte->sys_mgt = MASK_EXTR(ivrs_dev->device_flags, 
> >>>> ACPI_IVHD_SYSTEM_MGMT);
> >>>>  
> >>>> -        if ( pci_ats_device(iommu->seg, bus, pdev->devfn) &&
> >>>> +        if ( opt_ats > 0 &&
> >>>>               !ivrs_dev->block_ats &&
> >>>> -             iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) )
> >>>> -            dte->i = ats_enabled;
> >>>> +             iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) &&
> >>>> +             pci_ats_device(iommu->seg, bus, pdev->devfn) )
> >>>> +            dte->i = true;
> >>>>  
> >>>>          spin_unlock_irqrestore(&iommu->lock, flags);
> >>>>  
> >>>> @@ -248,10 +249,11 @@ static int __must_check amd_iommu_setup_
> >>>>          ASSERT(dte->sys_mgt == MASK_EXTR(ivrs_dev->device_flags,
> >>>>                                           ACPI_IVHD_SYSTEM_MGMT));
> >>>>  
> >>>> -        if ( pci_ats_device(iommu->seg, bus, pdev->devfn) &&
> >>>> +        if ( opt_ats > 0 &&
> >>>>               !ivrs_dev->block_ats &&
> >>>> -             iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) )
> >>>> -            ASSERT(dte->i == ats_enabled);
> >>>> +             iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) &&
> >>>> +             pci_ats_device(iommu->seg, bus, pdev->devfn) )
> >>>> +            ASSERT(dte->i);
> >>>>  
> >>>>          spin_unlock_irqrestore(&iommu->lock, flags);
> >>>>  
> >>>> @@ -268,9 +270,10 @@ static int __must_check amd_iommu_setup_
> >>>>  
> >>>>      ASSERT(pcidevs_locked());
> >>>>  
> >>>> -    if ( pci_ats_device(iommu->seg, bus, pdev->devfn) &&
> >>>> +    if ( opt_ats > 0 &&
> >>>>           !ivrs_dev->block_ats &&
> >>>>           iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) &&
> >>>> +         pci_ats_device(iommu->seg, bus, pdev->devfn) &&
> >>>>           !pci_ats_enabled(iommu->seg, bus, pdev->devfn) )
> >>>
> >>> Seeing that this same set of conditions is used in 3 different checks,
> >>> could we add a wrapper for it?
> >>>
> >>> opt_ats > 0 && !ivrs_dev->block_ats &&
> >>> iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) &&
> >>> pci_ats_device(iommu->seg, bus, pdev->devfn)
> >>>
> >>> pci_device_ats_capable()? or some such.
> >>
> >> I was pondering that, yes (iirc already once when adding block_ats).
> >> Problem is the name. "capable" isn't quite right when considering
> >> the tristate opt_ats. And pci_device_may_use_ats() reads, well,
> >> clumsy to me. If you have any good idea for a name that's fully
> >> applicable and not odd or overly long, I can certainly introduce
> >> such a helper.
> > 
> > But if ATS is soft-disabled (-1) or hard disabled (0), it's fine to
> > consider the devices as not ATS capable for the context here?
> 
> I don't like mixing capability and policy aspects into a resulting
> "capable".

IMO we should prefer avoiding code repetition, even if at the cost
of having a handler that have a maybe not ideal naming, but I can't
force you to do that.

Thanks, Roger.