Re: [PATCH V2 4/5] vDPA/ifcvf: synchronize irqs in the reset routine

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, May 26, 2023 at 1:30 PM Zhu, Lingshan <lingshan.zhu@xxxxxxxxx> wrote:
>
>
>
> On 5/26/2023 11:36 AM, Zhu, Lingshan wrote:
> >
> >
> > On 5/26/2023 9:34 AM, Jason Wang wrote:
> >> On Thu, May 25, 2023 at 5:38 PM Zhu, Lingshan
> >> <lingshan.zhu@xxxxxxxxx> wrote:
> >>>
> >>>
> >>> On 5/24/2023 4:03 PM, Jason Wang wrote:
> >>>> On Mon, May 8, 2023 at 6:05 PM Zhu Lingshan
> >>>> <lingshan.zhu@xxxxxxxxx> wrote:
> >>>>> This commit synchronize irqs of the virtqueues
> >>>>> and config space in the reset routine.
> >>>>> Thus ifcvf_stop_hw() and reset() are refactored as well.
> >>>>>
> >>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@xxxxxxxxx>
> >>>>> ---
> >>>>>    drivers/vdpa/ifcvf/ifcvf_base.c | 41 +++++++++++++++++++++--------
> >>>>>    drivers/vdpa/ifcvf/ifcvf_base.h |  1 +
> >>>>>    drivers/vdpa/ifcvf/ifcvf_main.c | 46
> >>>>> +++++----------------------------
> >>>>>    3 files changed, 38 insertions(+), 50 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c
> >>>>> b/drivers/vdpa/ifcvf/ifcvf_base.c
> >>>>> index 79e313c5e10e..1f39290baa38 100644
> >>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> >>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> >>>>> @@ -170,12 +170,9 @@ void ifcvf_set_status(struct ifcvf_hw *hw, u8
> >>>>> status)
> >>>>>
> >>>>>    void ifcvf_reset(struct ifcvf_hw *hw)
> >>>>>    {
> >>>>> -       hw->config_cb.callback = NULL;
> >>>>> -       hw->config_cb.private = NULL;
> >>>>> -
> >>>>>           ifcvf_set_status(hw, 0);
> >>>>> -       /* flush set_status, make sure VF is stopped, reset */
> >>>>> -       ifcvf_get_status(hw);
> >>>>> +       while (ifcvf_get_status(hw))
> >>>>> +               msleep(1);
> >>>>>    }
> >>>>>
> >>>>>    u64 ifcvf_get_hw_features(struct ifcvf_hw *hw)
> >>>>> @@ -368,20 +365,42 @@ void ifcvf_set_vq_ready(struct ifcvf_hw *hw,
> >>>>> u16 qid, bool ready)
> >>>>>           vp_iowrite16(ready, &cfg->queue_enable);
> >>>>>    }
> >>>>>
> >>>>> -static void ifcvf_hw_disable(struct ifcvf_hw *hw)
> >>>>> +static void ifcvf_reset_vring(struct ifcvf_hw *hw)
> >>>>>    {
> >>>>> -       u32 i;
> >>>>> +       u16 qid;
> >>>>> +
> >>>>> +       for (qid = 0; qid < hw->nr_vring; qid++) {
> >>>>> +               hw->vring[qid].cb.callback = NULL;
> >>>>> +               hw->vring[qid].cb.private = NULL;
> >>>>> +               ifcvf_set_vq_vector(hw, qid, VIRTIO_MSI_NO_VECTOR);
> >>>>> +       }
> >>>>> +}
> >>>>>
> >>>>> +static void ifcvf_reset_config_handler(struct ifcvf_hw *hw)
> >>>>> +{
> >>>>> +       hw->config_cb.callback = NULL;
> >>>>> +       hw->config_cb.private = NULL;
> >>>>>           ifcvf_set_config_vector(hw, VIRTIO_MSI_NO_VECTOR);
> >>>>> -       for (i = 0; i < hw->nr_vring; i++) {
> >>>>> -               ifcvf_set_vq_vector(hw, i, VIRTIO_MSI_NO_VECTOR);
> >>>>> +}
> >>>>> +
> >>>>> +static void ifcvf_synchronize_irq(struct ifcvf_hw *hw)
> >>>>> +{
> >>>>> +       u32 nvectors = hw->num_msix_vectors;
> >>>>> +       struct pci_dev *pdev = hw->pdev;
> >>>>> +       int i, irq;
> >>>>> +
> >>>>> +       for (i = 0; i < nvectors; i++) {
> >>>>> +               irq = pci_irq_vector(pdev, i);
> >>>>> +               if (irq >= 0)
> >>>>> +                       synchronize_irq(irq);
> >>>>>           }
> >>>>>    }
> >>>>>
> >>>>>    void ifcvf_stop_hw(struct ifcvf_hw *hw)
> >>>>>    {
> >>>>> -       ifcvf_hw_disable(hw);
> >>>>> -       ifcvf_reset(hw);
> >>>>> +       ifcvf_synchronize_irq(hw);
> >>>>> +       ifcvf_reset_vring(hw);
> >>>>> +       ifcvf_reset_config_handler(hw);
> >>>> Nit:
> >>>>
> >>>> So the name of this function is kind of misleading since irq
> >>>> synchronization and virtqueue/config handler are not belong to
> >>>> hardware?
> >>>>
> >>>> Maybe it would be better to call it ifcvf_stop().
> >>> Sure, I will send a V3 with this renaming,
> >>> do you ack patch 1/5?
> >> Yes, I think I've acked to that patch.
> > I will send a V3 with this renaming and your ack for patch 1/5
> By the way, do you ack this one after this function renaming?
> If so, I will also include your ack in V3, so we don't need another
> review process, I will ping Michael for a merge.

Have you seen the ack here?

https://lists.linuxfoundation.org/pipermail/virtualization/2023-May/066890.html

Thanks

> >>
> >> Thanks
> >>
> >>> Thanks
> >>> Zhu Lingshan
> >>>> Thanks
> >>>>
> >>>>>    }
> >>>>>
> >>>>>    void ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid)
> >>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
> >>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
> >>>>> index d34d3bc0dbf4..7430f80779be 100644
> >>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> >>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> >>>>> @@ -82,6 +82,7 @@ struct ifcvf_hw {
> >>>>>           int vqs_reused_irq;
> >>>>>           u16 nr_vring;
> >>>>>           /* VIRTIO_PCI_CAP_DEVICE_CFG size */
> >>>>> +       u32 num_msix_vectors;
> >>>>>           u32 cap_dev_config_size;
> >>>>>           struct pci_dev *pdev;
> >>>>>    };
> >>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
> >>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
> >>>>> index 968687159e44..3401b9901dd2 100644
> >>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> >>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> >>>>> @@ -125,6 +125,7 @@ static void ifcvf_free_irq(struct ifcvf_hw *vf)
> >>>>>           ifcvf_free_vq_irq(vf);
> >>>>>           ifcvf_free_config_irq(vf);
> >>>>>           ifcvf_free_irq_vectors(pdev);
> >>>>> +       vf->num_msix_vectors = 0;
> >>>>>    }
> >>>>>
> >>>>>    /* ifcvf MSIX vectors allocator, this helper tries to allocate
> >>>>> @@ -343,36 +344,11 @@ static int ifcvf_request_irq(struct ifcvf_hw
> >>>>> *vf)
> >>>>>           if (ret)
> >>>>>                   return ret;
> >>>>>
> >>>>> -       return 0;
> >>>>> -}
> >>>>> -
> >>>>> -static int ifcvf_stop_datapath(struct ifcvf_adapter *adapter)
> >>>>> -{
> >>>>> -       struct ifcvf_hw *vf = adapter->vf;
> >>>>> -       int i;
> >>>>> -
> >>>>> -       for (i = 0; i < vf->nr_vring; i++)
> >>>>> -               vf->vring[i].cb.callback = NULL;
> >>>>> -
> >>>>> -       ifcvf_stop_hw(vf);
> >>>>> +       vf->num_msix_vectors = nvectors;
> >>>>>
> >>>>>           return 0;
> >>>>>    }
> >>>>>
> >>>>> -static void ifcvf_reset_vring(struct ifcvf_adapter *adapter)
> >>>>> -{
> >>>>> -       struct ifcvf_hw *vf = adapter->vf;
> >>>>> -       int i;
> >>>>> -
> >>>>> -       for (i = 0; i < vf->nr_vring; i++) {
> >>>>> -               vf->vring[i].last_avail_idx = 0;
> >>>>> -               vf->vring[i].cb.callback = NULL;
> >>>>> -               vf->vring[i].cb.private = NULL;
> >>>>> -       }
> >>>>> -
> >>>>> -       ifcvf_reset(vf);
> >>>>> -}
> >>>>> -
> >>>>>    static struct ifcvf_adapter *vdpa_to_adapter(struct vdpa_device
> >>>>> *vdpa_dev)
> >>>>>    {
> >>>>>           return container_of(vdpa_dev, struct ifcvf_adapter, vdpa);
> >>>>> @@ -462,23 +438,15 @@ static void ifcvf_vdpa_set_status(struct
> >>>>> vdpa_device *vdpa_dev, u8 status)
> >>>>>
> >>>>>    static int ifcvf_vdpa_reset(struct vdpa_device *vdpa_dev)
> >>>>>    {
> >>>>> -       struct ifcvf_adapter *adapter;
> >>>>> -       struct ifcvf_hw *vf;
> >>>>> -       u8 status_old;
> >>>>> -
> >>>>> -       vf  = vdpa_to_vf(vdpa_dev);
> >>>>> -       adapter = vdpa_to_adapter(vdpa_dev);
> >>>>> -       status_old = ifcvf_get_status(vf);
> >>>>> +       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> >>>>> +       u8 status = ifcvf_get_status(vf);
> >>>>>
> >>>>> -       if (status_old == 0)
> >>>>> -               return 0;
> >>>>> +       ifcvf_stop_hw(vf);
> >>>>>
> >>>>> -       if (status_old & VIRTIO_CONFIG_S_DRIVER_OK) {
> >>>>> -               ifcvf_stop_datapath(adapter);
> >>>>> +       if (status & VIRTIO_CONFIG_S_DRIVER_OK)
> >>>>>                   ifcvf_free_irq(vf);
> >>>>> -       }
> >>>>>
> >>>>> -       ifcvf_reset_vring(adapter);
> >>>>> +       ifcvf_reset(vf);
> >>>>>
> >>>>>           return 0;
> >>>>>    }
> >>>>> --
> >>>>> 2.39.1
> >>>>>
> >
> > _______________________________________________
> > Virtualization mailing list
> > Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
> > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>

_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization




[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux