Re: [PATCH] ifcvf: move IRQ request/free to status change handlers

[Jason Wang]

On 2020/5/12 äå11:38, Jason Wang wrote:
> > > > Â static int ifcvf_start_datapath(void *private)
> > > > Â {
> > > > ÂÂÂÂÂ struct ifcvf_hw *vf = ifcvf_private_to_vf(private);
> > > > @@ -118,9 +172,12 @@ static void ifcvf_vdpa_set_status(struct 
> > > > vdpa_device *vdpa_dev, u8 status)
> > > > Â {
> > > > ÂÂÂÂÂ struct ifcvf_adapter *adapter;
> > > > ÂÂÂÂÂ struct ifcvf_hw *vf;
> > > > +ÂÂÂ u8 status_old;
> > > > +ÂÂÂ int ret;
> > > >  Â vf = vdpa_to_vf(vdpa_dev);
> > > > ÂÂÂÂÂ adapter = dev_get_drvdata(vdpa_dev->dev.parent);
> > > > +ÂÂÂ status_old = ifcvf_get_status(vf);
> > > > Â ÂÂÂÂÂ if (status == 0) {
> > > > ÂÂÂÂÂÂÂÂÂ ifcvf_stop_datapath(adapter);
> > > > @@ -128,7 +185,22 @@ static void ifcvf_vdpa_set_status(struct 
> > > > vdpa_device *vdpa_dev, u8 status)
> > > > ÂÂÂÂÂÂÂÂÂ return;
> > > > ÂÂÂÂÂ }
> > > > Â -ÂÂÂ if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
> > > > +ÂÂÂ if ((status_old & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > > > +ÂÂÂÂÂÂÂ !(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > > +ÂÂÂÂÂÂÂ ifcvf_stop_datapath(adapter);
> > > > +ÂÂÂÂÂÂÂ ifcvf_free_irq(adapter, IFCVF_MAX_QUEUE_PAIRS * 2);
> > > > +ÂÂÂ }
> > > > +
> > > > +ÂÂÂ if ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
> > > > +ÂÂÂÂÂÂÂ !(status_old & VIRTIO_CONFIG_S_DRIVER_OK)) {
> > > > +ÂÂÂÂÂÂÂ ret = ifcvf_request_irq(adapter);
> > > > +ÂÂÂÂÂÂÂ if (ret) {
> > > > +ÂÂÂÂÂÂÂÂÂÂÂ status = ifcvf_get_status(vf);
> > > > +ÂÂÂÂÂÂÂÂÂÂÂ status |= VIRTIO_CONFIG_S_FAILED;
> > > > +ÂÂÂÂÂÂÂÂÂÂÂ ifcvf_set_status(vf, status);
> > > > +ÂÂÂÂÂÂÂÂÂÂÂ return;
> > > > +ÂÂÂÂÂÂÂ }
> > > > +
> > >
> > >
> > > Have a hard though on the logic here.
> > >
> > > This depends on the status setting from guest or userspace. Which 
> > > means it can not deal with e.g when qemu or userspace is crashed? Do 
> > > we need to care this or it's a over engineering?
> > >
> > > Thanks
> > If qemu crash, I guess users may re-run qmeu / re-initialize the 
> > device, according to the spec, there should be a reset routine.
> > This code piece handles status change on DRIVER_OK flipping. I am not 
> > sure I get your point, mind to give more hints?
>
>
> The problem is if we don't launch new qemu instance, the interrupt 
> will be still there?


Ok, we reset on vhost_vdpa_release() so the following is suspicious:

With the patch, we do:

static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status)
{
ÂÂÂÂÂÂÂ struct ifcvf_adapter *adapter;
ÂÂÂÂÂÂÂ struct ifcvf_hw *vf;
ÂÂÂÂÂÂÂ u8 status_old;
ÂÂÂÂÂÂÂ int ret;

 vf = vdpa_to_vf(vdpa_dev);
ÂÂÂÂÂÂÂ adapter = dev_get_drvdata(vdpa_dev->dev.parent);
ÂÂÂÂÂÂÂ status_old = ifcvf_get_status(vf);

ÂÂÂÂÂÂÂ if (status == 0) {
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ifcvf_stop_datapath(adapter);
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ifcvf_reset_vring(adapter);
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return;
ÂÂÂÂÂÂÂ }

ÂÂÂÂÂÂÂ if ((status_old & VIRTIO_CONFIG_S_DRIVER_OK) &&
ÂÂÂÂÂÂÂÂÂÂÂ !(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ifcvf_stop_datapath(adapter);
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ifcvf_free_irq(adapter, IFCVF_MAX_QUEUE_PAIRS * 2);
ÂÂÂÂÂÂÂ }

...

So the handling of status == 0 looks wrong.

The OK -> !OK check should already cover the datapath stopping and irq 
stuffs.

We only need to deal with vring reset and only need to do it after we 
stop the datapath/irq stuffs.

Thanks



> Thanks