Re: [PATCH v5 cxl2.0-v3-doe 2/6] include/hw/pci: headers for PCIe DOE

[Jonathan Cameron]

On Mon, 26 Apr 2021 13:16:43 -0400
Chris Browy <cbrowy@avery-design.com> wrote:

> From: hchkuo <hchkuo@avery-design.com.tw>
> 
> Macros for the vender ID of PCI-SIG and the size of PCIe Data Object
> Exchange.

The PCI SIG vendor ID is a little tricky to track down as it's only
called out indirectly in PCI specs.

In a similar fashion to what Bjorn asked for in the kernel code, perhaps
a reference to where it is in the DOE ECN would at least reassure people
that 0x0001 definitely is the PCI-SIG?

> 
> Signed-off-by: Chris Browy <cbrowy@avery-design.com>
One comment inline.

Jonathan

> ---
>  include/hw/pci/pci_ids.h   | 2 ++
>  include/hw/pci/pcie_regs.h | 3 +++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
> index 95f92d98e9..471c915395 100644
> --- a/include/hw/pci/pci_ids.h
> +++ b/include/hw/pci/pci_ids.h
> @@ -157,6 +157,8 @@
>  
>  /* Vendors and devices.  Sort key: vendor first, device next. */
>  
> +#define PCI_VENDOR_ID_PCI_SIG            0x0001
> +
>  #define PCI_VENDOR_ID_LSI_LOGIC          0x1000
>  #define PCI_DEVICE_ID_LSI_53C810         0x0001
>  #define PCI_DEVICE_ID_LSI_53C895A        0x0012
> diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
> index 1db86b0ec4..5ec7014211 100644
> --- a/include/hw/pci/pcie_regs.h
> +++ b/include/hw/pci/pcie_regs.h
> @@ -179,4 +179,7 @@ typedef enum PCIExpLinkWidth {
>  #define PCI_ACS_VER                     0x1
>  #define PCI_ACS_SIZEOF                  8
>  
> +/* DOE Capability Register Fields */
> +#define PCI_DOE_SIZEOF                  24
I wonder if we should do the same thing as for other cases above and also
have the PCI_DOE_VER defined here?

In theory the SIZEOF only refers to the current version of DOE - it might get
bigger in future...

Jonathan

> +
>  #endif /* QEMU_PCIE_REGS_H */