| Message ID | 20210428082133.6766-3-lingshan.zhu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vDPA/ifcvf: implement doorbell mapping feature | expand |
在 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)
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) >
在 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) >> >
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) >>> >> >
在 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) >>>> >>> >> >
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) >>>>> >>>> >>> >> >
在 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) >>>>>> >>>>> >>>> >>> >> >
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) >>>>>>> >>>>>> >>>>> >>>> >>> >> >
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)
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(+)