Re: [PATCH 1/2] CXL/cxl_type3: add first_dvsec_offset() helper

2024-04-02 Thread Jonathan Cameron via 
On Tue,  2 Apr 2024 09:46:46 +0800
Li Zhijian  wrote:

> It helps to figure out where the first dvsec register is located. In
> addition, replace offset and size hardcore with existing macros.
> 
> Signed-off-by: Li Zhijian 

I agree we should be using the macros.

The offset calc is a bit specific to the the chosen memory layout,
so not sure it makes sense to break it out to a separate function.

I'll suggest alternative possible approaches in review of next patch.

Jonathan

> ---
>  hw/mem/cxl_type3.c | 19 +--
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index b0a7e9f11b64..ad2fe7d463fb 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -643,6 +643,16 @@ static DOEProtocol doe_cdat_prot[] = {
>  { }
>  };
>  
> +static uint16_t first_dvsec_offset(CXLType3Dev *ct3d)
> +{
> +uint16_t offset = PCI_CONFIG_SPACE_SIZE;
> +
> +if (ct3d->sn != UI64_NULL)
> +offset += PCI_EXT_CAP_DSN_SIZEOF;
> +
> +return offset;
> +}
> +
>  static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>  {
>  ERRP_GUARD();
> @@ -663,13 +673,10 @@ static void ct3_realize(PCIDevice *pci_dev, Error 
> **errp)
>  pci_config_set_prog_interface(pci_conf, 0x10);
>  
>  pcie_endpoint_cap_init(pci_dev, 0x80);
> -if (ct3d->sn != UI64_NULL) {
> -pcie_dev_ser_num_init(pci_dev, 0x100, ct3d->sn);
> -cxl_cstate->dvsec_offset = 0x100 + 0x0c;
> -} else {
> -cxl_cstate->dvsec_offset = 0x100;
> -}
> +if (ct3d->sn != UI64_NULL)
> +pcie_dev_ser_num_init(pci_dev, PCI_CONFIG_SPACE_SIZE, ct3d->sn);
>  
> +cxl_cstate->dvsec_offset = first_dvsec_offset(ct3d);
>  ct3d->cxl_cstate.pdev = pci_dev;
>  build_dvsecs(ct3d);
>

Re: [PATCH 1/2] CXL/cxl_type3: add first_dvsec_offset() helper

2024-04-01 Thread Zhijian Li (Fujitsu) 


On 02/04/2024 12:09, fan wrote:
> On Tue, Apr 02, 2024 at 09:46:46AM +0800, Li Zhijian via wrote:
>> It helps to figure out where the first dvsec register is located. In
>> addition, replace offset and size hardcore with existing macros.
>>
>> Signed-off-by: Li Zhijian 
>> ---
>>   hw/mem/cxl_type3.c | 19 +--
>>   1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
>> index b0a7e9f11b64..ad2fe7d463fb 100644
>> --- a/hw/mem/cxl_type3.c
>> +++ b/hw/mem/cxl_type3.c
>> @@ -643,6 +643,16 @@ static DOEProtocol doe_cdat_prot[] = {
>>   { }
>>   };
>>   
>> +static uint16_t first_dvsec_offset(CXLType3Dev *ct3d)
>> +{
>> +uint16_t offset = PCI_CONFIG_SPACE_SIZE;
>> +
>> +if (ct3d->sn != UI64_NULL)
>> +offset += PCI_EXT_CAP_DSN_SIZEOF;
>> +
>> +return offset;
>> +}
>> +
>>   static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>>   {
>>   ERRP_GUARD();
>> @@ -663,13 +673,10 @@ static void ct3_realize(PCIDevice *pci_dev, Error 
>> **errp)
>>   pci_config_set_prog_interface(pci_conf, 0x10);
>>   
>>   pcie_endpoint_cap_init(pci_dev, 0x80);
>> -if (ct3d->sn != UI64_NULL) {
>> -pcie_dev_ser_num_init(pci_dev, 0x100, ct3d->sn);
>> -cxl_cstate->dvsec_offset = 0x100 + 0x0c;
>> -} else {
>> -cxl_cstate->dvsec_offset = 0x100;
>> -}
>> +if (ct3d->sn != UI64_NULL)
>> +pcie_dev_ser_num_init(pci_dev, PCI_CONFIG_SPACE_SIZE, ct3d->sn);
>>   
>> +cxl_cstate->dvsec_offset = first_dvsec_offset(ct3d);
>>   ct3d->cxl_cstate.pdev = pci_dev;
>>   build_dvsecs(ct3d);
>>   
>> -- 
>> 2.29.2
>>
>>
> Hi Zhijian,
> 
> Please use Qemu's checkpatch tool to make sure the patches meet the
> qemu code format requirement.


My mistake, any other input for these 2 patches?



> Also, please cc linux-...@vger.kernel.org if the code is cxl related.

Thanks for your remainder, do you mind if I send a patch to add the
"L: linux-...@vger.kernel.org' field to the CXL entry


Thanks
Zhijian
> 

> Fan

Re: [PATCH 1/2] CXL/cxl_type3: add first_dvsec_offset() helper

2024-04-01 Thread fan 
On Tue, Apr 02, 2024 at 09:46:46AM +0800, Li Zhijian via wrote:
> It helps to figure out where the first dvsec register is located. In
> addition, replace offset and size hardcore with existing macros.
> 
> Signed-off-by: Li Zhijian 
> ---
>  hw/mem/cxl_type3.c | 19 +--
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index b0a7e9f11b64..ad2fe7d463fb 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -643,6 +643,16 @@ static DOEProtocol doe_cdat_prot[] = {
>  { }
>  };
>  
> +static uint16_t first_dvsec_offset(CXLType3Dev *ct3d)
> +{
> +uint16_t offset = PCI_CONFIG_SPACE_SIZE;
> +
> +if (ct3d->sn != UI64_NULL)
> +offset += PCI_EXT_CAP_DSN_SIZEOF;
> +
> +return offset;
> +}
> +
>  static void ct3_realize(PCIDevice *pci_dev, Error **errp)
>  {
>  ERRP_GUARD();
> @@ -663,13 +673,10 @@ static void ct3_realize(PCIDevice *pci_dev, Error 
> **errp)
>  pci_config_set_prog_interface(pci_conf, 0x10);
>  
>  pcie_endpoint_cap_init(pci_dev, 0x80);
> -if (ct3d->sn != UI64_NULL) {
> -pcie_dev_ser_num_init(pci_dev, 0x100, ct3d->sn);
> -cxl_cstate->dvsec_offset = 0x100 + 0x0c;
> -} else {
> -cxl_cstate->dvsec_offset = 0x100;
> -}
> +if (ct3d->sn != UI64_NULL)
> +pcie_dev_ser_num_init(pci_dev, PCI_CONFIG_SPACE_SIZE, ct3d->sn);
>  
> +cxl_cstate->dvsec_offset = first_dvsec_offset(ct3d);
>  ct3d->cxl_cstate.pdev = pci_dev;
>  build_dvsecs(ct3d);
>  
> -- 
> 2.29.2
> 
> 
Hi Zhijian,

Please use Qemu's checkpatch tool to make sure the patches meet the
qemu code format requirement.
Also, please cc linux-...@vger.kernel.org if the code is cxl related.

Fan

[PATCH 1/2] CXL/cxl_type3: add first_dvsec_offset() helper

2024-04-01 Thread Li Zhijian via 
It helps to figure out where the first dvsec register is located. In
addition, replace offset and size hardcore with existing macros.

Signed-off-by: Li Zhijian 
---
 hw/mem/cxl_type3.c | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index b0a7e9f11b64..ad2fe7d463fb 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -643,6 +643,16 @@ static DOEProtocol doe_cdat_prot[] = {
 { }
 };
 
+static uint16_t first_dvsec_offset(CXLType3Dev *ct3d)
+{
+uint16_t offset = PCI_CONFIG_SPACE_SIZE;
+
+if (ct3d->sn != UI64_NULL)
+offset += PCI_EXT_CAP_DSN_SIZEOF;
+
+return offset;
+}
+
 static void ct3_realize(PCIDevice *pci_dev, Error **errp)
 {
 ERRP_GUARD();
@@ -663,13 +673,10 @@ static void ct3_realize(PCIDevice *pci_dev, Error **errp)
 pci_config_set_prog_interface(pci_conf, 0x10);
 
 pcie_endpoint_cap_init(pci_dev, 0x80);
-if (ct3d->sn != UI64_NULL) {
-pcie_dev_ser_num_init(pci_dev, 0x100, ct3d->sn);
-cxl_cstate->dvsec_offset = 0x100 + 0x0c;
-} else {
-cxl_cstate->dvsec_offset = 0x100;
-}
+if (ct3d->sn != UI64_NULL)
+pcie_dev_ser_num_init(pci_dev, PCI_CONFIG_SPACE_SIZE, ct3d->sn);
 
+cxl_cstate->dvsec_offset = first_dvsec_offset(ct3d);
 ct3d->cxl_cstate.pdev = pci_dev;
 build_dvsecs(ct3d);
 
-- 
2.29.2