Re: [PATCH] vfio/pci: Add support for PCIe extended capabilities

[Vivek Kumar Gautam <vivek.gautam@arm.com>]

Hi Will,


On 9/1/21 4:14 PM, Will Deacon wrote:
> On Wed, Sep 01, 2021 at 11:27:21AM +0100, Andre Przywara wrote:
>> On 8/31/21 3:54 PM, Will Deacon wrote:
>>
>> Hi Will,
>>
>>> On Tue, Aug 10, 2021 at 11:55:14AM +0530, Vivek Gautam wrote:
>>>> Add support to parse extended configuration space for vfio based
>>>> assigned PCIe devices and add extended capabilities for the device
>>>> in the guest. This allows the guest to see and work on extended
>>>> capabilities, for example to toggle PRI extended cap to enable and
>>>> disable Shared virtual addressing (SVA) support.
>>>> PCIe extended capability header that is the first DWORD of all
>>>> extended caps is shown below -
>>>>
>>>>      31               20  19   16  15                 0
>>>>      ____________________|_______|_____________________
>>>>     |    Next cap off    |  1h   |     Cap ID          |
>>>>     |____________________|_______|_____________________|
>>>>
>>>> Out of the two upper bytes of extended cap header, the
>>>> lower nibble is actually cap version - 0x1.
>>>> 'next cap offset' if present at bits [31:20], should be
>>>> right shifted by 4 bits to calculate the position of next
>>>> capability.
>>>> This change supports parsing and adding ATS, PRI and PASID caps.
>>>>
>>>> Signed-off-by: Vivek Gautam <vivek.gautam@arm.com>
>>>> ---
>>>>    include/kvm/pci.h |   6 +++
>>>>    vfio/pci.c        | 104 ++++++++++++++++++++++++++++++++++++++++++----
>>>>    2 files changed, 103 insertions(+), 7 deletions(-)
>>>
>>> Does this work correctly for architectures which don't define ARCH_HAS_PCI_EXP?
>>
>> I think it does: the code compiles fine, and the whole functionality is
>> guarded by:
>> +    /* Extended cap only for PCIe devices */
>> +    if (!arch_has_pci_exp())
>> +            return 0;
>>
>> A clever compiler might even decide to not include this code at all.
>>
>> Did you see any particular problem?
>
> The part I was worried about is that PCI_DEV_CFG_MASK (which is used by
> the cfg space dispatch code) is derived from PCI_DEV_CFG_SIZE, but actually
> I think this patch might _fix_ that problem because it removes the explicit
> usage of PCI_DEV_CFG_SIZE_LEGACY!

That's right. On adding change for PCI_DEV_CFG_SIZE the entire
configuration space can now be read. With Alex's change for PCI Express
support PCI_DEV_CFG_SIZE can be either PCI_DEV_CFG_SIZE_EXTENDED or
PCI_DEV_CFG_SIZE_LEGACY depending on arch_has_pci_exp() check.

I booted with enabling and disabling ARCH_HAS_PCI_EXP and was able to
see a passthrough device getting detected fine in the VM. Logs are below.

With ARCH_HAS_PCI_EXP enabled  (master with this patch):
--------------------------------------------------------
# lspci -vv

00:00.0 Unassigned class [ff00]: ARM Device ff80

         Subsystem: ARM Device 0000

         Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop-
ParErr- Stepping- SERR- FastB2B- DisINTx+

         Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort-
<TAbort- <MAbort- >SERR- <PERR- INTx-

         Latency: 0

         Region 0: Memory at 50000000 (32-bit, non-prefetchable) [size=256K]

         Region 2: Memory at 50040000 (32-bit, non-prefetchable) [size=32K]

         Region 4: Memory at 50048000 (32-bit, non-prefetchable) [size=4K]

         Capabilities: [48] MSI-X: Enable+ Count=2048 Masked-

                 Vector table: BAR=2 offset=00000000

                 PBA: BAR=4 offset=00000000

         Capabilities: [54] Express (v2) Endpoint, MSI 00

                 DevCap: MaxPayload 128 bytes, PhantFunc 0, Latency L0s
<64ns, L1 <1us

                         ExtTag+ AttnBtn- AttnInd- PwrInd- RBE+ FLReset+
SlotPowerLimit 0.000W

                 DevCtl: CorrErr- NonFatalErr- FatalErr- UnsupReq-

                         RlxdOrd- ExtTag+ PhantFunc- AuxPwr- NoSnoop-
FLReset-

                         MaxPayload 128 bytes, MaxReadReq 128 bytes

                 DevSta: CorrErr- NonFatalErr- FatalErr- UnsupReq-
AuxPwr- TransPend-

                 LnkCap: Port #0, Speed unknown, Width x0, ASPM not
supported

                         ClockPM- Surprise- LLActRep- BwNot- ASPMOptComp-

                 LnkCtl: ASPM Disabled; RCB 64 bytes, Disabled- CommClk-

                         ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-

                 LnkSta: Speed unknown (ok), Width x0 (ok)

                         TrErr- Train- SlotClk- DLActive- BWMgmt- ABWMgmt-

                 DevCap2: Completion Timeout: Not Supported, TimeoutDis-
NROPrPrP- LTR-

                          10BitTagComp- 10BitTagReq- OBFF Not Supported,
ExtFmt- EETLPPrefix-

                          EmergencyPowerReduction Not Supported,
EmergencyPowerReductionInit-

                          FRS- TPHComp- ExtTPHComp-

                          AtomicOpsCap: 32bit- 64bit- 128bitCAS-

                 DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis-
LTR- OBFF Disabled,

                          AtomicOpsCtl: ReqEn-

                 LnkCtl2: Target Link Speed: 2.5GT/s, EnterCompliance-
SpeedDis-

                          Transmit Margin: Normal Operating Range,
EnterModifiedCompliance- ComplianceSOS-

                          Compliance De-emphasis: -6dB

                 LnkSta2: Current De-emphasis Level: -6dB,
EqualizationComplete- EqualizationPhase1-

                          EqualizationPhase2- EqualizationPhase3-
LinkEqualizationRequest-

                          Retimer- 2Retimers- CrosslinkRes: unsupported

         Capabilities: [100 v1] Address Translation Service (ATS)

                 ATSCap: Invalidate Queue Depth: 00

                 ATSCtl: Enable+, Smallest Translation Unit: 00

         Capabilities: [108 v0] Page Request Interface (PRI)

                 PRICtl: Enable- Reset-

                 PRISta: RF- UPRGI- Stopped+

                 Page Request Capacity: 000000ff, Page Request
Allocation: 00000000

         Kernel driver in use: smmute-pci





With ARCH_HAS_PCI_EXP disabled (on top of this patch):
--------------------------------------------------------
# lspci -vv
00:00.0 Unassigned class [ff00]: ARM Device ff80
         Subsystem: ARM Device 0000
         Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop-
ParErr- Stepping- SERR- FastB2B- DisINTx+
         Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort-
<TAbort- <MAbort- >SERR- <PERR- INTx-
         Latency: 0
         Region 0: Memory at 50000000 (32-bit, non-prefetchable) [size=256K]
         Region 2: Memory at 50040000 (32-bit, non-prefetchable) [size=32K]
         Region 4: Memory at 50048000 (32-bit, non-prefetchable) [size=4K]
         Capabilities: [48] MSI-X: Enable+ Count=2048 Masked-
                 Vector table: BAR=2 offset=00000000
                 PBA: BAR=4 offset=00000000
         Kernel driver in use: smmute-pci

Let me know if you have concerns.

Best regards
Vivek

>
> Will
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.