Disabling ACS for peer-to-peer support

Disabling ACS for peer-to-peer support

[Skidanov, Alexey]

Hello,

I have recently found the below commit to disabling ACS bits. Using kernel parameter is pretty simple but requires to know in advance which devices might be participated in peer-to-peer sessions.

 Why we can't disable the ACS bits *after* the driver is initialized (and there is a request to connect between two peers) and not *during* device discovering?.

Thanks,
Alexey


commit aaca43fda742223e4f62bd73e13055f5364e9a9b
Author: Logan Gunthorpe <logang@deltatee.com>
Date:   Mon Jul 30 10:18:40 2018 -0600

    PCI: Add "pci=disable_acs_redir=" parameter for peer-to-peer support

    To support peer-to-peer traffic on a segment of the PCI hierarchy, we must
    disable the ACS redirect bits for select PCI bridges.  The bridges must be
    selected before the devices are discovered by the kernel and the IOMMU
    groups created.  Therefore, add a kernel command line parameter to specify
    devices which must have their ACS bits disabled.

    The new parameter takes a list of devices separated by a semicolon.  Each;
    device specified will have its ACS redirect bits disabled.  This is
    similar to the existing 'resource_alignment' parameter.

    The ACS Request P2P Request Redirect, P2P Completion Redirect and P2P
    Egress Control bits are disabled, which is sufficient to always allow
    passing P2P traffic uninterrupted.  The bits are set after the kernel
    (optionally) enables the ACS bits itself.  It is also done regardless of
    whether the kernel or platform firmware sets the bits.

    If the user tries to disable the ACS redirect for a device without the ACS
    capability, print a warning to dmesg.

    Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
    [bhelgaas: reorder to add the generic code first and move the
    device-specific quirk to subsequent patches]
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Reviewed-by: Stephen Bates <sbates@raithlin.com>
    Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
    Acked-by: Christian König <christian.koenig@amd.com>

Re: Disabling ACS for peer-to-peer support

[Christian König]

Am 27.01.20 um 08:18 schrieb Skidanov, Alexey:
> Hello,
>
> I have recently found the below commit to disabling ACS bits. Using kernel parameter is pretty simple but requires to know in advance which devices might be participated in peer-to-peer sessions.
>
>   Why we can't disable the ACS bits *after* the driver is initialized (and there is a request to connect between two peers) and not *during* device discovering?.

That's exactly what was initially proposed but we have seen hardware 
which reacts allergic to disabling those bits on the fly.

Please read up the discussion on the mailing list leading to this patch.

Regards,
Christian.

>
> Thanks,
> Alexey
>
>
> commit aaca43fda742223e4f62bd73e13055f5364e9a9b
> Author: Logan Gunthorpe <logang@deltatee.com>
> Date:   Mon Jul 30 10:18:40 2018 -0600
>
>      PCI: Add "pci=disable_acs_redir=" parameter for peer-to-peer support
>
>      To support peer-to-peer traffic on a segment of the PCI hierarchy, we must
>      disable the ACS redirect bits for select PCI bridges.  The bridges must be
>      selected before the devices are discovered by the kernel and the IOMMU
>      groups created.  Therefore, add a kernel command line parameter to specify
>      devices which must have their ACS bits disabled.
>
>      The new parameter takes a list of devices separated by a semicolon.  Each;
>      device specified will have its ACS redirect bits disabled.  This is
>      similar to the existing 'resource_alignment' parameter.
>
>      The ACS Request P2P Request Redirect, P2P Completion Redirect and P2P
>      Egress Control bits are disabled, which is sufficient to always allow
>      passing P2P traffic uninterrupted.  The bits are set after the kernel
>      (optionally) enables the ACS bits itself.  It is also done regardless of
>      whether the kernel or platform firmware sets the bits.
>
>      If the user tries to disable the ACS redirect for a device without the ACS
>      capability, print a warning to dmesg.
>
>      Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
>      [bhelgaas: reorder to add the generic code first and move the
>      device-specific quirk to subsequent patches]
>      Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>      Reviewed-by: Stephen Bates <sbates@raithlin.com>
>      Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
>      Acked-by: Christian König <christian.koenig@amd.com>

Re: Disabling ACS for peer-to-peer support

[Logan Gunthorpe]

On 2020-01-27 1:18 a.m., Christian König wrote:
> Am 27.01.20 um 08:18 schrieb Skidanov, Alexey:
>> Hello,
>>
>> I have recently found the below commit to disabling ACS bits. Using kernel parameter is pretty simple but requires to know in advance which devices might be participated in peer-to-peer sessions.
>>
>>   Why we can't disable the ACS bits *after* the driver is initialized (and there is a request to connect between two peers) and not *during* device discovering?.
> 
> That's exactly what was initially proposed but we have seen hardware 
> which reacts allergic to disabling those bits on the fly.

I wasn't aware of that and haven't seen anything like that.

> Please read up the discussion on the mailing list leading to this patch.

The issue was the IOMMU code does not allow for any kind of dynamic
changes in the groups devices are assigned in. In theory, this could be
possible but you'd still at least have to unbind the devices from their
driver because you definitely can't change the IOMMU group while there
are DMA requests in flight. Ultimately it's easier for most use cases to
just disable it on boot.

Logan

Re: Disabling ACS for peer-to-peer support

[Christian König]

Am 27.01.20 um 17:58 schrieb Logan Gunthorpe:
>
> On 2020-01-27 1:18 a.m., Christian König wrote:
>> Am 27.01.20 um 08:18 schrieb Skidanov, Alexey:
>>> Hello,
>>>
>>> I have recently found the below commit to disabling ACS bits. Using kernel parameter is pretty simple but requires to know in advance which devices might be participated in peer-to-peer sessions.
>>>
>>>    Why we can't disable the ACS bits *after* the driver is initialized (and there is a request to connect between two peers) and not *during* device discovering?.
>> That's exactly what was initially proposed but we have seen hardware
>> which reacts allergic to disabling those bits on the fly.
> I wasn't aware of that and haven't seen anything like that.
>
>> Please read up the discussion on the mailing list leading to this patch.
> The issue was the IOMMU code does not allow for any kind of dynamic
> changes in the groups devices are assigned in. In theory, this could be
> possible but you'd still at least have to unbind the devices from their
> driver because you definitely can't change the IOMMU group while there
> are DMA requests in flight. Ultimately it's easier for most use cases to
> just disable it on boot.

As far as I know you can't change the ACS bit either when there are 
transactions in flight on the affected devices/bridges.

Otherwise what could happen is that the response of an transaction takes 
a different path than the request. That in turn can result in quite a 
bunch of ordering problem on the PCIe bus.

But the idea of unbinding a device, changing the bit and rebinding it 
would probably work.

Regards,
Christian.

>
> Logan

Re: Disabling ACS for peer-to-peer support

[Logan Gunthorpe]

On 2020-01-27 12:12 p.m., Christian König wrote:
> Am 27.01.20 um 17:58 schrieb Logan Gunthorpe:
>>
>> On 2020-01-27 1:18 a.m., Christian König wrote:
>>> Am 27.01.20 um 08:18 schrieb Skidanov, Alexey:
>>>> Hello,
>>>>
>>>> I have recently found the below commit to disabling ACS bits. Using kernel parameter is pretty simple but requires to know in advance which devices might be participated in peer-to-peer sessions.
>>>>
>>>>    Why we can't disable the ACS bits *after* the driver is initialized (and there is a request to connect between two peers) and not *during* device discovering?.
>>> That's exactly what was initially proposed but we have seen hardware
>>> which reacts allergic to disabling those bits on the fly.
>> I wasn't aware of that and haven't seen anything like that.
>>
>>> Please read up the discussion on the mailing list leading to this patch.
>> The issue was the IOMMU code does not allow for any kind of dynamic
>> changes in the groups devices are assigned in. In theory, this could be
>> possible but you'd still at least have to unbind the devices from their
>> driver because you definitely can't change the IOMMU group while there
>> are DMA requests in flight. Ultimately it's easier for most use cases to
>> just disable it on boot.
> 
> As far as I know you can't change the ACS bit either when there are 
> transactions in flight on the affected devices/bridges.

No, I think the ACS bits are fine to change at any time. I've never had
any issue changing them. The problem is the act of changing them changes
the isolation between the devices which means the IOMMU groups have to
change.

It's certainly possible today to just use setpci to adjust those bits at
any time. It just means the isolation the IOMMU is expecting will be
wrong and that may mean you broke the security between VMs on your machine.

> Otherwise what could happen is that the response of an transaction takes 
> a different path than the request. That in turn can result in quite a 
> bunch of ordering problem on the PCIe bus.
> 
> But the idea of unbinding a device, changing the bit and rebinding it 
> would probably work.

Well, no, you can't just change the bit, you have to change the IOMMU
group the device belongs to. Right now, we don't have any interface to
do that except during scanning at boot.

Logan

RE: Disabling ACS for peer-to-peer support

[Skidanov, Alexey]

>On 2020-01-27 12:12 p.m., Christian König wrote:
>> Am 27.01.20 um 17:58 schrieb Logan Gunthorpe:
>>>
>>> On 2020-01-27 1:18 a.m., Christian König wrote:
>>>> Am 27.01.20 um 08:18 schrieb Skidanov, Alexey:
>>>>> Hello,
>>>>>
>>>>> I have recently found the below commit to disabling ACS bits. Using kernel parameter
>is pretty simple but requires to know in advance which devices might be participated in
>peer-to-peer sessions.
>>>>>
>>>>>    Why we can't disable the ACS bits *after* the driver is initialized (and there is a
>request to connect between two peers) and not *during* device discovering?.
>>>> That's exactly what was initially proposed but we have seen hardware
>>>> which reacts allergic to disabling those bits on the fly.
>>> I wasn't aware of that and haven't seen anything like that.
>>>
>>>> Please read up the discussion on the mailing list leading to this patch.
>>> The issue was the IOMMU code does not allow for any kind of dynamic
>>> changes in the groups devices are assigned in. In theory, this could be
>>> possible but you'd still at least have to unbind the devices from their
>>> driver because you definitely can't change the IOMMU group while there
>>> are DMA requests in flight. Ultimately it's easier for most use cases to
>>> just disable it on boot.
>>
>> As far as I know you can't change the ACS bit either when there are
>> transactions in flight on the affected devices/bridges.
>
>No, I think the ACS bits are fine to change at any time. I've never had
>any issue changing them. The problem is the act of changing them changes
>the isolation between the devices which means the IOMMU groups have to
>change.
>
>It's certainly possible today to just use setpci to adjust those bits at
>any time. It just means the isolation the IOMMU is expecting will be
>wrong and that may mean you broke the security between VMs on your machine.
>

According to the PCIe spec, there are two mechanisms to deal with isolation:
- Redirected Request Validation logic within the RC and
- ACS P2P Egress Control
So anyone who cares about the isolation must use at least one of these mechanisms. 
I would expect that on VM creation, the above mechanisms will be configured appropriately. 

>> Otherwise what could happen is that the response of an transaction takes
>> a different path than the request. That in turn can result in quite a
>> bunch of ordering problem on the PCIe bus.
>>
>> But the idea of unbinding a device, changing the bit and rebinding it
>> would probably work.
>
>Well, no, you can't just change the bit, you have to change the IOMMU
>group the device belongs to. Right now, we don't have any interface to
>do that except during scanning at boot.
>
>Logan

Re: Disabling ACS for peer-to-peer support

[Alex Williamson]

On Tue, 28 Jan 2020 07:13:01 +0000
"Skidanov, Alexey" <alexey.skidanov@intel.com> wrote:

> >On 2020-01-27 12:12 p.m., Christian König wrote:  
> >> Am 27.01.20 um 17:58 schrieb Logan Gunthorpe:  
> >>>
> >>> On 2020-01-27 1:18 a.m., Christian König wrote:  
> >>>> Am 27.01.20 um 08:18 schrieb Skidanov, Alexey:  
> >>>>> Hello,
> >>>>>
> >>>>> I have recently found the below commit to disabling ACS bits. Using kernel parameter  
> >is pretty simple but requires to know in advance which devices might be participated in
> >peer-to-peer sessions.  
> >>>>>
> >>>>>    Why we can't disable the ACS bits *after* the driver is initialized (and there is a  
> >request to connect between two peers) and not *during* device discovering?.  
> >>>> That's exactly what was initially proposed but we have seen hardware
> >>>> which reacts allergic to disabling those bits on the fly.  
> >>> I wasn't aware of that and haven't seen anything like that.
> >>>  
> >>>> Please read up the discussion on the mailing list leading to this patch.  
> >>> The issue was the IOMMU code does not allow for any kind of dynamic
> >>> changes in the groups devices are assigned in. In theory, this could be
> >>> possible but you'd still at least have to unbind the devices from their
> >>> driver because you definitely can't change the IOMMU group while there
> >>> are DMA requests in flight. Ultimately it's easier for most use cases to
> >>> just disable it on boot.  
> >>
> >> As far as I know you can't change the ACS bit either when there are
> >> transactions in flight on the affected devices/bridges.  
> >
> >No, I think the ACS bits are fine to change at any time. I've never had
> >any issue changing them. The problem is the act of changing them changes
> >the isolation between the devices which means the IOMMU groups have to
> >change.
> >
> >It's certainly possible today to just use setpci to adjust those bits at
> >any time. It just means the isolation the IOMMU is expecting will be
> >wrong and that may mean you broke the security between VMs on your machine.
> >  
> 
> According to the PCIe spec, there are two mechanisms to deal with isolation:
> - Redirected Request Validation logic within the RC and
> - ACS P2P Egress Control
> So anyone who cares about the isolation must use at least one of these mechanisms. 
> I would expect that on VM creation, the above mechanisms will be configured appropriately. 

Redirected Request Validation within the RC presumes that transactions
make it to the RC, and thus implies things like Upstream Forwarding,
Request Redirection, and Completion Redirection.  I think much of the
desire for P2P wants to avoid the RC entirely.  Also, this Redirected
Request Validation logic and control of it is not part of the PCIe
spec.  P2P Egress Control only allows blocking of P2P between select
downstream ports.  AIUI this is used in conjunction with P2P Direct
Translation to limit which direct translated paths are available to a
device.  DT on its own implies ATS, which has questionable isolation
security (ie. susceptible to an exploitable endpoint).  IIRC, it's also
been rejected in previous discussions that we could simply wait for
hardware that supports ATS and DT to build the direct P2P paths
desired.  There are also some arguable benefits to IOMMU isolation for
non-VM use cases, so let's not put blinders on to those aspects.

> >> Otherwise what could happen is that the response of an transaction takes
> >> a different path than the request. That in turn can result in quite a
> >> bunch of ordering problem on the PCIe bus.
> >>
> >> But the idea of unbinding a device, changing the bit and rebinding it
> >> would probably work.  
> >
> >Well, no, you can't just change the bit, you have to change the IOMMU
> >group the device belongs to. Right now, we don't have any interface to
> >do that except during scanning at boot.

Right, and groups potentially contain multiple devices and multiple
groups might be dependent on the isolation of a given interconnect,
which means the scope of modifying that grouping may involve unrelated
devices and drivers, which feeds into that boot-time restriction.  We
could do runtime modification, but it seems to imply quiescing DMA
among unrelated drivers, which probably means the simplest solution is
unbinding an entire hierarchy of drivers, soft unplugging the devices,
and re-scanning with a different ACS policy, which in practice may not
have a lot of benefit vs rebooting.  Thanks,

Alex

Re: Disabling ACS for peer-to-peer support

[Logan Gunthorpe]

On 2020-01-28 12:13 a.m., Skidanov, Alexey wrote:
>> On 2020-01-27 12:12 p.m., Christian König wrote:
>>> Am 27.01.20 um 17:58 schrieb Logan Gunthorpe:
>>>>
>>>> On 2020-01-27 1:18 a.m., Christian König wrote:
>>>>> Am 27.01.20 um 08:18 schrieb Skidanov, Alexey:
>>>>>> Hello,
>>>>>>
>>>>>> I have recently found the below commit to disabling ACS bits. Using kernel parameter
>> is pretty simple but requires to know in advance which devices might be participated in
>> peer-to-peer sessions.
>>>>>>
>>>>>>    Why we can't disable the ACS bits *after* the driver is initialized (and there is a
>> request to connect between two peers) and not *during* device discovering?.
>>>>> That's exactly what was initially proposed but we have seen hardware
>>>>> which reacts allergic to disabling those bits on the fly.
>>>> I wasn't aware of that and haven't seen anything like that.
>>>>
>>>>> Please read up the discussion on the mailing list leading to this patch.
>>>> The issue was the IOMMU code does not allow for any kind of dynamic
>>>> changes in the groups devices are assigned in. In theory, this could be
>>>> possible but you'd still at least have to unbind the devices from their
>>>> driver because you definitely can't change the IOMMU group while there
>>>> are DMA requests in flight. Ultimately it's easier for most use cases to
>>>> just disable it on boot.
>>>
>>> As far as I know you can't change the ACS bit either when there are
>>> transactions in flight on the affected devices/bridges.
>>
>> No, I think the ACS bits are fine to change at any time. I've never had
>> any issue changing them. The problem is the act of changing them changes
>> the isolation between the devices which means the IOMMU groups have to
>> change.
>>
>> It's certainly possible today to just use setpci to adjust those bits at
>> any time. It just means the isolation the IOMMU is expecting will be
>> wrong and that may mean you broke the security between VMs on your machine.
>>
> 
> According to the PCIe spec, there are two mechanisms to deal with isolation:
> - Redirected Request Validation logic within the RC and
> - ACS P2P Egress Control
> So anyone who cares about the isolation must use at least one of these mechanisms. 
> I would expect that on VM creation, the above mechanisms will be configured appropriately. 

That was my expectation too, a long time ago, but that is not how it
works. IOMMU groups are created, on boot, based on the ACS bit settings
(and other mechanisms). Any devices that are not isolated are put in the
same groups. The groups are then used to program the IOMMU such that
each group has it's own virtual address region.

When a VM is created with a PCI passthru device, it will fail if you
don't pass through every device in a each group involved. If the ACS
bits are updated after boot, there is no mechanism to change the groups
so you could create a VM with a device that is not isolated.

Logan