Re: [PATCH 2/5] get_driver_features from virito registers

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

On 4/26/2023 12:02 PM, Jason Wang wrote:
> 在 2023/4/1 04:48, Zhu Lingshan 写道:
> > This commit implements a new function ifcvf_get_driver_feature()
> > which read driver_features from virtio registers.
> >
> > To be less ambiguous, ifcvf_set_features() is renamed to
> > ifcvf_set_driver_features(), and ifcvf_get_features()
> > is renamed to ifcvf_get_dev_features() which returns
> > the provisioned vDPA device features.
> >
> > Signed-off-by: Zhu Lingshan <lingshan.zhu@xxxxxxxxx>
> > ---
> >   drivers/vdpa/ifcvf/ifcvf_base.c | 38 +++++++++++++++++----------------
> >   drivers/vdpa/ifcvf/ifcvf_base.h |  5 +++--
> >   drivers/vdpa/ifcvf/ifcvf_main.c |  9 +++++---
> >   3 files changed, 29 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c 
> > b/drivers/vdpa/ifcvf/ifcvf_base.c
> > index 6c5650f73007..546e923bcd16 100644
> > --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> > +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> > @@ -204,11 +204,29 @@ u64 ifcvf_get_hw_features(struct ifcvf_hw *hw)
> >       return features;
> >   }
> >   -u64 ifcvf_get_features(struct ifcvf_hw *hw)
> > +/* return provisioned vDPA dev features */
> > +u64 ifcvf_get_dev_features(struct ifcvf_hw *hw)
> >   {
> >       return hw->dev_features;
> >   }
> >   +u64 ifcvf_get_driver_features(struct ifcvf_hw *hw)
> > +{
> > +    struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
> > +    u32 features_lo, features_hi;
> > +    u64 features;
> > +
> > +    vp_iowrite32(0, &cfg->device_feature_select);
> > +    features_lo = vp_ioread32(&cfg->guest_feature);
> > +
> > +    vp_iowrite32(1, &cfg->device_feature_select);
> > +    features_hi = vp_ioread32(&cfg->guest_feature);
> > +
> > +    features = ((u64)features_hi << 32) | features_lo;
> > +
> > +    return features;
> > +}
>
>
> This duplicates with the logic ifcvf_get_hw_features(), it would be 
> simpler if we just do a rename.
Yes, they look very similar. ifcvf_get_hw_features() reads 
virtio_pci_common_cfg.device_feature
and ifcvf_get_driver_features reads virtio_pci_common_cfg.driver_feature.

Do you suggest we merge these two functions? something like this may 
look chaotic:

u64 ifcvf_get_features(struct ifcvf_hw *hw, bool device_feature)
{
    struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
    u32 features_lo, features_hi;
    u64 features;

    if (device_feature) {
vp_iowrite32(0, &cfg->device_feature_select);
features_lo = vp_ioread32(&cfg->guest_feature);
       vp_iowrite32(1, &cfg->device_feature_select);
       features_hi = vp_ioread32(&cfg->guest_feature);
    } else {
        vp_iowrite32(0, &cfg->device_feature_select);
        features_lo = vp_ioread32(&cfg->guest_feature);

        vp_iowrite32(1, &cfg->device_feature_select);
        features_hi = vp_ioread32(&cfg->guest_feature);

    }

features = ((u64)features_hi << 32) | features_lo;

    return features;
}

Maybe separate functions looks better.
> Thanks
>
>
> > +
> >   int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64 features)
> >   {
> >       if (!(features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)) && features) {
> > @@ -275,7 +293,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_driver_features(struct ifcvf_hw *hw, u64 features)
> >   {
> >       struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
> >   @@ -286,19 +304,6 @@ static void ifcvf_set_features(struct ifcvf_hw 
> > *hw, u64 features)
> >       vp_iowrite32(features >> 32, &cfg->guest_feature);
> >   }
> >   -static int ifcvf_config_features(struct ifcvf_hw *hw)
> > -{
> > -    ifcvf_set_features(hw, hw->req_features);
> > -    ifcvf_add_status(hw, VIRTIO_CONFIG_S_FEATURES_OK);
> > -
> > -    if (!(ifcvf_get_status(hw) & VIRTIO_CONFIG_S_FEATURES_OK)) {
> > -        IFCVF_ERR(hw->pdev, "Failed to set FEATURES_OK status\n");
> > -        return -EIO;
> > -    }
> > -
> > -    return 0;
> > -}
> > -
> >   u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid)
> >   {
> >       struct ifcvf_lm_cfg __iomem *ifcvf_lm;
> > @@ -387,9 +392,6 @@ int ifcvf_start_hw(struct ifcvf_hw *hw)
> >       ifcvf_add_status(hw, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> >       ifcvf_add_status(hw, VIRTIO_CONFIG_S_DRIVER);
> >   -    if (ifcvf_config_features(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 d545a9411143..cb19196c3ece 100644
> > --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> > +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> > @@ -69,7 +69,6 @@ struct ifcvf_hw {
> >       phys_addr_t notify_base_pa;
> >       u32 notify_off_multiplier;
> >       u32 dev_type;
> > -    u64 req_features;
> >       u64 hw_features;
> >       /* provisioned device features */
> >       u64 dev_features;
> > @@ -122,7 +121,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_dev_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);
> > @@ -137,4 +136,6 @@ 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);
> > +void ifcvf_set_driver_features(struct ifcvf_hw *hw, u64 features);
> > +u64 ifcvf_get_driver_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 1357c67014ab..4588484bd53d 100644
> > --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> > @@ -410,7 +410,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_dev_features(vf);
> >       else {
> >           features = 0;
> >           IFCVF_ERR(pdev, "VIRTIO ID %u not supported\n", vf->dev_type);
> > @@ -428,7 +428,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_driver_features(vf, features);
> >         return 0;
> >   }
> > @@ -436,8 +436,11 @@ static int ifcvf_vdpa_set_driver_features(struct 
> > vdpa_device *vdpa_dev, u64 feat
> >   static u64 ifcvf_vdpa_get_driver_features(struct vdpa_device 
> > *vdpa_dev)
> >   {
> >       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> > +    u64 features;
> > +
> > +    features = ifcvf_get_driver_features(vf);
> >   -    return vf->req_features;
> > +    return features;
> >   }
> >     static u8 ifcvf_vdpa_get_status(struct vdpa_device *vdpa_dev)

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