Jason Wang
2022-Apr-27 05:56 UTC
[PATCH V2] vDPA/ifcvf: allow userspace to suspend a queue
? 2022/4/24 19:33, Zhu Lingshan ??:> Formerly, ifcvf driver has implemented a lazy-initialization mechanism > for the virtqueues, it would store all virtqueue config fields that > passed down from the userspace, then load them to the virtqueues and > enable the queues upon DRIVER_OK. > > To allow the userspace to suspend a virtqueue, > this commit passes queue_enable to the virtqueue directly through > set_vq_ready(). > > This feature requires and this commits implementing all virtqueue > ops(set_vq_addr, set_vq_num and set_vq_ready) to take immediate > actions than lazy-initialization, so ifcvf_hw_enable() is retired. > > set_features() should take immediate actions as well. > > ifcvf_add_status() is retierd because we should not add > status like FEATURES_OK by ifcvf's decision, this driver should > only set device status upon vdpa_ops.set_status() > > To avoid losing virtqueue configurations caused by multiple > rounds of reset(), this commit also refactors thed evice reset > routine, now it simply reset the config handler and the virtqueues, > and only once device-reset().It looks like the patch tries to do too many things at one run. I'd suggest to split them: 1) on-the-fly set via set_vq_ready(), but I don't see a reason why we need to change other lazy stuffs, since setting queue_enable to 1 before DRIVER_OK won't start the virtqueue anyhow 2) if necessary, converting the lazy stuffs 3) the synchornize_irq() fixes 4) other stuffs Thanks> > Signed-off-by: Zhu Lingshan <lingshan.zhu at intel.com> > --- > drivers/vdpa/ifcvf/ifcvf_base.c | 150 +++++++++++++++++++------------- > drivers/vdpa/ifcvf/ifcvf_base.h | 16 ++-- > drivers/vdpa/ifcvf/ifcvf_main.c | 81 +++-------------- > 3 files changed, 111 insertions(+), 136 deletions(-) > > diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c > index 48c4dadb0c7c..bbc9007a6f34 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_base.c > +++ b/drivers/vdpa/ifcvf/ifcvf_base.c > @@ -179,20 +179,7 @@ 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); > -} > - > -static void ifcvf_add_status(struct ifcvf_hw *hw, u8 status) > -{ > - if (status != 0) > - status |= ifcvf_get_status(hw); > - > - ifcvf_set_status(hw, status); > ifcvf_get_status(hw); > } > > @@ -213,7 +200,7 @@ u64 ifcvf_get_hw_features(struct ifcvf_hw *hw) > return features; > } > > -u64 ifcvf_get_features(struct ifcvf_hw *hw) > +u64 ifcvf_get_device_features(struct ifcvf_hw *hw) > { > return hw->hw_features; > } > @@ -280,7 +267,7 @@ void ifcvf_write_dev_config(struct ifcvf_hw *hw, u64 offset, > vp_iowrite8(*p++, hw->dev_cfg + offset + i); > } > > -static void ifcvf_set_features(struct ifcvf_hw *hw, u64 features) > +void ifcvf_set_features(struct ifcvf_hw *hw, u64 features) > { > struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg; > > @@ -289,22 +276,22 @@ static void ifcvf_set_features(struct ifcvf_hw *hw, u64 features) > > vp_iowrite32(1, &cfg->guest_feature_select); > vp_iowrite32(features >> 32, &cfg->guest_feature); > + > + vp_ioread32(&cfg->guest_feature); > } > > -static int ifcvf_config_features(struct ifcvf_hw *hw) > +u64 ifcvf_get_features(struct ifcvf_hw *hw) > { > - struct ifcvf_adapter *ifcvf; > + struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg; > + u64 features; > > - ifcvf = vf_to_adapter(hw); > - ifcvf_set_features(hw, hw->req_features); > - ifcvf_add_status(hw, VIRTIO_CONFIG_S_FEATURES_OK); > + vp_iowrite32(0, &cfg->device_feature_select); > + features = vp_ioread32(&cfg->device_feature); > > - if (!(ifcvf_get_status(hw) & VIRTIO_CONFIG_S_FEATURES_OK)) { > - IFCVF_ERR(ifcvf->pdev, "Failed to set FEATURES_OK status\n"); > - return -EIO; > - } > + vp_iowrite32(1, &cfg->device_feature_select); > + features |= ((u64)vp_ioread32(&cfg->guest_feature) << 32); > > - return 0; > + return features; > } > > u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid) > @@ -331,68 +318,111 @@ int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num) > ifcvf_lm = (struct ifcvf_lm_cfg __iomem *)hw->lm_cfg; > q_pair_id = qid / hw->nr_vring; > avail_idx_addr = &ifcvf_lm->vring_lm_cfg[q_pair_id].idx_addr[qid % 2]; > - hw->vring[qid].last_avail_idx = num; > vp_iowrite16(num, avail_idx_addr); > > 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; > } > > -static void ifcvf_hw_disable(struct ifcvf_hw *hw) > +void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready) > { > - u32 i; > + struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg; > + > + vp_iowrite16(qid, &cfg->queue_select); > + /* write 0 to queue_enable will suspend a queue*/ > + vp_iowrite16(ready, &cfg->queue_enable); > +} > + > +bool ifcvf_get_vq_ready(struct ifcvf_hw *hw, u16 qid) > +{ > + struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg; > + bool queue_enable; > + > + vp_iowrite16(qid, &cfg->queue_select); > + queue_enable = vp_ioread16(&cfg->queue_enable); > + > + return (bool)queue_enable; > +} > + > +static void synchronize_per_vq_irq(struct ifcvf_hw *hw) > +{ > + int i; > > - 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); > + if (hw->vring[i].irq != -EINVAL) > + synchronize_irq(hw->vring[i].irq); > } > } > > -int ifcvf_start_hw(struct ifcvf_hw *hw) > +static void synchronize_vqs_reused_irq(struct ifcvf_hw *hw) > { > - ifcvf_reset(hw); > - ifcvf_add_status(hw, VIRTIO_CONFIG_S_ACKNOWLEDGE); > - ifcvf_add_status(hw, VIRTIO_CONFIG_S_DRIVER); > + if (hw->vqs_reused_irq != -EINVAL) > + synchronize_irq(hw->vqs_reused_irq); > +} > > - if (ifcvf_config_features(hw) < 0) > - return -EINVAL; > +static void synchronize_vq_irq(struct ifcvf_hw *hw) > +{ > + u8 status = hw->msix_vector_status; > > - if (ifcvf_hw_enable(hw) < 0) > - return -EINVAL; > + if (status == MSIX_VECTOR_PER_VQ_AND_CONFIG) > + synchronize_per_vq_irq(hw); > + else > + synchronize_vqs_reused_irq(hw); > +} > > - ifcvf_add_status(hw, VIRTIO_CONFIG_S_DRIVER_OK); > +static void synchronize_config_irq(struct ifcvf_hw *hw) > +{ > + if (hw->config_irq != -EINVAL) > + synchronize_irq(hw->config_irq); > +} > > - return 0; > +static void ifcvf_reset_vring(struct ifcvf_hw *hw) > +{ > + int i; > + > + for (i = 0; i < hw->nr_vring; i++) { > + synchronize_vq_irq(hw); > + hw->vring[i].cb.callback = NULL; > + hw->vring[i].cb.private = NULL; > + ifcvf_set_vq_vector(hw, i, VIRTIO_MSI_NO_VECTOR); > + } > +} > + > +static void ifcvf_reset_config_handler(struct ifcvf_hw *hw) > +{ > + synchronize_config_irq(hw); > + hw->config_cb.callback = NULL; > + hw->config_cb.private = NULL; > + ifcvf_set_config_vector(hw, VIRTIO_MSI_NO_VECTOR); > } > > void ifcvf_stop_hw(struct ifcvf_hw *hw) > { > - ifcvf_hw_disable(hw); > - ifcvf_reset(hw); > + ifcvf_reset_vring(hw); > + ifcvf_reset_config_handler(hw); > } > > 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 115b61f4924b..f3dce0d795cb 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_base.h > +++ b/drivers/vdpa/ifcvf/ifcvf_base.h > @@ -49,12 +49,6 @@ > #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; > @@ -76,7 +70,6 @@ struct ifcvf_hw { > phys_addr_t notify_base_pa; > u32 notify_off_multiplier; > u32 dev_type; > - u64 req_features; > u64 hw_features; > struct virtio_pci_common_cfg __iomem *common_cfg; > void __iomem *dev_cfg; > @@ -123,7 +116,7 @@ u8 ifcvf_get_status(struct ifcvf_hw *hw); > void ifcvf_set_status(struct ifcvf_hw *hw, u8 status); > void io_write64_twopart(u64 val, u32 *lo, u32 *hi); > void ifcvf_reset(struct ifcvf_hw *hw); > -u64 ifcvf_get_features(struct ifcvf_hw *hw); > +u64 ifcvf_get_device_features(struct ifcvf_hw *hw); > u64 ifcvf_get_hw_features(struct ifcvf_hw *hw); > int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64 features); > u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid); > @@ -131,6 +124,13 @@ int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num); > struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw); > int ifcvf_probed_virtio_net(struct ifcvf_hw *hw); > u32 ifcvf_get_config_size(struct ifcvf_hw *hw); > +int ifcvf_set_vq_address(struct ifcvf_hw *hw, u16 qid, u64 desc_area, > + u64 driver_area, u64 device_area); > 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); > +void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready); > +bool ifcvf_get_vq_ready(struct ifcvf_hw *hw, u16 qid); > +void ifcvf_set_features(struct ifcvf_hw *hw, u64 features); > +u64 ifcvf_get_features(struct ifcvf_hw *hw); > #endif /* _IFCVF_H_ */ > diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c > index 4366320fb68d..0257ba98cffe 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_main.c > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c > @@ -358,53 +358,6 @@ static int ifcvf_request_irq(struct ifcvf_adapter *adapter) > return 0; > } > > -static int ifcvf_start_datapath(void *private) > -{ > - struct ifcvf_hw *vf = ifcvf_private_to_vf(private); > - u8 status; > - int ret; > - > - ret = ifcvf_start_hw(vf); > - if (ret < 0) { > - status = ifcvf_get_status(vf); > - status |= VIRTIO_CONFIG_S_FAILED; > - ifcvf_set_status(vf, status); > - } > - > - return ret; > -} > - > -static int ifcvf_stop_datapath(void *private) > -{ > - struct ifcvf_hw *vf = ifcvf_private_to_vf(private); > - int i; > - > - for (i = 0; i < vf->nr_vring; i++) > - vf->vring[i].cb.callback = NULL; > - > - ifcvf_stop_hw(vf); > - > - return 0; > -} > - > -static void ifcvf_reset_vring(struct ifcvf_adapter *adapter) > -{ > - struct ifcvf_hw *vf = ifcvf_private_to_vf(adapter); > - int i; > - > - 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; > - } > - > - ifcvf_reset(vf); > -} > - > static struct ifcvf_adapter *vdpa_to_adapter(struct vdpa_device *vdpa_dev) > { > return container_of(vdpa_dev, struct ifcvf_adapter, vdpa); > @@ -426,7 +379,7 @@ static u64 ifcvf_vdpa_get_device_features(struct vdpa_device *vdpa_dev) > u64 features; > > if (type == VIRTIO_ID_NET || type == VIRTIO_ID_BLOCK) > - features = ifcvf_get_features(vf); > + features = ifcvf_get_device_features(vf); > else { > features = 0; > IFCVF_ERR(pdev, "VIRTIO ID %u not supported\n", vf->dev_type); > @@ -444,7 +397,7 @@ static int ifcvf_vdpa_set_driver_features(struct vdpa_device *vdpa_dev, u64 feat > if (ret) > return ret; > > - vf->req_features = features; > + ifcvf_set_features(vf, features); > > return 0; > } > @@ -453,7 +406,7 @@ static u64 ifcvf_vdpa_get_driver_features(struct vdpa_device *vdpa_dev) > { > struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); > > - return vf->req_features; > + return ifcvf_get_features(vf); > } > > static u8 ifcvf_vdpa_get_status(struct vdpa_device *vdpa_dev) > @@ -486,11 +439,6 @@ static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status) > ifcvf_set_status(vf, status); > return; > } > - > - if (ifcvf_start_datapath(adapter) < 0) > - IFCVF_ERR(adapter->pdev, > - "Failed to set ifcvf vdpa status %u\n", > - status); > } > > ifcvf_set_status(vf, status); > @@ -509,12 +457,10 @@ static int ifcvf_vdpa_reset(struct vdpa_device *vdpa_dev) > if (status_old == 0) > return 0; > > - if (status_old & VIRTIO_CONFIG_S_DRIVER_OK) { > - ifcvf_stop_datapath(adapter); > - ifcvf_free_irq(adapter); > - } > + ifcvf_stop_hw(vf); > + ifcvf_free_irq(adapter); > > - ifcvf_reset_vring(adapter); > + ifcvf_reset(vf); > > return 0; > } > @@ -554,14 +500,17 @@ 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); > + bool ready; > + > + ready = ifcvf_get_vq_ready(vf, qid); > > - return vf->vring[qid].ready; > + return ready; > } > > static void ifcvf_vdpa_set_vq_num(struct vdpa_device *vdpa_dev, u16 qid, > @@ -569,7 +518,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, > @@ -578,11 +527,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)