[PATCH 0/2] vDPA/ifcvf: implement doorbell mapping feature

[PATCH 0/2] vDPA/ifcvf: implement doorbell mapping feature

[Zhu Lingshan]

This series implements doorbell mapping feature for ifcvf.

Please help review

Thanks!

Zhu Lingshan (2):
  vDPA/ifcvf: record virtio notify base
  vDPA/ifcvf: implement doorbell mapping for ifcvf

 drivers/vdpa/ifcvf/ifcvf_base.c |  1 +
 drivers/vdpa/ifcvf/ifcvf_base.h |  1 +
 drivers/vdpa/ifcvf/ifcvf_main.c | 18 ++++++++++++++++++
 3 files changed, 20 insertions(+)

-- 
2.27.0

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

[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;
 			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 {
-- 
2.27.0

[PATCH 2/2] vDPA/ifcvf: implement doorbell mapping for ifcvf

[Zhu Lingshan]

This commit implements doorbell mapping feature for ifcvf.
This feature maps the notify page to userspace, to eliminate
vmexit when kick a vq.

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

diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index e48e6b74fe2e..afcb71bc0f51 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -413,6 +413,23 @@ static int ifcvf_vdpa_get_vq_irq(struct vdpa_device *vdpa_dev,
 	return vf->vring[qid].irq;
 }
 
+static struct vdpa_notification_area ifcvf_get_vq_notification(struct vdpa_device *vdpa_dev,
+							       u16 idx)
+{
+	struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
+	struct vdpa_notification_area area;
+
+	if (vf->notify_pa % PAGE_SIZE) {
+		area.addr = 0;
+		area.size = 0;
+	} else {
+		area.addr = vf->notify_pa;
+		area.size = PAGE_SIZE;
+	}
+
+	return area;
+}
+
 /*
  * IFCVF currently does't have on-chip IOMMU, so not
  * implemented set_map()/dma_map()/dma_unmap()
@@ -440,6 +457,7 @@ static const struct vdpa_config_ops ifc_vdpa_ops = {
 	.get_config	= ifcvf_vdpa_get_config,
 	.set_config	= ifcvf_vdpa_set_config,
 	.set_config_cb  = ifcvf_vdpa_set_config_cb,
+	.get_vq_notification = ifcvf_get_vq_notification,
 };
 
 static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
-- 
2.27.0

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

[Jason Wang]

在 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 {

Re: [PATCH 2/2] vDPA/ifcvf: implement doorbell mapping for ifcvf

[Jason Wang]

在 2021/4/28 下午4:21, Zhu Lingshan 写道:
> This commit implements doorbell mapping feature for ifcvf.
> This feature maps the notify page to userspace, to eliminate
> vmexit when kick a vq.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>   drivers/vdpa/ifcvf/ifcvf_main.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index e48e6b74fe2e..afcb71bc0f51 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -413,6 +413,23 @@ static int ifcvf_vdpa_get_vq_irq(struct vdpa_device *vdpa_dev,
>   	return vf->vring[qid].irq;
>   }
>   
> +static struct vdpa_notification_area ifcvf_get_vq_notification(struct vdpa_device *vdpa_dev,
> +							       u16 idx)
> +{
> +	struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> +	struct vdpa_notification_area area;
> +
> +	if (vf->notify_pa % PAGE_SIZE) {
> +		area.addr = 0;
> +		area.size = 0;


We don't need this since:

1) there's a check in the vhost vDPA
2) device is unaware of the bound driver, non page aligned doorbell 
doesn't necessarily meant it can be used

Let's leave those polices to the driver.

Thanks


> +	} else {
> +		area.addr = vf->notify_pa;
> +		area.size = PAGE_SIZE;
> +	}
> +
> +	return area;
> +}
> +
>   /*
>    * IFCVF currently does't have on-chip IOMMU, so not
>    * implemented set_map()/dma_map()/dma_unmap()
> @@ -440,6 +457,7 @@ static const struct vdpa_config_ops ifc_vdpa_ops = {
>   	.get_config	= ifcvf_vdpa_get_config,
>   	.set_config	= ifcvf_vdpa_set_config,
>   	.set_config_cb  = ifcvf_vdpa_set_config_cb,
> +	.get_vq_notification = ifcvf_get_vq_notification,
>   };
>   
>   static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)

Re: [PATCH 2/2] vDPA/ifcvf: implement doorbell mapping for ifcvf

[Zhu, Lingshan]

On 4/28/2021 4:42 PM, Jason Wang wrote:
>
> 在 2021/4/28 下午4:21, Zhu Lingshan 写道:
>> This commit implements doorbell mapping feature for ifcvf.
>> This feature maps the notify page to userspace, to eliminate
>> vmexit when kick a vq.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vdpa/ifcvf/ifcvf_main.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index e48e6b74fe2e..afcb71bc0f51 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -413,6 +413,23 @@ static int ifcvf_vdpa_get_vq_irq(struct 
>> vdpa_device *vdpa_dev,
>>       return vf->vring[qid].irq;
>>   }
>>   +static struct vdpa_notification_area 
>> ifcvf_get_vq_notification(struct vdpa_device *vdpa_dev,
>> +                                   u16 idx)
>> +{
>> +    struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>> +    struct vdpa_notification_area area;
>> +
>> +    if (vf->notify_pa % PAGE_SIZE) {
>> +        area.addr = 0;
>> +        area.size = 0;
>
>
> We don't need this since:
>
> 1) there's a check in the vhost vDPA
I think you mean this code block in vdpa.c
         notify = ops->get_vq_notification(vdpa, index);
         if (notify.addr & (PAGE_SIZE - 1))
                 return -EINVAL;

This should work, however, I think the parent driver should ensure it 
passes a PAGE_SIZE aligned address to userspace, to be robust, to be 
reliable.
> 2) device is unaware of the bound driver, non page aligned doorbell 
> doesn't necessarily meant it can be used
Yes, non page aligned doorbell can not be used, so there is a check.

Thanks
Zhu Lingshan
>
> Let's leave those polices to the driver.
>
> Thanks
>
>
>> +    } else {
>> +        area.addr = vf->notify_pa;
>> +        area.size = PAGE_SIZE;
>> +    }
>> +
>> +    return area;
>> +}
>> +
>>   /*
>>    * IFCVF currently does't have on-chip IOMMU, so not
>>    * implemented set_map()/dma_map()/dma_unmap()
>> @@ -440,6 +457,7 @@ static const struct vdpa_config_ops ifc_vdpa_ops = {
>>       .get_config    = ifcvf_vdpa_get_config,
>>       .set_config    = ifcvf_vdpa_set_config,
>>       .set_config_cb  = ifcvf_vdpa_set_config_cb,
>> +    .get_vq_notification = ifcvf_get_vq_notification,
>>   };
>>     static int ifcvf_probe(struct pci_dev *pdev, const struct 
>> pci_device_id *id)
>

Re: [PATCH 2/2] vDPA/ifcvf: implement doorbell mapping for ifcvf

[Jason Wang]

在 2021/4/28 下午4:59, Zhu, Lingshan 写道:
>
>
> On 4/28/2021 4:42 PM, Jason Wang wrote:
>>
>> 在 2021/4/28 下午4:21, Zhu Lingshan 写道:
>>> This commit implements doorbell mapping feature for ifcvf.
>>> This feature maps the notify page to userspace, to eliminate
>>> vmexit when kick a vq.
>>>
>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>> ---
>>>   drivers/vdpa/ifcvf/ifcvf_main.c | 18 ++++++++++++++++++
>>>   1 file changed, 18 insertions(+)
>>>
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> index e48e6b74fe2e..afcb71bc0f51 100644
>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> @@ -413,6 +413,23 @@ static int ifcvf_vdpa_get_vq_irq(struct 
>>> vdpa_device *vdpa_dev,
>>>       return vf->vring[qid].irq;
>>>   }
>>>   +static struct vdpa_notification_area 
>>> ifcvf_get_vq_notification(struct vdpa_device *vdpa_dev,
>>> +                                   u16 idx)
>>> +{
>>> +    struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>> +    struct vdpa_notification_area area;
>>> +
>>> +    if (vf->notify_pa % PAGE_SIZE) {
>>> +        area.addr = 0;
>>> +        area.size = 0;
>>
>>
>> We don't need this since:
>>
>> 1) there's a check in the vhost vDPA
> I think you mean this code block in vdpa.c
>         notify = ops->get_vq_notification(vdpa, index);
>         if (notify.addr & (PAGE_SIZE - 1))
>                 return -EINVAL;
>
> This should work, however, I think the parent driver should ensure it 
> passes a PAGE_SIZE aligned address to userspace, to be robust, to be 
> reliable.


The point is parent is unaware of whether or not there's a userspace.


>> 2) device is unaware of the bound driver, non page aligned doorbell 
>> doesn't necessarily meant it can be used
> Yes, non page aligned doorbell can not be used, so there is a check.


Typo, what I meant is "it can't be used". That is to say, we should let 
the vDPA bus driver to decide whether or not it can be used.

Thanks


>
> Thanks
> Zhu Lingshan
>>
>> Let's leave those polices to the driver.
>>
>> Thanks
>>
>>
>>> +    } else {
>>> +        area.addr = vf->notify_pa;
>>> +        area.size = PAGE_SIZE;
>>> +    }
>>> +
>>> +    return area;
>>> +}
>>> +
>>>   /*
>>>    * IFCVF currently does't have on-chip IOMMU, so not
>>>    * implemented set_map()/dma_map()/dma_unmap()
>>> @@ -440,6 +457,7 @@ static const struct vdpa_config_ops ifc_vdpa_ops ={
>>>       .get_config    = ifcvf_vdpa_get_config,
>>>       .set_config    = ifcvf_vdpa_set_config,
>>>       .set_config_cb  = ifcvf_vdpa_set_config_cb,
>>> +    .get_vq_notification = ifcvf_get_vq_notification,
>>>   };
>>>     static int ifcvf_probe(struct pci_dev *pdev, const struct 
>>> pci_device_id *id)
>>
>

Re: [PATCH 2/2] vDPA/ifcvf: implement doorbell mapping for ifcvf

[Zhu, Lingshan]

On 4/28/2021 5:21 PM, Jason Wang wrote:
>
> 在 2021/4/28 下午4:59, Zhu, Lingshan 写道:
>>
>>
>> On 4/28/2021 4:42 PM, Jason Wang wrote:
>>>
>>> 在 2021/4/28 下午4:21, Zhu Lingshan 写道:
>>>> This commit implements doorbell mapping feature for ifcvf.
>>>> This feature maps the notify page to userspace, to eliminate
>>>> vmexit when kick a vq.
>>>>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> ---
>>>>   drivers/vdpa/ifcvf/ifcvf_main.c | 18 ++++++++++++++++++
>>>>   1 file changed, 18 insertions(+)
>>>>
>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> index e48e6b74fe2e..afcb71bc0f51 100644
>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> @@ -413,6 +413,23 @@ static int ifcvf_vdpa_get_vq_irq(struct 
>>>> vdpa_device *vdpa_dev,
>>>>       return vf->vring[qid].irq;
>>>>   }
>>>>   +static struct vdpa_notification_area 
>>>> ifcvf_get_vq_notification(struct vdpa_device *vdpa_dev,
>>>> +                                   u16 idx)
>>>> +{
>>>> +    struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>> +    struct vdpa_notification_area area;
>>>> +
>>>> +    if (vf->notify_pa % PAGE_SIZE) {
>>>> +        area.addr = 0;
>>>> +        area.size = 0;
>>>
>>>
>>> We don't need this since:
>>>
>>> 1) there's a check in the vhost vDPA
>> I think you mean this code block in vdpa.c
>>         notify = ops->get_vq_notification(vdpa, index);
>>         if (notify.addr & (PAGE_SIZE - 1))
>>                 return -EINVAL;
>>
>> This should work, however, I think the parent driver should ensure it 
>> passes a PAGE_SIZE aligned address to userspace, to be robust, to be 
>> reliable.
>
>
> The point is parent is unaware of whether or not there's a userspace.
when calling this, I think it targets a usersapce program, why kernel 
space need it, so IMHO no harm if we check this to keep the parent 
driver robust.
>
>
>>> 2) device is unaware of the bound driver, non page aligned doorbell 
>>> doesn't necessarily meant it can be used
>> Yes, non page aligned doorbell can not be used, so there is a check.
>
>
> Typo, what I meant is "it can't be used". That is to say, we should 
> let the vDPA bus driver to decide whether or not it can be used.
If it is not page aligned, there would be extra complexities for 
vhost/qemu, I see it as a hardware defect, why adapt to this kind of 
defects?

Thanks
Zhu Lingshan
>
> Thanks
>
>
>>
>> Thanks
>> Zhu Lingshan
>>>
>>> Let's leave those polices to the driver.
>>>
>>> Thanks
>>>
>>>
>>>> +    } else {
>>>> +        area.addr = vf->notify_pa;
>>>> +        area.size = PAGE_SIZE;
>>>> +    }
>>>> +
>>>> +    return area;
>>>> +}
>>>> +
>>>>   /*
>>>>    * IFCVF currently does't have on-chip IOMMU, so not
>>>>    * implemented set_map()/dma_map()/dma_unmap()
>>>> @@ -440,6 +457,7 @@ static const struct vdpa_config_ops 
>>>> ifc_vdpa_ops ={
>>>>       .get_config    = ifcvf_vdpa_get_config,
>>>>       .set_config    = ifcvf_vdpa_set_config,
>>>>       .set_config_cb  = ifcvf_vdpa_set_config_cb,
>>>> +    .get_vq_notification = ifcvf_get_vq_notification,
>>>>   };
>>>>     static int ifcvf_probe(struct pci_dev *pdev, const struct 
>>>> pci_device_id *id)
>>>
>>
>

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

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

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

Re: [PATCH 2/2] vDPA/ifcvf: implement doorbell mapping for ifcvf

[Jason Wang]

在 2021/4/28 下午5:56, Zhu, Lingshan 写道:
>
>
> On 4/28/2021 5:21 PM, Jason Wang wrote:
>>
>> 在 2021/4/28 下午4:59, Zhu, Lingshan 写道:
>>>
>>>
>>> On 4/28/2021 4:42 PM, Jason Wang wrote:
>>>>
>>>> 在 2021/4/28 下午4:21, Zhu Lingshan 写道:
>>>>> This commit implements doorbell mapping feature for ifcvf.
>>>>> This feature maps the notify page to userspace, to eliminate
>>>>> vmexit when kick a vq.
>>>>>
>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>> ---
>>>>>   drivers/vdpa/ifcvf/ifcvf_main.c | 18 ++++++++++++++++++
>>>>>   1 file changed, 18 insertions(+)
>>>>>
>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> index e48e6b74fe2e..afcb71bc0f51 100644
>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> @@ -413,6 +413,23 @@ static int ifcvf_vdpa_get_vq_irq(struct 
>>>>> vdpa_device *vdpa_dev,
>>>>>       return vf->vring[qid].irq;
>>>>>   }
>>>>>   +static struct vdpa_notification_area 
>>>>> ifcvf_get_vq_notification(struct vdpa_device *vdpa_dev,
>>>>> +                                   u16 idx)
>>>>> +{
>>>>> +    struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>> +    struct vdpa_notification_area area;
>>>>> +
>>>>> +    if (vf->notify_pa % PAGE_SIZE) {
>>>>> +        area.addr = 0;
>>>>> +        area.size = 0;
>>>>
>>>>
>>>> We don't need this since:
>>>>
>>>> 1) there's a check in the vhost vDPA
>>> I think you mean this code block in vdpa.c
>>>         notify = ops->get_vq_notification(vdpa, index);
>>>         if (notify.addr & (PAGE_SIZE - 1))
>>>                 return -EINVAL;
>>>
>>> This should work, however, I think the parent driver should ensure 
>>> it passes a PAGE_SIZE aligned address to userspace, to be robust, to 
>>> be reliable.
>>
>>
>> The point is parent is unaware of whether or not there's a userspace.
> when calling this, I think it targets a usersapce program, why kernel 
> space need it, so IMHO no harm if we check this to keep the parent 
> driver robust.


Again, vDPA device is unaware of what driver that is bound. It could be 
virtio-vpda, vhost-vdpa or other in the future. It's only the vDPA bus 
driver know how it is actually used.


>>
>>
>>>> 2) device is unaware of the bound driver, non page aligned doorbell 
>>>> doesn't necessarily meant it can be used
>>> Yes, non page aligned doorbell can not be used, so there is a check.
>>
>>
>> Typo, what I meant is "it can't be used". That is to say, we should 
>> let the vDPA bus driver to decide whether or not it can be used.
> If it is not page aligned, there would be extra complexities for 
> vhost/qemu, I see it as a hardware defect, 


It is allowed by the virtio spec, isn't it?

Thanks


> why adapt to this kind of defects?
>
> Thanks
> Zhu Lingshan
>>
>> Thanks
>>
>>
>>>
>>> Thanks
>>> Zhu Lingshan
>>>>
>>>> Let's leave those polices to the driver.
>>>>
>>>> Thanks
>>>>
>>>>
>>>>> +    } else {
>>>>> +        area.addr = vf->notify_pa;
>>>>> +        area.size = PAGE_SIZE;
>>>>> +    }
>>>>> +
>>>>> +    return area;
>>>>> +}
>>>>> +
>>>>>   /*
>>>>>    * IFCVF currently does't have on-chip IOMMU, so not
>>>>>    * implemented set_map()/dma_map()/dma_unmap()
>>>>> @@ -440,6 +457,7 @@ static const struct vdpa_config_ops 
>>>>> ifc_vdpa_ops ={
>>>>>       .get_config    = ifcvf_vdpa_get_config,
>>>>>       .set_config    = ifcvf_vdpa_set_config,
>>>>>       .set_config_cb  = ifcvf_vdpa_set_config_cb,
>>>>> +    .get_vq_notification = ifcvf_get_vq_notification,
>>>>>   };
>>>>>     static int ifcvf_probe(struct pci_dev *pdev, const struct 
>>>>> pci_device_id *id)
>>>>
>>>
>>
>

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

[Jason Wang]

在 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 {
>>
>

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

[Zhu, Lingshan]

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

Re: [PATCH 2/2] vDPA/ifcvf: implement doorbell mapping for ifcvf

[Zhu, Lingshan]

On 4/28/2021 6:03 PM, Jason Wang wrote:
>
> 在 2021/4/28 下午5:56, Zhu, Lingshan 写道:
>>
>>
>> On 4/28/2021 5:21 PM, Jason Wang wrote:
>>>
>>> 在 2021/4/28 下午4:59, Zhu, Lingshan 写道:
>>>>
>>>>
>>>> On 4/28/2021 4:42 PM, Jason Wang wrote:
>>>>>
>>>>> 在 2021/4/28 下午4:21, Zhu Lingshan 写道:
>>>>>> This commit implements doorbell mapping feature for ifcvf.
>>>>>> This feature maps the notify page to userspace, to eliminate
>>>>>> vmexit when kick a vq.
>>>>>>
>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>> ---
>>>>>>   drivers/vdpa/ifcvf/ifcvf_main.c | 18 ++++++++++++++++++
>>>>>>   1 file changed, 18 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>> index e48e6b74fe2e..afcb71bc0f51 100644
>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>> @@ -413,6 +413,23 @@ static int ifcvf_vdpa_get_vq_irq(struct 
>>>>>> vdpa_device *vdpa_dev,
>>>>>>       return vf->vring[qid].irq;
>>>>>>   }
>>>>>>   +static struct vdpa_notification_area 
>>>>>> ifcvf_get_vq_notification(struct vdpa_device *vdpa_dev,
>>>>>> +                                   u16 idx)
>>>>>> +{
>>>>>> +    struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>>> +    struct vdpa_notification_area area;
>>>>>> +
>>>>>> +    if (vf->notify_pa % PAGE_SIZE) {
>>>>>> +        area.addr = 0;
>>>>>> +        area.size = 0;
>>>>>
>>>>>
>>>>> We don't need this since:
>>>>>
>>>>> 1) there's a check in the vhost vDPA
>>>> I think you mean this code block in vdpa.c
>>>>         notify = ops->get_vq_notification(vdpa, index);
>>>>         if (notify.addr & (PAGE_SIZE - 1))
>>>>                 return -EINVAL;
>>>>
>>>> This should work, however, I think the parent driver should ensure 
>>>> it passes a PAGE_SIZE aligned address to userspace, to be robust, 
>>>> to be reliable.
>>>
>>>
>>> The point is parent is unaware of whether or not there's a userspace.
>> when calling this, I think it targets a usersapce program, why kernel 
>> space need it, so IMHO no harm if we check this to keep the parent 
>> driver robust.
>
>
> Again, vDPA device is unaware of what driver that is bound. It could 
> be virtio-vpda, vhost-vdpa or other in the future. It's only the vDPA 
> bus driver know how it is actually used.
>
>
>>>
>>>
>>>>> 2) device is unaware of the bound driver, non page aligned 
>>>>> doorbell doesn't necessarily meant it can be used
>>>> Yes, non page aligned doorbell can not be used, so there is a check.
>>>
>>>
>>> Typo, what I meant is "it can't be used". That is to say, we should 
>>> let the vDPA bus driver to decide whether or not it can be used.
>> If it is not page aligned, there would be extra complexities for 
>> vhost/qemu, I see it as a hardware defect, 
>
>
> It is allowed by the virtio spec, isn't it?
The spec does not require the doorbell to be page size aligned, however 
it still a hardware defect if non page size aligned notify base present, 
I will leave a warning message here instead of the 0 value.

Thanks
Zhu Lingshan
>
> Thanks
>
>
>> why adapt to this kind of defects?
>>
>> Thanks
>> Zhu Lingshan
>>>
>>> Thanks
>>>
>>>
>>>>
>>>> Thanks
>>>> Zhu Lingshan
>>>>>
>>>>> Let's leave those polices to the driver.
>>>>>
>>>>> Thanks
>>>>>
>>>>>
>>>>>> +    } else {
>>>>>> +        area.addr = vf->notify_pa;
>>>>>> +        area.size = PAGE_SIZE;
>>>>>> +    }
>>>>>> +
>>>>>> +    return area;
>>>>>> +}
>>>>>> +
>>>>>>   /*
>>>>>>    * IFCVF currently does't have on-chip IOMMU, so not
>>>>>>    * implemented set_map()/dma_map()/dma_unmap()
>>>>>> @@ -440,6 +457,7 @@ static const struct vdpa_config_ops 
>>>>>> ifc_vdpa_ops ={
>>>>>>       .get_config    = ifcvf_vdpa_get_config,
>>>>>>       .set_config    = ifcvf_vdpa_set_config,
>>>>>>       .set_config_cb  = ifcvf_vdpa_set_config_cb,
>>>>>> +    .get_vq_notification = ifcvf_get_vq_notification,
>>>>>>   };
>>>>>>     static int ifcvf_probe(struct pci_dev *pdev, const struct 
>>>>>> pci_device_id *id)
>>>>>
>>>>
>>>
>>
>

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4303 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31685 bytes --]

Re: [PATCH 2/2] vDPA/ifcvf: implement doorbell mapping for ifcvf

[Jason Wang]

在 2021/4/28 下午6:20, Zhu, Lingshan 写道:
>
>
> On 4/28/2021 6:03 PM, Jason Wang wrote:
>>
>> 在 2021/4/28 下午5:56, Zhu, Lingshan 写道:
>>>
>>>
>>> On 4/28/2021 5:21 PM, Jason Wang wrote:
>>>>
>>>> 在 2021/4/28 下午4:59, Zhu, Lingshan 写道:
>>>>>
>>>>>
>>>>> On 4/28/2021 4:42 PM, Jason Wang wrote:
>>>>>>
>>>>>> 在 2021/4/28 下午4:21, Zhu Lingshan 写道:
>>>>>>> This commit implements doorbell mapping feature for ifcvf.
>>>>>>> This feature maps the notify page to userspace, to eliminate
>>>>>>> vmexit when kick a vq.
>>>>>>>
>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>>> ---
>>>>>>>   drivers/vdpa/ifcvf/ifcvf_main.c | 18 ++++++++++++++++++
>>>>>>>   1 file changed, 18 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>>>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>>> index e48e6b74fe2e..afcb71bc0f51 100644
>>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>>> @@ -413,6 +413,23 @@ static int ifcvf_vdpa_get_vq_irq(struct 
>>>>>>> vdpa_device *vdpa_dev,
>>>>>>>       return vf->vring[qid].irq;
>>>>>>>   }
>>>>>>>   +static struct vdpa_notification_area 
>>>>>>> ifcvf_get_vq_notification(struct vdpa_device *vdpa_dev,
>>>>>>> +                                   u16 idx)
>>>>>>> +{
>>>>>>> +    struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>>>> +    struct vdpa_notification_area area;
>>>>>>> +
>>>>>>> +    if (vf->notify_pa % PAGE_SIZE) {
>>>>>>> +        area.addr = 0;
>>>>>>> +        area.size = 0;
>>>>>>
>>>>>>
>>>>>> We don't need this since:
>>>>>>
>>>>>> 1) there's a check in the vhost vDPA
>>>>> I think you mean this code block in vdpa.c
>>>>>         notify = ops->get_vq_notification(vdpa, index);
>>>>>         if (notify.addr & (PAGE_SIZE - 1))
>>>>>                 return -EINVAL;
>>>>>
>>>>> This should work, however, I think the parent driver should ensure 
>>>>> it passes a PAGE_SIZE aligned address to userspace, to be robust, 
>>>>> to be reliable.
>>>>
>>>>
>>>> The point is parent is unaware of whether or not there's a userspace.
>>> when calling this, I think it targets a usersapce program, why 
>>> kernel space need it, so IMHO no harm if we check this to keep the 
>>> parent driver robust.
>>
>>
>> Again, vDPA device is unaware of what driver that is bound. It could 
>> be virtio-vpda, vhost-vdpa or other in the future. It's only the vDPA 
>> bus driver know how it is actually used.
>>
>>
>>>>
>>>>
>>>>>> 2) device is unaware of the bound driver, non page aligned 
>>>>>> doorbell doesn't necessarily meant it can be used
>>>>> Yes, non page aligned doorbell can not be used, so there is a check.
>>>>
>>>>
>>>> Typo, what I meant is "it can't be used". That is to say, we should 
>>>> let the vDPA bus driver to decide whether or not it can be used.
>>> If it is not page aligned, there would be extra complexities for 
>>> vhost/qemu, I see it as a hardware defect, 
>>
>>
>> It is allowed by the virtio spec, isn't it?
> The spec does not require the doorbell to be page size aligned, 
> however it still a hardware defect if non page size aligned notify 
> base present, I will leave a warning message here instead of the 0 value.
>

Another note is that, using PAGE_SIZE is wrong here since it varies 
among archs (at most 64K on some one).

Thanks


> Thanks
> Zhu Lingshan
>>
>> Thanks
>>
>>
>>> why adapt to this kind of defects?
>>>
>>> Thanks
>>> Zhu Lingshan
>>>>
>>>> Thanks
>>>>
>>>>
>>>>>
>>>>> Thanks
>>>>> Zhu Lingshan
>>>>>>
>>>>>> Let's leave those polices to the driver.
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>>
>>>>>>> +    } else {
>>>>>>> +        area.addr = vf->notify_pa;
>>>>>>> +        area.size = PAGE_SIZE;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    return area;
>>>>>>> +}
>>>>>>> +
>>>>>>>   /*
>>>>>>>    * IFCVF currently does't have on-chip IOMMU, so not
>>>>>>>    * implemented set_map()/dma_map()/dma_unmap()
>>>>>>> @@ -440,6 +457,7 @@ static const struct vdpa_config_ops 
>>>>>>> ifc_vdpa_ops ={
>>>>>>>       .get_config    = ifcvf_vdpa_get_config,
>>>>>>>       .set_config    = ifcvf_vdpa_set_config,
>>>>>>>       .set_config_cb  = ifcvf_vdpa_set_config_cb,
>>>>>>> +    .get_vq_notification = ifcvf_get_vq_notification,
>>>>>>>   };
>>>>>>>     static int ifcvf_probe(struct pci_dev *pdev, const struct 
>>>>>>> pci_device_id *id)
>>>>>>
>>>>>
>>>>
>>>
>>
>

Re: [PATCH 2/2] vDPA/ifcvf: implement doorbell mapping for ifcvf

[Zhu, Lingshan]

On 4/28/2021 9:08 PM, Jason Wang wrote:
>
> 在 2021/4/28 下午6:20, Zhu, Lingshan 写道:
>>
>>
>> On 4/28/2021 6:03 PM, Jason Wang wrote:
>>>
>>> 在 2021/4/28 下午5:56, Zhu, Lingshan 写道:
>>>>
>>>>
>>>> On 4/28/2021 5:21 PM, Jason Wang wrote:
>>>>>
>>>>> 在 2021/4/28 下午4:59, Zhu, Lingshan 写道:
>>>>>>
>>>>>>
>>>>>> On 4/28/2021 4:42 PM, Jason Wang wrote:
>>>>>>>
>>>>>>> 在 2021/4/28 下午4:21, Zhu Lingshan 写道:
>>>>>>>> This commit implements doorbell mapping feature for ifcvf.
>>>>>>>> This feature maps the notify page to userspace, to eliminate
>>>>>>>> vmexit when kick a vq.
>>>>>>>>
>>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>>>> ---
>>>>>>>>   drivers/vdpa/ifcvf/ifcvf_main.c | 18 ++++++++++++++++++
>>>>>>>>   1 file changed, 18 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>>>>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>>>> index e48e6b74fe2e..afcb71bc0f51 100644
>>>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>>>> @@ -413,6 +413,23 @@ static int ifcvf_vdpa_get_vq_irq(struct 
>>>>>>>> vdpa_device *vdpa_dev,
>>>>>>>>       return vf->vring[qid].irq;
>>>>>>>>   }
>>>>>>>>   +static struct vdpa_notification_area 
>>>>>>>> ifcvf_get_vq_notification(struct vdpa_device *vdpa_dev,
>>>>>>>> +                                   u16 idx)
>>>>>>>> +{
>>>>>>>> +    struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>>>>> +    struct vdpa_notification_area area;
>>>>>>>> +
>>>>>>>> +    if (vf->notify_pa % PAGE_SIZE) {
>>>>>>>> +        area.addr = 0;
>>>>>>>> +        area.size = 0;
>>>>>>>
>>>>>>>
>>>>>>> We don't need this since:
>>>>>>>
>>>>>>> 1) there's a check in the vhost vDPA
>>>>>> I think you mean this code block in vdpa.c
>>>>>>         notify = ops->get_vq_notification(vdpa, index);
>>>>>>         if (notify.addr & (PAGE_SIZE - 1))
>>>>>>                 return -EINVAL;
>>>>>>
>>>>>> This should work, however, I think the parent driver should 
>>>>>> ensure it passes a PAGE_SIZE aligned address to userspace, to be 
>>>>>> robust, to be reliable.
>>>>>
>>>>>
>>>>> The point is parent is unaware of whether or not there's a userspace.
>>>> when calling this, I think it targets a usersapce program, why 
>>>> kernel space need it, so IMHO no harm if we check this to keep the 
>>>> parent driver robust.
>>>
>>>
>>> Again, vDPA device is unaware of what driver that is bound. It could 
>>> be virtio-vpda, vhost-vdpa or other in the future. It's only the 
>>> vDPA bus driver know how it is actually used.
>>>
>>>
>>>>>
>>>>>
>>>>>>> 2) device is unaware of the bound driver, non page aligned 
>>>>>>> doorbell doesn't necessarily meant it can be used
>>>>>> Yes, non page aligned doorbell can not be used, so there is a check.
>>>>>
>>>>>
>>>>> Typo, what I meant is "it can't be used". That is to say, we 
>>>>> should let the vDPA bus driver to decide whether or not it can be 
>>>>> used.
>>>> If it is not page aligned, there would be extra complexities for 
>>>> vhost/qemu, I see it as a hardware defect, 
>>>
>>>
>>> It is allowed by the virtio spec, isn't it?
>> The spec does not require the doorbell to be page size aligned, 
>> however it still a hardware defect if non page size aligned notify 
>> base present, I will leave a warning message here instead of the 0 
>> value.
>>
>
> Another note is that, using PAGE_SIZE is wrong here since it varies 
> among archs (at most 64K on some one).
For the page alignment checks, I think this is the point of using 
PAGE_SIZE, we want the doorbell placed at the page boundary, PAGE_SIZE 
depends on the arch,
so I think we don't want to use hard code here. We will pass the 
notify_pa to upper layer anyway, just print an warning if not PAGE_SIZE 
aligned.

However I think this may refer to vdpa_notification_area.size, YES, I 
think use PAGE_SIZE directly is wrong here, this size depends on the 
device(bar layout) than the arch, so I will add more code to tell which 
device is probed by the driver, then assign correct value.

Thanks
>
> Thanks
>
>
>> Thanks
>> Zhu Lingshan
>>>
>>> Thanks
>>>
>>>
>>>> why adapt to this kind of defects?
>>>>
>>>> Thanks
>>>> Zhu Lingshan
>>>>>
>>>>> Thanks
>>>>>
>>>>>
>>>>>>
>>>>>> Thanks
>>>>>> Zhu Lingshan
>>>>>>>
>>>>>>> Let's leave those polices to the driver.
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>>
>>>>>>>> +    } else {
>>>>>>>> +        area.addr = vf->notify_pa;
>>>>>>>> +        area.size = PAGE_SIZE;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    return area;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>   /*
>>>>>>>>    * IFCVF currently does't have on-chip IOMMU, so not
>>>>>>>>    * implemented set_map()/dma_map()/dma_unmap()
>>>>>>>> @@ -440,6 +457,7 @@ static const struct vdpa_config_ops 
>>>>>>>> ifc_vdpa_ops ={
>>>>>>>>       .get_config    = ifcvf_vdpa_get_config,
>>>>>>>>       .set_config    = ifcvf_vdpa_set_config,
>>>>>>>>       .set_config_cb  = ifcvf_vdpa_set_config_cb,
>>>>>>>> +    .get_vq_notification = ifcvf_get_vq_notification,
>>>>>>>>   };
>>>>>>>>     static int ifcvf_probe(struct pci_dev *pdev, const struct 
>>>>>>>> pci_device_id *id)
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>