[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/7] IOMMU: rename and re-type ats_enabled
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.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |