diff mbox series

[1/2] vDPA/ifcvf: record virtio notify base

Message ID 20210428082133.6766-2-lingshan.zhu@intel.com (mailing list archive)
State Not Applicable
Headers show
Series vDPA/ifcvf: implement doorbell mapping feature | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Zhu, Lingshan April 28, 2021, 8:21 a.m. UTC 
This commit records virtio notify base addr to implemente
doorbell mapping feature

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vdpa/ifcvf/ifcvf_base.c | 1 +
 drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
 2 files changed, 2 insertions(+)

Comments

Jason Wang April 28, 2021, 8:39 a.m. UTC | #1 
在 2021/4/28 下午4:21, Zhu Lingshan 写道:
> This commit records virtio notify base addr to implemente
> doorbell mapping feature
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>   drivers/vdpa/ifcvf/ifcvf_base.c | 1 +
>   drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
>   2 files changed, 2 insertions(+)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> index 1a661ab45af5..cc61a5bfc5b1 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> @@ -133,6 +133,7 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *pdev)
>   					      &hw->notify_off_multiplier);
>   			hw->notify_bar = cap.bar;
>   			hw->notify_base = get_cap_addr(hw, &cap);
> +			hw->notify_pa = pci_resource_start(pdev, cap.bar) + cap.offset;


To be more generic and avoid future changes, let's use the math defined 
in the virtio spec.

You may refer how it is implemented in virtio_pci vdpa driver[1].

Thanks

[1] 
https://lore.kernel.org/virtualization/20210415073147.19331-5-jasowang@redhat.com/T/


>   			IFCVF_DBG(pdev, "hw->notify_base = %p\n",
>   				  hw->notify_base);
>   			break;
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> index 0111bfdeb342..bcca7c1669dd 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> @@ -98,6 +98,7 @@ struct ifcvf_hw {
>   	char config_msix_name[256];
>   	struct vdpa_callback config_cb;
>   	unsigned int config_irq;
> +	phys_addr_t  notify_pa;
>   };
>   
>   struct ifcvf_adapter {
Zhu, Lingshan April 28, 2021, 10 a.m. UTC | #2 
On 4/28/2021 4:39 PM, Jason Wang wrote:
>
> 在 2021/4/28 下午4:21, Zhu Lingshan 写道:
>> This commit records virtio notify base addr to implemente
>> doorbell mapping feature
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vdpa/ifcvf/ifcvf_base.c | 1 +
>>   drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
>>   2 files changed, 2 insertions(+)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c 
>> b/drivers/vdpa/ifcvf/ifcvf_base.c
>> index 1a661ab45af5..cc61a5bfc5b1 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>> @@ -133,6 +133,7 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct 
>> pci_dev *pdev)
>> &hw->notify_off_multiplier);
>>               hw->notify_bar = cap.bar;
>>               hw->notify_base = get_cap_addr(hw, &cap);
>> +            hw->notify_pa = pci_resource_start(pdev, cap.bar) + 
>> cap.offset;
>
>
> To be more generic and avoid future changes, let's use the math 
> defined in the virtio spec.
>
> You may refer how it is implemented in virtio_pci vdpa driver[1].
Are you suggesting every vq keep its own notify_pa? In this case, we 
still need to record notify_pa in hw when init_hw, then initialize 
vq->notify_pa accrediting to hw->notify_pa.

Thanks
Zhu Lingshan
>
> Thanks
>
> [1] 
> https://lore.kernel.org/virtualization/20210415073147.19331-5-jasowang@redhat.com/T/
>
>
>> IFCVF_DBG(pdev, "hw->notify_base = %p\n",
>>                     hw->notify_base);
>>               break;
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>> index 0111bfdeb342..bcca7c1669dd 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>> @@ -98,6 +98,7 @@ struct ifcvf_hw {
>>       char config_msix_name[256];
>>       struct vdpa_callback config_cb;
>>       unsigned int config_irq;
>> +    phys_addr_t  notify_pa;
>>   };
>>     struct ifcvf_adapter {
>
Jason Wang April 28, 2021, 10:09 a.m. UTC | #3 
在 2021/4/28 下午6:00, Zhu, Lingshan 写道:
>
>
> On 4/28/2021 4:39 PM, Jason Wang wrote:
>>
>> 在 2021/4/28 下午4:21, Zhu Lingshan 写道:
>>> This commit records virtio notify base addr to implemente
>>> doorbell mapping feature
>>>
>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>> ---
>>>   drivers/vdpa/ifcvf/ifcvf_base.c | 1 +
>>>   drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
>>>   2 files changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c 
>>> b/drivers/vdpa/ifcvf/ifcvf_base.c
>>> index 1a661ab45af5..cc61a5bfc5b1 100644
>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>>> @@ -133,6 +133,7 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct 
>>> pci_dev *pdev)
>>> &hw->notify_off_multiplier);
>>>               hw->notify_bar = cap.bar;
>>>               hw->notify_base = get_cap_addr(hw, &cap);
>>> +            hw->notify_pa = pci_resource_start(pdev, cap.bar) + 
>>> cap.offset;
>>
>>
>> To be more generic and avoid future changes, let's use the math 
>> defined in the virtio spec.
>>
>> You may refer how it is implemented in virtio_pci vdpa driver[1].
> Are you suggesting every vq keep its own notify_pa? In this case, we 
> still need to record notify_pa in hw when init_hw, then initialize 
> vq->notify_pa accrediting to hw->notify_pa.


I meant you need to follow how virtio spec did to calculate the doorbell 
address per vq:

         cap.offset + queue_notify_off * notify_off_multiplier

Obviously, you ignore queue_notify_off and notify_off_multiplier here. 
This may bring troubles for the existing device IFCVF and future devices.

If I understand correctly, this device can be probed by virtio-pci 
driver which use the above math. There's no reason for using ad-hoc hack.

Thanks


>
> Thanks
> Zhu Lingshan
>>
>> Thanks
>>
>> [1] 
>> https://lore.kernel.org/virtualization/20210415073147.19331-5-jasowang@redhat.com/T/
>>
>>
>>> IFCVF_DBG(pdev, "hw->notify_base = %p\n",
>>>                     hw->notify_base);
>>>               break;
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>> index 0111bfdeb342..bcca7c1669dd 100644
>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>> @@ -98,6 +98,7 @@ struct ifcvf_hw {
>>>       char config_msix_name[256];
>>>       struct vdpa_callback config_cb;
>>>       unsigned int config_irq;
>>> +    phys_addr_t  notify_pa;
>>>   };
>>>     struct ifcvf_adapter {
>>
>
Zhu, Lingshan April 28, 2021, 10:18 a.m. UTC | #4 
On 4/28/2021 6:09 PM, Jason Wang wrote:
>
> 在 2021/4/28 下午6:00, Zhu, Lingshan 写道:
>>
>>
>> On 4/28/2021 4:39 PM, Jason Wang wrote:
>>>
>>> 在 2021/4/28 下午4:21, Zhu Lingshan 写道:
>>>> This commit records virtio notify base addr to implemente
>>>> doorbell mapping feature
>>>>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> ---
>>>>   drivers/vdpa/ifcvf/ifcvf_base.c | 1 +
>>>>   drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
>>>>   2 files changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c 
>>>> b/drivers/vdpa/ifcvf/ifcvf_base.c
>>>> index 1a661ab45af5..cc61a5bfc5b1 100644
>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>>>> @@ -133,6 +133,7 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct 
>>>> pci_dev *pdev)
>>>> &hw->notify_off_multiplier);
>>>>               hw->notify_bar = cap.bar;
>>>>               hw->notify_base = get_cap_addr(hw, &cap);
>>>> +            hw->notify_pa = pci_resource_start(pdev, cap.bar) + 
>>>> cap.offset;
>>>
>>>
>>> To be more generic and avoid future changes, let's use the math 
>>> defined in the virtio spec.
>>>
>>> You may refer how it is implemented in virtio_pci vdpa driver[1].
>> Are you suggesting every vq keep its own notify_pa? In this case, we 
>> still need to record notify_pa in hw when init_hw, then initialize 
>> vq->notify_pa accrediting to hw->notify_pa.
>
>
> I meant you need to follow how virtio spec did to calculate the 
> doorbell address per vq:
>
>         cap.offset + queue_notify_off * notify_off_multiplier
>
> Obviously, you ignore queue_notify_off and notify_off_multiplier here. 
> This may bring troubles for the existing device IFCVF and future devices.
>
> If I understand correctly, this device can be probed by virtio-pci 
> driver which use the above math. There's no reason for using ad-hoc hack.
sure, when talking about initialize vq->notify_pa, I mean calculate with 
with notify_base and multiplier, V2 will include this.

Thanks,
Zhu Lingshan
>
> Thanks
>
>
>>
>> Thanks
>> Zhu Lingshan
>>>
>>> Thanks
>>>
>>> [1] 
>>> https://lore.kernel.org/virtualization/20210415073147.19331-5-jasowang@redhat.com/T/
>>>
>>>
>>>> IFCVF_DBG(pdev, "hw->notify_base = %p\n",
>>>>                     hw->notify_base);
>>>>               break;
>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> index 0111bfdeb342..bcca7c1669dd 100644
>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> @@ -98,6 +98,7 @@ struct ifcvf_hw {
>>>>       char config_msix_name[256];
>>>>       struct vdpa_callback config_cb;
>>>>       unsigned int config_irq;
>>>> +    phys_addr_t  notify_pa;
>>>>   };
>>>>     struct ifcvf_adapter {
>>>
>>
>
kernel test robot April 28, 2021, 11:48 a.m. UTC | #5 
Hi Zhu,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.12 next-20210428]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Zhu-Lingshan/vDPA-ifcvf-implement-doorbell-mapping-feature/20210428-162843
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git acd3d28594536e9096c1ea76c5867d8a68babef6
config: x86_64-randconfig-s022-20210428 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/79474dca6dd202684180ad6ea88b5f73fc4a87c2
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Zhu-Lingshan/vDPA-ifcvf-implement-doorbell-mapping-feature/20210428-162843
        git checkout 79474dca6dd202684180ad6ea88b5f73fc4a87c2
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/vdpa/ifcvf/ifcvf_base.c:136:80: sparse: sparse: restricted __le32 degrades to integer

vim +136 drivers/vdpa/ifcvf/ifcvf_base.c

    98	
    99	int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *pdev)
   100	{
   101		struct virtio_pci_cap cap;
   102		u16 notify_off;
   103		int ret;
   104		u8 pos;
   105		u32 i;
   106	
   107		ret = pci_read_config_byte(pdev, PCI_CAPABILITY_LIST, &pos);
   108		if (ret < 0) {
   109			IFCVF_ERR(pdev, "Failed to read PCI capability list\n");
   110			return -EIO;
   111		}
   112	
   113		while (pos) {
   114			ret = ifcvf_read_config_range(pdev, (u32 *)&cap,
   115						      sizeof(cap), pos);
   116			if (ret < 0) {
   117				IFCVF_ERR(pdev,
   118					  "Failed to get PCI capability at %x\n", pos);
   119				break;
   120			}
   121	
   122			if (cap.cap_vndr != PCI_CAP_ID_VNDR)
   123				goto next;
   124	
   125			switch (cap.cfg_type) {
   126			case VIRTIO_PCI_CAP_COMMON_CFG:
   127				hw->common_cfg = get_cap_addr(hw, &cap);
   128				IFCVF_DBG(pdev, "hw->common_cfg = %p\n",
   129					  hw->common_cfg);
   130				break;
   131			case VIRTIO_PCI_CAP_NOTIFY_CFG:
   132				pci_read_config_dword(pdev, pos + sizeof(cap),
   133						      &hw->notify_off_multiplier);
   134				hw->notify_bar = cap.bar;
   135				hw->notify_base = get_cap_addr(hw, &cap);
 > 136				hw->notify_pa = pci_resource_start(pdev, cap.bar) + cap.offset;
   137				IFCVF_DBG(pdev, "hw->notify_base = %p\n",
   138					  hw->notify_base);
   139				break;
   140			case VIRTIO_PCI_CAP_ISR_CFG:
   141				hw->isr = get_cap_addr(hw, &cap);
   142				IFCVF_DBG(pdev, "hw->isr = %p\n", hw->isr);
   143				break;
   144			case VIRTIO_PCI_CAP_DEVICE_CFG:
   145				hw->net_cfg = get_cap_addr(hw, &cap);
   146				IFCVF_DBG(pdev, "hw->net_cfg = %p\n", hw->net_cfg);
   147				break;
   148			}
   149	
   150	next:
   151			pos = cap.cap_next;
   152		}
   153	
   154		if (hw->common_cfg == NULL || hw->notify_base == NULL ||
   155		    hw->isr == NULL || hw->net_cfg == NULL) {
   156			IFCVF_ERR(pdev, "Incomplete PCI capabilities\n");
   157			return -EIO;
   158		}
   159	
   160		for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++) {
   161			ifc_iowrite16(i, &hw->common_cfg->queue_select);
   162			notify_off = ifc_ioread16(&hw->common_cfg->queue_notify_off);
   163			hw->vring[i].notify_addr = hw->notify_base +
   164				notify_off * hw->notify_off_multiplier;
   165		}
   166	
   167		hw->lm_cfg = hw->base[IFCVF_LM_BAR];
   168	
   169		IFCVF_DBG(pdev,
   170			  "PCI capability mapping: common cfg: %p, notify base: %p\n, isr cfg: %p, device cfg: %p, multiplier: %u\n",
   171			  hw->common_cfg, hw->notify_base, hw->isr,
   172			  hw->net_cfg, hw->notify_off_multiplier);
   173	
   174		return 0;
   175	}
   176	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
index 1a661ab45af5..cc61a5bfc5b1 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.c
+++ b/drivers/vdpa/ifcvf/ifcvf_base.c
@@ -133,6 +133,7 @@  int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *pdev)
 					      &hw->notify_off_multiplier);
 			hw->notify_bar = cap.bar;
 			hw->notify_base = get_cap_addr(hw, &cap);
+			hw->notify_pa = pci_resource_start(pdev, cap.bar) + cap.offset;
 			IFCVF_DBG(pdev, "hw->notify_base = %p\n",
 				  hw->notify_base);
 			break;
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
index 0111bfdeb342..bcca7c1669dd 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -98,6 +98,7 @@  struct ifcvf_hw {
 	char config_msix_name[256];
 	struct vdpa_callback config_cb;
 	unsigned int config_irq;
+	phys_addr_t  notify_pa;
 };
 
 struct ifcvf_adapter {