[04/13] pcie: Introduce function for DSN capability creation

From: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>

Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
---
 hw/pci/pcie.c              | 7 +++++++
 include/hw/pci/pcie.h      | 1 +
 include/hw/pci/pcie_regs.h | 4 ++++
 3 files changed, 12 insertions(+)

On Thu, Feb 18, 2016 at 12:07:25PM +0200, Leonid Bloch wrote:
> From: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
> 
> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
> Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
> ---
>  hw/pci/pcie.c              | 7 +++++++
>  include/hw/pci/pcie.h      | 1 +
>  include/hw/pci/pcie_regs.h | 4 ++++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index be3a318..f7ac7d4 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -690,3 +690,10 @@ void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn)
>                          offset, PCI_ARI_SIZEOF);
>      pci_set_long(dev->config + offset + PCI_ARI_CAP, (nextfn & 0xff) << 8);
>  }
> +
> +void pcie_dsn_init(PCIDevice *dev, uint16_t offset, uint64_t val)
> +{
> +    pcie_add_capability(dev, PCI_EXT_CAP_ID_DSN, PCI_DSN_VER,
> +                        offset, PCI_EXT_CAP_DSN_SIZEOF);
> +    pci_set_quad(dev->config + offset + PCI_DSN_CAP, val);
> +}
> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> index cbbf0c5..83a325c 100644
> --- a/include/hw/pci/pcie.h
> +++ b/include/hw/pci/pcie.h
> @@ -119,6 +119,7 @@ void pcie_add_capability(PCIDevice *dev,
>                           uint16_t offset, uint16_t size);
>  
>  void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
> +void pcie_dsn_init(PCIDevice *dev, uint16_t offset, uint64_t val);


val is not a good parameter name.
Please make it meaningful. E.g. serial_number.

I would also write device_serial_number out fully
since PCI spec does not say DSN anywhere.

>  
>  extern const VMStateDescription vmstate_pcie_device;
>  
> diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
> index a95522a..a891a45 100644
> --- a/include/hw/pci/pcie_regs.h
> +++ b/include/hw/pci/pcie_regs.h
> @@ -79,6 +79,10 @@
>  #define PCI_ARI_VER                     1
>  #define PCI_ARI_SIZEOF                  8
>  
> +/* DSN */
> +#define PCI_DSN_VER                     1
> +#define PCI_DSN_CAP                     0x04
> +
>  /* AER */
>  #define PCI_ERR_VER                     2
>  #define PCI_ERR_SIZEOF                  0x48

Again no need for this: you have a wrapper so
they will never be used anywhere else. Just document it well where
you use these numbers.

> -- 
> 2.5.0

On Thu, Feb 18, 2016 at 12:41 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Feb 18, 2016 at 12:07:25PM +0200, Leonid Bloch wrote:
>> From: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
>>
>> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com>
>> Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com>
>> ---
>>  hw/pci/pcie.c              | 7 +++++++
>>  include/hw/pci/pcie.h      | 1 +
>>  include/hw/pci/pcie_regs.h | 4 ++++
>>  3 files changed, 12 insertions(+)
>>
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index be3a318..f7ac7d4 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -690,3 +690,10 @@ void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn)
>>                          offset, PCI_ARI_SIZEOF);
>>      pci_set_long(dev->config + offset + PCI_ARI_CAP, (nextfn & 0xff) << 8);
>>  }
>> +
>> +void pcie_dsn_init(PCIDevice *dev, uint16_t offset, uint64_t val)
>> +{
>> +    pcie_add_capability(dev, PCI_EXT_CAP_ID_DSN, PCI_DSN_VER,
>> +                        offset, PCI_EXT_CAP_DSN_SIZEOF);
>> +    pci_set_quad(dev->config + offset + PCI_DSN_CAP, val);
>> +}
>> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
>> index cbbf0c5..83a325c 100644
>> --- a/include/hw/pci/pcie.h
>> +++ b/include/hw/pci/pcie.h
>> @@ -119,6 +119,7 @@ void pcie_add_capability(PCIDevice *dev,
>>                           uint16_t offset, uint16_t size);
>>
>>  void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
>> +void pcie_dsn_init(PCIDevice *dev, uint16_t offset, uint64_t val);
>
>
> val is not a good parameter name.
> Please make it meaningful. E.g. serial_number.
>
> I would also write device_serial_number out fully
> since PCI spec does not say DSN anywhere.

Changed to dev_ser_num and ser_num.

>
>>
>>  extern const VMStateDescription vmstate_pcie_device;
>>
>> diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
>> index a95522a..a891a45 100644
>> --- a/include/hw/pci/pcie_regs.h
>> +++ b/include/hw/pci/pcie_regs.h
>> @@ -79,6 +79,10 @@
>>  #define PCI_ARI_VER                     1
>>  #define PCI_ARI_SIZEOF                  8
>>
>> +/* DSN */
>> +#define PCI_DSN_VER                     1
>> +#define PCI_DSN_CAP                     0x04
>> +
>>  /* AER */
>>  #define PCI_ERR_VER                     2
>>  #define PCI_ERR_SIZEOF                  0x48
>
> Again no need for this: you have a wrapper so
> they will never be used anywhere else. Just document it well where
> you use these numbers.

Switched to static const ints in the function itself. There the "dsn"
remained (pci_dsn_ver, pci_dsn_cap), because they are used only inside
of a function that deals with the device serial number, so it should
be clear enough.

>
>> --
>> 2.5.0