Re: [PATCH] pci: Enable overrides for missing ACS capabilities

[Bjorn Helgaas]

On Tue, Jun 18, 2013 at 12:20 PM, Alex Williamson
<alex.williamson@xxxxxxxxxx> wrote:
> On Tue, 2013-06-18 at 11:28 -0600, Bjorn Helgaas wrote:
>> On Thu, May 30, 2013 at 12:40:19PM -0600, Alex Williamson wrote:
>> > PCIe ACS (Access Control Services) is the PCIe 2.0+ feature that
>> > allows us to control whether transactions are allowed to be redirected
>> > in various subnodes of a PCIe topology.  For instance, if two
>> > endpoints are below a root port or downsteam switch port, the
>> > downstream port may optionally redirect transactions between the
>> > devices, bypassing upstream devices.  The same can happen internally
>> > on multifunction devices.  The transaction may never be visible to the
>> > upstream devices.
>> >
>> > One upstream device that we particularly care about is the IOMMU.  If
>> > a redirection occurs in the topology below the IOMMU, then the IOMMU
>> > cannot provide isolation between devices.  This is why the PCIe spec
>> > encourages topologies to include ACS support.  Without it, we have to
>> > assume peer-to-peer DMA within a hierarchy can bypass IOMMU isolation.
>> >
>> > Unfortunately, far too many topologies do not support ACS to make this
>> > a steadfast requirement.  Even the latest chipsets from Intel are only
>> > sporadically supporting ACS.  We have trouble getting interconnect
>> > vendors to include the PCIe spec required PCIe capability, let alone
>> > suggested features.
>> >
>> > Therefore, we need to add some flexibility.  The pcie_acs_override=
>> > boot option lets users opt-in specific devices or sets of devices to
>> > assume ACS support.
>>
>> "ACS support" means the device provides an ACS capability and we
>> can change bits in the ACS Control Register to control how things
>> work.
>>
>> You are really adding a way to "assume this device always routes
>> peer-to-peer DMA upstream," which ultimately translates into "assume
>> this device can be isolated from others via the IOMMU."  I think.
>
> We've heard from one vendor that they support ACS with a NULL capability
> structure, ie. the ACS PCIe capability exists, but reports no ACS
> capabilities and allows no control.  This takes advantage of the "if
> supported" style wording of the spec to imply that peer-to-peer is not
> supported because the capability is not available.  This supported but
> not controllable version is what we're trying to enable here.

I was wrong to say "we can change bits in the control register."  All
the control register bits are actually required to be hardwired to
zero when the relevant functionality is not implemented.

The example you mention (a device with an ACS capability structure
that reports no supported capabilities and allows no control) sounds
perfectly legal as a description of a device that doesn't support
peer-to-peer, and I hope it doesn't require any user intervention
(e.g., this patch) or quirks to make it work.

The ACS P2P Request Redirect "must be implemented by Functions that
support peer-to-peer traffic with other Functions."  This example
device doesn't support peer-to-peer traffic, so why would it implement
the ACS R bit?  In fact, it looks like the R bit (and all the other
bits) *must* be hardwired to zero in both capability and control
registers.

>> > The "downstream" option assumes full ACS support
>> > on root ports and downstream switch ports.  The "multifunction"
>> > option assumes the subset of ACS features available on multifunction
>> > endpoints and upstream switch ports are supported.  The "id:nnnn:nnnn"
>> > option enables ACS support on devices matching the provided vendor
>> > and device IDs, allowing more strategic ACS overrides.  These options
>> > may be combined in any order.  A maximum of 16 id specific overrides
>> > are available.  It's suggested to use the most limited set of options
>> > necessary to avoid completely disabling ACS across the topology.
>>
>> Probably I'm confused by your use of "assume full ACS support,"
>
> [on root ports and downstream ports]
>
>>  but I
>> don't understand the bit about "completely disabling ACS."
>
> [across the topology]
>
>>   If we use
>> more options than necessary, it seems like we'd assume more isolation
>> than really exists.  That sounds like pretending ACS is *enabled* in
>> more places than it really is.
>
> Exactly.  I'm just trying to suggest that booting with
> pcie_acs_override=downstream,multifunction is not generally recommended
> because it effectively disables ACS checking across the topology and
> assumes isolation where there may be none.  In other words, if
> everything is overriding ACS checks, then we've disabled any benefit of
> doing the checking in the first place (so I really mean disable
> checking, not disable ACS).

Yep, the missing "checking" is what is confusing.  Also, I think it
would be good to make the implication more explicit -- using this
option makes the kernel assume isolation where it may not actually
exist -- because the users of this option don't know about checking
anyway; that's an internal implementation detail.

> Instead, the recommendation is to be more
> selective, possibly opting for just "downstream" or even better, using
> the specific IDs for devices which are known or suspected to not allow
> peer-to-peer.
>
>> > Note to hardware vendors, we have facilities to permanently quirk
>> > specific devices which enforce isolation but not provide an ACS
>> > capability.  Please contact me to have your devices added and save
>> > your customers the hassle of this boot option.
>>
>> Who do you expect to decide whether to use this option?  I think it
>> requires intimate knowledge of how the device works.
>>
>> I think the benefit of using the option is that it makes assignment of
>> devices to guests more flexible, which will make it attractive to users.
>> But most users have no way of knowing whether it's actually *safe* to
>> use this.  So I worry that you're adding an easy way to pretend isolation
>> exists when there's no good way of being confident that it actually does.
>>
>> I see the warning you added for this case; I just don't feel good about
>> it.  Maybe the idea is that, e.g., Red Hat will research certain devices
>> and recommend the option for those cases, and sign up to support that
>> config.  I assume you would not be willing to support its use unless
>> Red Hat specifically recommended it.
>
> That pretty much sums it up.  We're working with vendors to try to
> figure out which devices do not support ACS but don't allow
> peer-to-peer, but it's difficult to get decisive statements to that
> affect.  Legacy KVM device assignment relied on libvirt to do this ACS
> checking and it does a poor job of it, allowing far more devices to be
> assigned with ACS enforced than it really should.  It's also trivial to
> disable libvirt's enforcement of ACS and people do it every day w/o
> really thinking of the implications.  With VFIO-based device assignment
> we move to a model where the kernel is enforcing device isolation rather
> than it being at the whim of a userspace service.  Now we have not only
> a more complete ACS test, but no way to make it more lenient.  Devices
> that were previously allowed, no longer are and there's no way around it
> without this patch.  However, this patch gives us more control than the
> global disable switch in libvirt.  We can still be selective and fine
> tune the shape of the groups, while hopefully adhering to the isolation
> exposed by the hardware elsewhere.
>
> I agree that it's difficult to determine whether it's safe to override,
> but I don't see a way around it.  If it was obvious how to maintain
> isolation, we'd do it automatically.  We need better ACS support from
> vendors.  In the mean time, this allows people to complain to their
> hardware vendors and opt-in to using the device anyway.  The warning is
> there because if I'm debugging odd problems, I want to know that
> isolation may be compromised, as the warning indicates.  Thanks,

I wonder if we should taint the kernel if this option is used (but not
for specific devices added to pci_dev_acs_enabled[]).  It would also
be nice if pci_dev_specific_acs_enabled() gave some indication in
dmesg for the specific devices you're hoping to add to
pci_dev_acs_enabled[].  It's not an enumeration-time quirk right now,
so I'm not sure how we'd limit it to one message per device.

I assume that if RH knows about certain devices that are safe in this
respect, you'd just add them to pci_dev_acs_enabled[] up front, and
the command-line option is really just a workaround until you can spin
a new kernel that has the table updated?

It might even make sense to simplify this option so it just assumes
*all* devices can be isolated, and get rid of the
"downstream/multifunction/vendor & device ID" stuff.  That would be
much easier to explain, and it would make it more obvious that we're
really doing something pretty scary here.

Bjorn (there is one doc comment below)

> Alex
>
>> > Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
>> > ---
>> >  Documentation/kernel-parameters.txt |   10 +++
>> >  drivers/pci/quirks.c                |  102 +++++++++++++++++++++++++++++++++++
>> >  2 files changed, 112 insertions(+)
>> >
>> > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>> > index 47bb23c..a60e6ad 100644
>> > --- a/Documentation/kernel-parameters.txt
>> > +++ b/Documentation/kernel-parameters.txt
>> > @@ -2349,6 +2349,16 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>> >             nomsi   Do not use MSI for native PCIe PME signaling (this makes
>> >                     all PCIe root ports use INTx for all services).
>> >
>> > +   pcie_acs_override =
>> > +                   [PCIE] Override missing PCIe ACS support for:
>> > +           downstream
>> > +                   All downstream ports - full ACS capabilties
>> > +           multifunction
>> > +                   All multifunction devices - multifunction ACS subset
>> > +           id:nnnn:nnnn
>> > +                   Specfic device - full ACS capabilities
>> > +                   Specified as vid:did (vendor/device ID) in hex

This should say something about "device isolation support," I think.
It's too big a leap from "missing ACS support" to expect users to make
it.

