qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] pcie: enable Extended tag field capability
@ 2024-10-17 13:33 Marcin Juszkiewicz
  2024-10-17 16:58 ` Michael S. Tsirkin
  0 siblings, 1 reply; 5+ messages in thread
From: Marcin Juszkiewicz @ 2024-10-17 13:33 UTC (permalink / raw)
  To: qemu-devel; +Cc: Michael S. Tsirkin, Marcel Apfelbaum, Marcin Juszkiewicz

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) |

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

Best regards,
-- 
Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] pcie: enable Extended tag field capability
  2024-10-17 13:33 [PATCH] pcie: enable Extended tag field capability Marcin Juszkiewicz
@ 2024-10-17 16:58 ` Michael S. Tsirkin
  2024-10-17 19:18   ` Marcin Juszkiewicz
  2024-10-18 10:24   ` Marcin Juszkiewicz
  0 siblings, 2 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2024-10-17 16:58 UTC (permalink / raw)
  To: Marcin Juszkiewicz; +Cc: qemu-devel, Marcel Apfelbaum

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>



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] pcie: enable Extended tag field capability
  2024-10-17 16:58 ` Michael S. Tsirkin
@ 2024-10-17 19:18   ` Marcin Juszkiewicz
  2024-10-18 10:24   ` Marcin Juszkiewicz
  1 sibling, 0 replies; 5+ messages in thread
From: Marcin Juszkiewicz @ 2024-10-17 19:18 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: qemu-devel, Marcel Apfelbaum

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.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] pcie: enable Extended tag field capability
  2024-10-17 16:58 ` Michael S. Tsirkin
  2024-10-17 19:18   ` Marcin Juszkiewicz
@ 2024-10-18 10:24   ` Marcin Juszkiewicz
  2024-10-22  7:34     ` Igor Mammedov
  1 sibling, 1 reply; 5+ messages in thread
From: Marcin Juszkiewicz @ 2024-10-18 10:24 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: qemu-devel, Marcel Apfelbaum

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.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] pcie: enable Extended tag field capability
  2024-10-18 10:24   ` Marcin Juszkiewicz
@ 2024-10-22  7:34     ` Igor Mammedov
  0 siblings, 0 replies; 5+ messages in thread
From: Igor Mammedov @ 2024-10-22  7:34 UTC (permalink / raw)
  To: Marcin Juszkiewicz; +Cc: Michael S. Tsirkin, qemu-devel, Marcel Apfelbaum

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.
> 



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-10-22  7:35 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-17 13:33 [PATCH] pcie: enable Extended tag field capability Marcin Juszkiewicz
2024-10-17 16:58 ` Michael S. Tsirkin
2024-10-17 19:18   ` Marcin Juszkiewicz
2024-10-18 10:24   ` Marcin Juszkiewicz
2024-10-22  7:34     ` Igor Mammedov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).