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

On 4/28/2021 5:21 PM, Jason Wang wrote:

在 2021/4/28 下午4:59, Zhu, Lingshan 写道:

quoted

On 4/28/2021 4:42 PM, Jason Wang wrote:

quoted

在 2021/4/28 下午4:21, Zhu Lingshan 写道:

quoted

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

quoted

quoted

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

quoted

Thanks
Zhu Lingshan

quoted

Let's leave those polices to the driver.

Thanks

quoted

+    } 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)