Re: [PATCH 1/5] virt queue ops take immediate actions

["Zhu, Lingshan" <lingshan.zhu@intel.com>]

On 4/26/2023 11:39 AM, Jason Wang wrote:
>
> 在 2023/4/1 04:48, Zhu Lingshan 写道:
>> In this commit, virtqueue operations including:
>> set_vq_num(), set_vq_address(), set_vq_ready()
>> and get_vq_ready() access PCI registers directly
>> to take immediate actions.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vdpa/ifcvf/ifcvf_base.c | 58 ++++++++++++++++++++-------------
>>   drivers/vdpa/ifcvf/ifcvf_base.h | 10 +++---
>>   drivers/vdpa/ifcvf/ifcvf_main.c | 16 +++------
>>   3 files changed, 45 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c 
>> b/drivers/vdpa/ifcvf/ifcvf_base.c
>> index 5563b3a773c7..6c5650f73007 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>> @@ -329,31 +329,49 @@ int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 
>> qid, u16 num)
>>       return 0;
>>   }
>>   -static int ifcvf_hw_enable(struct ifcvf_hw *hw)
>> +void ifcvf_set_vq_num(struct ifcvf_hw *hw, u16 qid, u32 num)
>>   {
>> -    struct virtio_pci_common_cfg __iomem *cfg;
>> -    u32 i;
>> +    struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
>>   -    cfg = hw->common_cfg;
>> -    for (i = 0; i < hw->nr_vring; i++) {
>> -        if (!hw->vring[i].ready)
>> -            break;
>> +    vp_iowrite16(qid, &cfg->queue_select);
>> +    vp_iowrite16(num, &cfg->queue_size);
>> +}
>>   -        vp_iowrite16(i, &cfg->queue_select);
>> -        vp_iowrite64_twopart(hw->vring[i].desc, &cfg->queue_desc_lo,
>> -                     &cfg->queue_desc_hi);
>> -        vp_iowrite64_twopart(hw->vring[i].avail, &cfg->queue_avail_lo,
>> -                      &cfg->queue_avail_hi);
>> -        vp_iowrite64_twopart(hw->vring[i].used, &cfg->queue_used_lo,
>> -                     &cfg->queue_used_hi);
>> -        vp_iowrite16(hw->vring[i].size, &cfg->queue_size);
>> -        ifcvf_set_vq_state(hw, i, hw->vring[i].last_avail_idx);
>> -        vp_iowrite16(1, &cfg->queue_enable);
>> -    }
>> +int ifcvf_set_vq_address(struct ifcvf_hw *hw, u16 qid, u64 desc_area,
>> +             u64 driver_area, u64 device_area)
>> +{
>> +    struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
>> +
>> +    vp_iowrite16(qid, &cfg->queue_select);
>> +    vp_iowrite64_twopart(desc_area, &cfg->queue_desc_lo,
>> +                 &cfg->queue_desc_hi);
>> +    vp_iowrite64_twopart(driver_area, &cfg->queue_avail_lo,
>> +                 &cfg->queue_avail_hi);
>> +    vp_iowrite64_twopart(device_area, &cfg->queue_used_lo,
>> +                 &cfg->queue_used_hi);
>>         return 0;
>>   }
>>   +bool ifcvf_get_vq_ready(struct ifcvf_hw *hw, u16 qid)
>> +{
>> +    struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
>> +    u16 queue_enable;
>> +
>> +    vp_iowrite16(qid, &cfg->queue_select);
>> +    queue_enable = vp_ioread16(&cfg->queue_enable);
>> +
>> +    return (bool)queue_enable;
>> +}
>> +
>> +void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready)
>> +{
>> +    struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
>> +
>> +    vp_iowrite16(qid, &cfg->queue_select);
>> +    vp_iowrite16(ready, &cfg->queue_enable);
>> +}
>> +
>>   static void ifcvf_hw_disable(struct ifcvf_hw *hw)
>>   {
>>       u32 i;
>> @@ -366,16 +384,12 @@ static void ifcvf_hw_disable(struct ifcvf_hw *hw)
>>     int ifcvf_start_hw(struct ifcvf_hw *hw)
>>   {
>> -    ifcvf_reset(hw);
>
>
> This seems unrelated to the immediate actions?
This is because we must not reset the device after vq settings already
take affections, e.g., we must not reset the hw after set_vq_address or
set_vq_ready.

Thanks
>
> The rest looks good.
>
> Thanks
>
>
>> ifcvf_add_status(hw, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>>       ifcvf_add_status(hw, VIRTIO_CONFIG_S_DRIVER);
>>         if (ifcvf_config_features(hw) < 0)
>>           return -EINVAL;
>>   -    if (ifcvf_hw_enable(hw) < 0)
>> -        return -EINVAL;
>> -
>>       ifcvf_add_status(hw, VIRTIO_CONFIG_S_DRIVER_OK);
>>         return 0;
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>> index c20d1c40214e..d545a9411143 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>> @@ -47,12 +47,7 @@
>>   #define MSIX_VECTOR_DEV_SHARED            3
>>     struct vring_info {
>> -    u64 desc;
>> -    u64 avail;
>> -    u64 used;
>> -    u16 size;
>>       u16 last_avail_idx;
>> -    bool ready;
>>       void __iomem *notify_addr;
>>       phys_addr_t notify_pa;
>>       u32 irq;
>> @@ -137,4 +132,9 @@ int ifcvf_probed_virtio_net(struct ifcvf_hw *hw);
>>   u32 ifcvf_get_config_size(struct ifcvf_hw *hw);
>>   u16 ifcvf_set_vq_vector(struct ifcvf_hw *hw, u16 qid, int vector);
>>   u16 ifcvf_set_config_vector(struct ifcvf_hw *hw, int vector);
>> +void ifcvf_set_vq_num(struct ifcvf_hw *hw, u16 qid, u32 num);
>> +int ifcvf_set_vq_address(struct ifcvf_hw *hw, u16 qid, u64 desc_area,
>> +             u64 driver_area, u64 device_area);
>> +bool ifcvf_get_vq_ready(struct ifcvf_hw *hw, u16 qid);
>> +void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready);
>>   #endif /* _IFCVF_H_ */
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index 7f78c47e40d6..1357c67014ab 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -382,10 +382,6 @@ static void ifcvf_reset_vring(struct 
>> ifcvf_adapter *adapter)
>>         for (i = 0; i < vf->nr_vring; i++) {
>>           vf->vring[i].last_avail_idx = 0;
>> -        vf->vring[i].desc = 0;
>> -        vf->vring[i].avail = 0;
>> -        vf->vring[i].used = 0;
>> -        vf->vring[i].ready = 0;
>>           vf->vring[i].cb.callback = NULL;
>>           vf->vring[i].cb.private = NULL;
>>       }
>> @@ -542,14 +538,14 @@ static void ifcvf_vdpa_set_vq_ready(struct 
>> vdpa_device *vdpa_dev,
>>   {
>>       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>   -    vf->vring[qid].ready = ready;
>> +    ifcvf_set_vq_ready(vf, qid, ready);
>>   }
>>     static bool ifcvf_vdpa_get_vq_ready(struct vdpa_device *vdpa_dev, 
>> u16 qid)
>>   {
>>       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>   -    return vf->vring[qid].ready;
>> +    return ifcvf_get_vq_ready(vf, qid);
>>   }
>>     static void ifcvf_vdpa_set_vq_num(struct vdpa_device *vdpa_dev, 
>> u16 qid,
>> @@ -557,7 +553,7 @@ static void ifcvf_vdpa_set_vq_num(struct 
>> vdpa_device *vdpa_dev, u16 qid,
>>   {
>>       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>   -    vf->vring[qid].size = num;
>> +    ifcvf_set_vq_num(vf, qid, num);
>>   }
>>     static int ifcvf_vdpa_set_vq_address(struct vdpa_device 
>> *vdpa_dev, u16 qid,
>> @@ -566,11 +562,7 @@ static int ifcvf_vdpa_set_vq_address(struct 
>> vdpa_device *vdpa_dev, u16 qid,
>>   {
>>       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>   -    vf->vring[qid].desc = desc_area;
>> -    vf->vring[qid].avail = driver_area;
>> -    vf->vring[qid].used = device_area;
>> -
>> -    return 0;
>> +    return ifcvf_set_vq_address(vf, qid, desc_area, driver_area, 
>> device_area);
>>   }
>>     static void ifcvf_vdpa_kick_vq(struct vdpa_device *vdpa_dev, u16 
>> qid)
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization