diff mbox series

pcie: enable Extended tag field capability

Message ID 20241017-pcie-extend-v1-1-03eb1029f4ca@linaro.org
State New
Headers show
Series pcie: enable Extended tag field capability | expand

Commit Message

Marcin Juszkiewicz Oct. 17, 2024, 1:33 p.m. UTC 
PCI has 32 transactions, PCI Express devices can handle 256.

SBSA ACS checks for this capability to be enabled on Arm server systems.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
---
SBSA Reference Platform work goes on so I am looking at PCIe related tests.

SBSA ACS has test 824 which checks for PCIe device capabilities. BSA
specification [1] (SBSA is on top of BSA) in section F.3.2 lists
expected values for Device Capabilities Register:

Device Capabilities Register     Requirement
Role based error reporting       RCEC and RCiEP: Hardwired to 1
Endpoint L0s acceptable latency  RCEC and RCiEP: Hardwired to 0
L1 acceptable latency            RCEC and RCiEP: Hardwired to 0
Captured slot power limit scale  RCEC and RCiEP: Hardwired to 0
Captured slot power limit value  RCEC and RCiEP: Hardwired to 0
Max payload size                 value must be compliant with PCIe spec
Phantom functions                RCEC and RCiEP: Recommendation is to
                                 hardwire this bit to 0.
Extended tag field               Hardwired to 1

1. https://developer.arm.com/documentation/den0094/c/

QEMU leaves 'Extended tag field' with 0 as value:

Capabilities: [e0] Express (v1) Root Complex Integrated Endpoint, IntMsgNum 0
        DevCap: MaxPayload 128 bytes, PhantFunc 0
                ExtTag- RBE+ FLReset- TEE-IO-

From what I read PCI has 32 transactions, PCI Express devices can handle
256 with Extended tag enabled (spec mentions also larger values but I
lack PCIe knowledge).
---
 hw/pci/pcie.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


---
base-commit: f774a677507966222624a9b2859f06ede7608100
change-id: 20241017-pcie-extend-a6a9de74dbd0

Best regards,

Comments

Michael S. Tsirkin Oct. 17, 2024, 4:58 p.m. UTC | #1 
On Thu, Oct 17, 2024 at 03:33:44PM +0200, Marcin Juszkiewicz wrote:
> PCI has 32 transactions, PCI Express devices can handle 256.
> 
> SBSA ACS checks for this capability to be enabled on Arm server systems.
> 
> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> ---
> SBSA Reference Platform work goes on so I am looking at PCIe related tests.
> 
> SBSA ACS has test 824 which checks for PCIe device capabilities. BSA
> specification [1] (SBSA is on top of BSA) in section F.3.2 lists
> expected values for Device Capabilities Register:
> 
> Device Capabilities Register     Requirement
> Role based error reporting       RCEC and RCiEP: Hardwired to 1
> Endpoint L0s acceptable latency  RCEC and RCiEP: Hardwired to 0
> L1 acceptable latency            RCEC and RCiEP: Hardwired to 0
> Captured slot power limit scale  RCEC and RCiEP: Hardwired to 0
> Captured slot power limit value  RCEC and RCiEP: Hardwired to 0
> Max payload size                 value must be compliant with PCIe spec
> Phantom functions                RCEC and RCiEP: Recommendation is to
>                                  hardwire this bit to 0.
> Extended tag field               Hardwired to 1
> 
> 1. https://developer.arm.com/documentation/den0094/c/
> 
> QEMU leaves 'Extended tag field' with 0 as value:
> 
> Capabilities: [e0] Express (v1) Root Complex Integrated Endpoint, IntMsgNum 0
>         DevCap: MaxPayload 128 bytes, PhantFunc 0
>                 ExtTag- RBE+ FLReset- TEE-IO-
> 
> >From what I read PCI has 32 transactions, PCI Express devices can handle
> 256 with Extended tag enabled (spec mentions also larger values but I
> lack PCIe knowledge).
> ---
>  hw/pci/pcie.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 4b2f0805c6..54c0f1ec67 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -86,7 +86,8 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t type, uint8_t version)
>       * Specification, Revision 1.1., or subsequent PCI Express Base
>       * Specification revisions.
>       */
> -    pci_set_long(exp_cap + PCI_EXP_DEVCAP, PCI_EXP_DEVCAP_RBER);
> +    pci_set_long(exp_cap + PCI_EXP_DEVCAP, PCI_EXP_DEVCAP_RBER |
> +                 PCI_EXP_DEVCAP_EXT_TAG);
>  
>      pci_set_long(exp_cap + PCI_EXP_LNKCAP,
>                   (port << PCI_EXP_LNKCAP_PN_SHIFT) |


We can't change capabilities unconditionally.
It needs at least a machine type compat thing.

> ---
> base-commit: f774a677507966222624a9b2859f06ede7608100
> change-id: 20241017-pcie-extend-a6a9de74dbd0
> 
> Best regards,
> -- 
> Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
Marcin Juszkiewicz Oct. 17, 2024, 7:18 p.m. UTC | #2 
W dniu 17.10.2024 o 18:58, Michael S. Tsirkin pisze:
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index 4b2f0805c6..54c0f1ec67 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -86,7 +86,8 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t type, uint8_t version)
>>        * Specification, Revision 1.1., or subsequent PCI Express Base
>>        * Specification revisions.
>>        */
>> -    pci_set_long(exp_cap + PCI_EXP_DEVCAP, PCI_EXP_DEVCAP_RBER);
>> +    pci_set_long(exp_cap + PCI_EXP_DEVCAP, PCI_EXP_DEVCAP_RBER |
>> +                 PCI_EXP_DEVCAP_EXT_TAG);
>>   
>>       pci_set_long(exp_cap + PCI_EXP_LNKCAP,
>>                    (port << PCI_EXP_LNKCAP_PN_SHIFT) |
> 
> We can't change capabilities unconditionally.
> It needs at least a machine type compat thing.

Thanks for review. Will look how to handle it better then.
Marcin Juszkiewicz Oct. 18, 2024, 10:24 a.m. UTC | #3 
W dniu 17.10.2024 o 18:58, Michael S. Tsirkin pisze:
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index 4b2f0805c6..54c0f1ec67 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -86,7 +86,8 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t type, uint8_t version)
>>        * Specification, Revision 1.1., or subsequent PCI Express Base
>>        * Specification revisions.
>>        */
>> -    pci_set_long(exp_cap + PCI_EXP_DEVCAP, PCI_EXP_DEVCAP_RBER);
>> +    pci_set_long(exp_cap + PCI_EXP_DEVCAP, PCI_EXP_DEVCAP_RBER |
>> +                 PCI_EXP_DEVCAP_EXT_TAG);
>>   
>>       pci_set_long(exp_cap + PCI_EXP_LNKCAP,
>>                    (port << PCI_EXP_LNKCAP_PN_SHIFT) |
> 
> We can't change capabilities unconditionally.
> It needs at least a machine type compat thing.

Started looking and wonder how to pass it from MachineClass level down 
to pcie.c/pcie_cap_v1_fill() level.

hw/arm/sbsa-ref.c (the machine I know best) has create_pcie() which 
allocates PCI_HOST_BRIDGE and then creates 2 pcie devices (default_nic 
(e1000e) and bochs-display gfx).

If I add "pcie_uses_ext_tags = true" to SBSAMachineState then I need to 
have it stored in PCIBus structure so pci_create_simple() will know. 
This function would copy it into "dev" (PCIDevice) to make it pass to 
pcie_cap_v1_fill() function.

But that's not right way because other PCIe devices are created in other 
places.

Need to dig deeper.
Igor Mammedov Oct. 22, 2024, 7:34 a.m. UTC | #4 
On Fri, 18 Oct 2024 12:24:05 +0200
Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org> wrote:

> W dniu 17.10.2024 o 18:58, Michael S. Tsirkin pisze:
> >> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> >> index 4b2f0805c6..54c0f1ec67 100644
> >> --- a/hw/pci/pcie.c
> >> +++ b/hw/pci/pcie.c
> >> @@ -86,7 +86,8 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t type, uint8_t version)
> >>        * Specification, Revision 1.1., or subsequent PCI Express Base
> >>        * Specification revisions.
> >>        */
> >> -    pci_set_long(exp_cap + PCI_EXP_DEVCAP, PCI_EXP_DEVCAP_RBER);
> >> +    pci_set_long(exp_cap + PCI_EXP_DEVCAP, PCI_EXP_DEVCAP_RBER |
> >> +                 PCI_EXP_DEVCAP_EXT_TAG);
> >>   
> >>       pci_set_long(exp_cap + PCI_EXP_LNKCAP,
> >>                    (port << PCI_EXP_LNKCAP_PN_SHIFT) |  
> > 
> > We can't change capabilities unconditionally.
> > It needs at least a machine type compat thing.  
> 
> Started looking and wonder how to pass it from MachineClass level down 
> to pcie.c/pcie_cap_v1_fill() level.

see as an example:
  fa905f65c5549 hw/nvme: add machine compatibility parameter to enable msix exclusive bar
 
> hw/arm/sbsa-ref.c (the machine I know best) has create_pcie() which 
> allocates PCI_HOST_BRIDGE and then creates 2 pcie devices (default_nic 
> (e1000e) and bochs-display gfx).
> 
> If I add "pcie_uses_ext_tags = true" to SBSAMachineState then I need to 
> have it stored in PCIBus structure so pci_create_simple() will know. 
> This function would copy it into "dev" (PCIDevice) to make it pass to 
> pcie_cap_v1_fill() function.
> 
> But that's not right way because other PCIe devices are created in other 
> places.
> 
> Need to dig deeper.
>
diff mbox series

Patch

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 4b2f0805c6..54c0f1ec67 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -86,7 +86,8 @@  pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t type, uint8_t version)
      * Specification, Revision 1.1., or subsequent PCI Express Base
      * Specification revisions.
      */
-    pci_set_long(exp_cap + PCI_EXP_DEVCAP, PCI_EXP_DEVCAP_RBER);
+    pci_set_long(exp_cap + PCI_EXP_DEVCAP, PCI_EXP_DEVCAP_RBER |
+                 PCI_EXP_DEVCAP_EXT_TAG);
 
     pci_set_long(exp_cap + PCI_EXP_LNKCAP,
                  (port << PCI_EXP_LNKCAP_PN_SHIFT) |