>> > +
>> >     pcmv=           [HW,PCMCIA] BadgePAD 4
>> >
>> >     pd.             [PARIDE]
>> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> > index 0369fb6..c7609f6 100644
>> > --- a/drivers/pci/quirks.c
>> > +++ b/drivers/pci/quirks.c
>> > @@ -3292,11 +3292,113 @@ struct pci_dev *pci_get_dma_source(struct pci_dev *dev)
>> >     return pci_dev_get(dev);
>> >  }
>> >
>> > +static bool acs_on_downstream;
>> > +static bool acs_on_multifunction;
>> > +
>> > +#define NUM_ACS_IDS 16
>> > +struct acs_on_id {
>> > +   unsigned short vendor;
>> > +   unsigned short device;
>> > +};
>> > +static struct acs_on_id acs_on_ids[NUM_ACS_IDS];
>> > +static u8 max_acs_id;
>> > +
>> > +static __init int pcie_acs_override_setup(char *p)
>> > +{
>> > +   if (!p)
>> > +           return -EINVAL;
>> > +
>> > +   while (*p) {
>> > +           if (!strncmp(p, "downstream", 10))
>> > +                   acs_on_downstream = true;
>> > +           if (!strncmp(p, "multifunction", 13))
>> > +                   acs_on_multifunction = true;
>> > +           if (!strncmp(p, "id:", 3)) {
>> > +                   char opt[5];
>> > +                   int ret;
>> > +                   long val;
>> > +
>> > +                   if (max_acs_id >= NUM_ACS_IDS - 1) {
>> > +                           pr_warn("Out of PCIe ACS override slots (%d)\n",
>> > +                                   NUM_ACS_IDS);
>> > +                           goto next;
>> > +                   }
>> > +
>> > +                   p += 3;
>> > +                   snprintf(opt, 5, "%s", p);
>> > +                   ret = kstrtol(opt, 16, &val);
>> > +                   if (ret) {
>> > +                           pr_warn("PCIe ACS ID parse error %d\n", ret);
>> > +                           goto next;
>> > +                   }
>> > +                   acs_on_ids[max_acs_id].vendor = val;
>> > +
>> > +                   p += strcspn(p, ":");
>> > +                   if (*p != ':') {
>> > +                           pr_warn("PCIe ACS invalid ID\n");
>> > +                           goto next;
>> > +                   }
>> > +
>> > +                   p++;
>> > +                   snprintf(opt, 5, "%s", p);
>> > +                   ret = kstrtol(opt, 16, &val);
>> > +                   if (ret) {
>> > +                           pr_warn("PCIe ACS ID parse error %d\n", ret);
>> > +                           goto next;
>> > +                   }
>> > +                   acs_on_ids[max_acs_id].device = val;
>> > +                   max_acs_id++;
>> > +           }
>> > +next:
>> > +           p += strcspn(p, ",");
>> > +           if (*p == ',')
>> > +                   p++;
>> > +   }
>> > +
>> > +   if (acs_on_downstream || acs_on_multifunction || max_acs_id)
>> > +           pr_warn("Warning: PCIe ACS overrides enabled; This may allow non-IOMMU protected peer-to-peer DMA\n");
>> > +
>> > +   return 0;
>> > +}
>> > +early_param("pcie_acs_override", pcie_acs_override_setup);
>> > +
>> > +static int pcie_acs_overrides(struct pci_dev *dev, u16 acs_flags)
>> > +{
>> > +   int i;
>> > +
>> > +   /* Never override ACS for legacy devices or devices with ACS caps */
>> > +   if (!pci_is_pcie(dev) ||
>> > +       pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS))
>> > +           return -ENOTTY;
>> > +
>> > +   for (i = 0; i < max_acs_id; i++)
>> > +           if (acs_on_ids[i].vendor == dev->vendor &&
>> > +               acs_on_ids[i].device == dev->device)
>> > +                   return 1;
>> > +
>> > +   switch (pci_pcie_type(dev)) {
>> > +   case PCI_EXP_TYPE_DOWNSTREAM:
>> > +   case PCI_EXP_TYPE_ROOT_PORT:
>> > +           if (acs_on_downstream)
>> > +                   return 1;
>> > +           break;
>> > +   case PCI_EXP_TYPE_ENDPOINT:
>> > +   case PCI_EXP_TYPE_UPSTREAM:
>> > +   case PCI_EXP_TYPE_LEG_END:
>> > +   case PCI_EXP_TYPE_RC_END:
>> > +           if (acs_on_multifunction && dev->multifunction)
>> > +                   return 1;
>> > +   }
>> > +
>> > +   return -ENOTTY;
>> > +}
>> > +
>> >  static const struct pci_dev_acs_enabled {
>> >     u16 vendor;
>> >     u16 device;
>> >     int (*acs_enabled)(struct pci_dev *dev, u16 acs_flags);
>> >  } pci_dev_acs_enabled[] = {
>> > +   { PCI_ANY_ID, PCI_ANY_ID, pcie_acs_overrides },
>> >     { 0 }
>> >  };
>> >
>> >
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/