| Message ID | 20210310090052.4762-7-lingshan.zhu@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | vDPA/ifcvf: enables Intel C5000X-PL virtio-net | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 2021/3/10 5:00 下午, Zhu Lingshan wrote: > vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit > examines this when set features. > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > --- > drivers/vdpa/ifcvf/ifcvf_base.c | 8 ++++++++ > drivers/vdpa/ifcvf/ifcvf_base.h | 1 + > drivers/vdpa/ifcvf/ifcvf_main.c | 5 +++++ > 3 files changed, 14 insertions(+) > > diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c > index ea6a78791c9b..58f47fdce385 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_base.c > +++ b/drivers/vdpa/ifcvf/ifcvf_base.c > @@ -224,6 +224,14 @@ u64 ifcvf_get_features(struct ifcvf_hw *hw) > return hw->hw_features; > } > > +int ifcvf_verify_min_features(struct ifcvf_hw *hw) > +{ > + if (!(hw->hw_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM))) > + return -EINVAL; > + > + return 0; > +} > + > void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset, > void *dst, int length) > { > diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h > index dbb8c10aa3b1..91c5735d4dc9 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_base.h > +++ b/drivers/vdpa/ifcvf/ifcvf_base.h > @@ -123,6 +123,7 @@ 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_hw_features(struct ifcvf_hw *hw); > +int ifcvf_verify_min_features(struct ifcvf_hw *hw); > u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid); > int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num); > struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw); > diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c > index 25fb9dfe23f0..f624f202447d 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_main.c > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c > @@ -179,6 +179,11 @@ static u64 ifcvf_vdpa_get_features(struct vdpa_device *vdpa_dev) > static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, u64 features) > { > struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); > + int ret; > + > + ret = ifcvf_verify_min_features(vf); So this validate device features instead of driver which is the one we really want to check? Thanks > + if (ret) > + return ret; > > vf->req_features = features; >
On 3/11/2021 11:20 AM, Jason Wang wrote: > > On 2021/3/10 5:00 下午, Zhu Lingshan wrote: >> vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit >> examines this when set features. >> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >> --- >> drivers/vdpa/ifcvf/ifcvf_base.c | 8 ++++++++ >> drivers/vdpa/ifcvf/ifcvf_base.h | 1 + >> drivers/vdpa/ifcvf/ifcvf_main.c | 5 +++++ >> 3 files changed, 14 insertions(+) >> >> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c >> b/drivers/vdpa/ifcvf/ifcvf_base.c >> index ea6a78791c9b..58f47fdce385 100644 >> --- a/drivers/vdpa/ifcvf/ifcvf_base.c >> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c >> @@ -224,6 +224,14 @@ u64 ifcvf_get_features(struct ifcvf_hw *hw) >> return hw->hw_features; >> } >> +int ifcvf_verify_min_features(struct ifcvf_hw *hw) >> +{ >> + if (!(hw->hw_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM))) >> + return -EINVAL; >> + >> + return 0; >> +} >> + >> void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset, >> void *dst, int length) >> { >> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h >> b/drivers/vdpa/ifcvf/ifcvf_base.h >> index dbb8c10aa3b1..91c5735d4dc9 100644 >> --- a/drivers/vdpa/ifcvf/ifcvf_base.h >> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h >> @@ -123,6 +123,7 @@ 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_hw_features(struct ifcvf_hw *hw); >> +int ifcvf_verify_min_features(struct ifcvf_hw *hw); >> u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid); >> int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num); >> struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw); >> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c >> b/drivers/vdpa/ifcvf/ifcvf_main.c >> index 25fb9dfe23f0..f624f202447d 100644 >> --- a/drivers/vdpa/ifcvf/ifcvf_main.c >> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c >> @@ -179,6 +179,11 @@ static u64 ifcvf_vdpa_get_features(struct >> vdpa_device *vdpa_dev) >> static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, >> u64 features) >> { >> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); >> + int ret; >> + >> + ret = ifcvf_verify_min_features(vf); > > > So this validate device features instead of driver which is the one we > really want to check? > > Thanks Hi Jason, Here we check device feature bits to make sure the device support ACCESS_PLATFORM. In get_features(), it will return a intersection of device features bit and driver supported features bits(which includes ACCESS_PLATFORM). Other components like QEMU should not set features bits more than this intersection of bits. so we can make sure if this ifcvf_verify_min_features() passed, both device and driver support ACCESS_PLATFORM. Are you suggesting check driver feature bits in ifcvf_verify_min_features() in the meantime as well? Thanks！ > > >> + if (ret) >> + return ret; >> vf->req_features = features; > > _______________________________________________ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On 2021/3/11 12:16 下午, Zhu Lingshan wrote: > > > On 3/11/2021 11:20 AM, Jason Wang wrote: >> >> On 2021/3/10 5:00 下午, Zhu Lingshan wrote: >>> vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit >>> examines this when set features. >>> >>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>> --- >>> drivers/vdpa/ifcvf/ifcvf_base.c | 8 ++++++++ >>> drivers/vdpa/ifcvf/ifcvf_base.h | 1 + >>> drivers/vdpa/ifcvf/ifcvf_main.c | 5 +++++ >>> 3 files changed, 14 insertions(+) >>> >>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c >>> b/drivers/vdpa/ifcvf/ifcvf_base.c >>> index ea6a78791c9b..58f47fdce385 100644 >>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c >>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c >>> @@ -224,6 +224,14 @@ u64 ifcvf_get_features(struct ifcvf_hw *hw) >>> return hw->hw_features; >>> } >>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw) >>> +{ >>> + if (!(hw->hw_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM))) >>> + return -EINVAL; >>> + >>> + return 0; >>> +} >>> + >>> void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset, >>> void *dst, int length) >>> { >>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h >>> b/drivers/vdpa/ifcvf/ifcvf_base.h >>> index dbb8c10aa3b1..91c5735d4dc9 100644 >>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h >>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h >>> @@ -123,6 +123,7 @@ 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_hw_features(struct ifcvf_hw *hw); >>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw); >>> u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid); >>> int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num); >>> struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw); >>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c >>> b/drivers/vdpa/ifcvf/ifcvf_main.c >>> index 25fb9dfe23f0..f624f202447d 100644 >>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c >>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c >>> @@ -179,6 +179,11 @@ static u64 ifcvf_vdpa_get_features(struct >>> vdpa_device *vdpa_dev) >>> static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, >>> u64 features) >>> { >>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); >>> + int ret; >>> + >>> + ret = ifcvf_verify_min_features(vf); >> >> >> So this validate device features instead of driver which is the one >> we really want to check? >> >> Thanks > > Hi Jason, > > Here we check device feature bits to make sure the device support > ACCESS_PLATFORM. If you want to check device features, you need to do that during probe() and fail the probing if without the feature. But I think you won't ship cards without ACCESS_PLATFORM. > In get_features(), > it will return a intersection of device features bit and driver > supported features bits(which includes ACCESS_PLATFORM). > Other components like QEMU should not set features bits more than this > intersection of bits. so we can make sure if this > ifcvf_verify_min_features() passed, both device and driver support > ACCESS_PLATFORM. > > Are you suggesting check driver feature bits in > ifcvf_verify_min_features() in the meantime as well? So it really depends on your hardware. If you hardware can always offer ACCESS_PLATFORM, you just need to check driver features. This is how vdpa_sim and mlx5_vdpa work. Thanks > > Thanks！ >> >> >>> + if (ret) >>> + return ret; >>> vf->req_features = features; >> >> _______________________________________________ >> Virtualization mailing list >> Virtualization@lists.linux-foundation.org >> https://lists.linuxfoundation.org/mailman/listinfo/virtualization >
On 3/11/2021 2:20 PM, Jason Wang wrote: > > On 2021/3/11 12:16 下午, Zhu Lingshan wrote: >> >> >> On 3/11/2021 11:20 AM, Jason Wang wrote: >>> >>> On 2021/3/10 5:00 下午, Zhu Lingshan wrote: >>>> vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit >>>> examines this when set features. >>>> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>>> --- >>>> drivers/vdpa/ifcvf/ifcvf_base.c | 8 ++++++++ >>>> drivers/vdpa/ifcvf/ifcvf_base.h | 1 + >>>> drivers/vdpa/ifcvf/ifcvf_main.c | 5 +++++ >>>> 3 files changed, 14 insertions(+) >>>> >>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c >>>> b/drivers/vdpa/ifcvf/ifcvf_base.c >>>> index ea6a78791c9b..58f47fdce385 100644 >>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c >>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c >>>> @@ -224,6 +224,14 @@ u64 ifcvf_get_features(struct ifcvf_hw *hw) >>>> return hw->hw_features; >>>> } >>>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw) >>>> +{ >>>> + if (!(hw->hw_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM))) >>>> + return -EINVAL; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset, >>>> void *dst, int length) >>>> { >>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h >>>> b/drivers/vdpa/ifcvf/ifcvf_base.h >>>> index dbb8c10aa3b1..91c5735d4dc9 100644 >>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h >>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h >>>> @@ -123,6 +123,7 @@ 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_hw_features(struct ifcvf_hw *hw); >>>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw); >>>> u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid); >>>> int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num); >>>> struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw); >>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c >>>> b/drivers/vdpa/ifcvf/ifcvf_main.c >>>> index 25fb9dfe23f0..f624f202447d 100644 >>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c >>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c >>>> @@ -179,6 +179,11 @@ static u64 ifcvf_vdpa_get_features(struct >>>> vdpa_device *vdpa_dev) >>>> static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, >>>> u64 features) >>>> { >>>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); >>>> + int ret; >>>> + >>>> + ret = ifcvf_verify_min_features(vf); >>> >>> >>> So this validate device features instead of driver which is the one >>> we really want to check? >>> >>> Thanks >> >> Hi Jason, >> >> Here we check device feature bits to make sure the device support >> ACCESS_PLATFORM. > > > If you want to check device features, you need to do that during > probe() and fail the probing if without the feature. But I think you > won't ship cards without ACCESS_PLATFORM. Yes, there are no reasons ship a card without ACCESS_PLATFORM > > >> In get_features(), >> it will return a intersection of device features bit and driver >> supported features bits(which includes ACCESS_PLATFORM). >> Other components like QEMU should not set features bits more than >> this intersection of bits. so we can make sure if this >> ifcvf_verify_min_features() passed, both device and driver support >> ACCESS_PLATFORM. >> >> Are you suggesting check driver feature bits in >> ifcvf_verify_min_features() in the meantime as well? > > > So it really depends on your hardware. If you hardware can always > offer ACCESS_PLATFORM, you just need to check driver features. This is > how vdpa_sim and mlx5_vdpa work. Yes, we always support ACCESS_PLATFORM, so it is hard coded in the macro IFCVF_SUPPORTED_FEATURES. Now we check whether device support this feature bit as a double conformation, are you suggesting we should check whether ACCESS_PLATFORM & IFCVF_SUPPORTED_FEATURES in set_features() as well? I prefer check both device and IFCVF_SUPPORTED_FEATURES both, more reliable. Thanks! > > Thanks > > >> >> Thanks！ >>> >>> >>>> + if (ret) >>>> + return ret; >>>> vf->req_features = features; >>> >>> _______________________________________________ >>> Virtualization mailing list >>> Virtualization@lists.linux-foundation.org >>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization >> >
On 2021/3/11 3:19 下午, Zhu, Lingshan wrote: > > > On 3/11/2021 2:20 PM, Jason Wang wrote: >> >> On 2021/3/11 12:16 下午, Zhu Lingshan wrote: >>> >>> >>> On 3/11/2021 11:20 AM, Jason Wang wrote: >>>> >>>> On 2021/3/10 5:00 下午, Zhu Lingshan wrote: >>>>> vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit >>>>> examines this when set features. >>>>> >>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>>>> --- >>>>> drivers/vdpa/ifcvf/ifcvf_base.c | 8 ++++++++ >>>>> drivers/vdpa/ifcvf/ifcvf_base.h | 1 + >>>>> drivers/vdpa/ifcvf/ifcvf_main.c | 5 +++++ >>>>> 3 files changed, 14 insertions(+) >>>>> >>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c >>>>> b/drivers/vdpa/ifcvf/ifcvf_base.c >>>>> index ea6a78791c9b..58f47fdce385 100644 >>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c >>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c >>>>> @@ -224,6 +224,14 @@ u64 ifcvf_get_features(struct ifcvf_hw *hw) >>>>> return hw->hw_features; >>>>> } >>>>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw) >>>>> +{ >>>>> + if (!(hw->hw_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM))) >>>>> + return -EINVAL; >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset, >>>>> void *dst, int length) >>>>> { >>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h >>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h >>>>> index dbb8c10aa3b1..91c5735d4dc9 100644 >>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h >>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h >>>>> @@ -123,6 +123,7 @@ 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_hw_features(struct ifcvf_hw *hw); >>>>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw); >>>>> u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid); >>>>> int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num); >>>>> struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw); >>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c >>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c >>>>> index 25fb9dfe23f0..f624f202447d 100644 >>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c >>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c >>>>> @@ -179,6 +179,11 @@ static u64 ifcvf_vdpa_get_features(struct >>>>> vdpa_device *vdpa_dev) >>>>> static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, >>>>> u64 features) >>>>> { >>>>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); >>>>> + int ret; >>>>> + >>>>> + ret = ifcvf_verify_min_features(vf); >>>> >>>> >>>> So this validate device features instead of driver which is the one >>>> we really want to check? >>>> >>>> Thanks >>> >>> Hi Jason, >>> >>> Here we check device feature bits to make sure the device support >>> ACCESS_PLATFORM. >> >> >> If you want to check device features, you need to do that during >> probe() and fail the probing if without the feature. But I think you >> won't ship cards without ACCESS_PLATFORM. > Yes, there are no reasons ship a card without ACCESS_PLATFORM >> >> >>> In get_features(), >>> it will return a intersection of device features bit and driver >>> supported features bits(which includes ACCESS_PLATFORM). >>> Other components like QEMU should not set features bits more than >>> this intersection of bits. so we can make sure if this >>> ifcvf_verify_min_features() passed, both device and driver support >>> ACCESS_PLATFORM. >>> >>> Are you suggesting check driver feature bits in >>> ifcvf_verify_min_features() in the meantime as well? >> >> >> So it really depends on your hardware. If you hardware can always >> offer ACCESS_PLATFORM, you just need to check driver features. This >> is how vdpa_sim and mlx5_vdpa work. > Yes, we always support ACCESS_PLATFORM, so it is hard coded in the > macro IFCVF_SUPPORTED_FEATURES. That's not what I read from the code: features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES; > Now we check whether device support this feature bit as a double > conformation, are you suggesting we should check whether > ACCESS_PLATFORM & IFCVF_SUPPORTED_FEATURES > in set_features() as well? If we know device will always offer ACCESS_PLATFORM, there's no need to check it again. What we should check if whether driver set that, and if it doesn't we need to fail set_features(). I think there's little chance that IFCVF can work when IOMMU_PLATFORM is not negotiated. > I prefer check both device and IFCVF_SUPPORTED_FEATURES both, more > reliable. So again, if you want to check device features, set_features() is not the proper place. We need to fail the probe in this case. Thanks > > Thanks! >> >> Thanks >> >> >>> >>> Thanks！ >>>> >>>> >>>>> + if (ret) >>>>> + return ret; >>>>> vf->req_features = features; >>>> >>>> _______________________________________________ >>>> Virtualization mailing list >>>> Virtualization@lists.linux-foundation.org >>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization >>> >> >
On 3/12/2021 1:52 PM, Jason Wang wrote: > > On 2021/3/11 3:19 下午, Zhu, Lingshan wrote: >> >> >> On 3/11/2021 2:20 PM, Jason Wang wrote: >>> >>> On 2021/3/11 12:16 下午, Zhu Lingshan wrote: >>>> >>>> >>>> On 3/11/2021 11:20 AM, Jason Wang wrote: >>>>> >>>>> On 2021/3/10 5:00 下午, Zhu Lingshan wrote: >>>>>> vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit >>>>>> examines this when set features. >>>>>> >>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>>>>> --- >>>>>> drivers/vdpa/ifcvf/ifcvf_base.c | 8 ++++++++ >>>>>> drivers/vdpa/ifcvf/ifcvf_base.h | 1 + >>>>>> drivers/vdpa/ifcvf/ifcvf_main.c | 5 +++++ >>>>>> 3 files changed, 14 insertions(+) >>>>>> >>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c >>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.c >>>>>> index ea6a78791c9b..58f47fdce385 100644 >>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c >>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c >>>>>> @@ -224,6 +224,14 @@ u64 ifcvf_get_features(struct ifcvf_hw *hw) >>>>>> return hw->hw_features; >>>>>> } >>>>>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw) >>>>>> +{ >>>>>> + if (!(hw->hw_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM))) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset, >>>>>> void *dst, int length) >>>>>> { >>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h >>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h >>>>>> index dbb8c10aa3b1..91c5735d4dc9 100644 >>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h >>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h >>>>>> @@ -123,6 +123,7 @@ 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_hw_features(struct ifcvf_hw *hw); >>>>>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw); >>>>>> u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid); >>>>>> int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num); >>>>>> struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw); >>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c >>>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c >>>>>> index 25fb9dfe23f0..f624f202447d 100644 >>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c >>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c >>>>>> @@ -179,6 +179,11 @@ static u64 ifcvf_vdpa_get_features(struct >>>>>> vdpa_device *vdpa_dev) >>>>>> static int ifcvf_vdpa_set_features(struct vdpa_device >>>>>> *vdpa_dev, u64 features) >>>>>> { >>>>>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); >>>>>> + int ret; >>>>>> + >>>>>> + ret = ifcvf_verify_min_features(vf); >>>>> >>>>> >>>>> So this validate device features instead of driver which is the >>>>> one we really want to check? >>>>> >>>>> Thanks >>>> >>>> Hi Jason, >>>> >>>> Here we check device feature bits to make sure the device support >>>> ACCESS_PLATFORM. >>> >>> >>> If you want to check device features, you need to do that during >>> probe() and fail the probing if without the feature. But I think you >>> won't ship cards without ACCESS_PLATFORM. >> Yes, there are no reasons ship a card without ACCESS_PLATFORM >>> >>> >>>> In get_features(), >>>> it will return a intersection of device features bit and driver >>>> supported features bits(which includes ACCESS_PLATFORM). >>>> Other components like QEMU should not set features bits more than >>>> this intersection of bits. so we can make sure if this >>>> ifcvf_verify_min_features() passed, both device and driver support >>>> ACCESS_PLATFORM. >>>> >>>> Are you suggesting check driver feature bits in >>>> ifcvf_verify_min_features() in the meantime as well? >>> >>> >>> So it really depends on your hardware. If you hardware can always >>> offer ACCESS_PLATFORM, you just need to check driver features. This >>> is how vdpa_sim and mlx5_vdpa work. >> Yes, we always support ACCESS_PLATFORM, so it is hard coded in the >> macro IFCVF_SUPPORTED_FEATURES. > > > That's not what I read from the code: > > features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES; ifcvf_get_features() reads device feature bits(which should always has ACCSSS_PLATFORM) and IFCVF_SUPPORTED_FEATURES is the driver supported feature bits which hard coded ACCESS_PLATFORM, so the intersection should include ACCESS_PLATFORM. the intersection "features" is returned in get_features(), qemu should set features according to it. > > >> Now we check whether device support this feature bit as a double >> conformation, are you suggesting we should check whether >> ACCESS_PLATFORM & IFCVF_SUPPORTED_FEATURES >> in set_features() as well? > > > If we know device will always offer ACCESS_PLATFORM, there's no need > to check it again. What we should check if whether driver set that, > and if it doesn't we need to fail set_features(). I think there's > little chance that IFCVF can work when IOMMU_PLATFORM is not negotiated. Agree, will check the features bit to set instead of device feature bits. Thanks! > > > >> I prefer check both device and IFCVF_SUPPORTED_FEATURES both, more >> reliable. > > > So again, if you want to check device features, set_features() is not > the proper place. We need to fail the probe in this case. > > Thanks > > >> >> Thanks! >>> >>> Thanks >>> >>> >>>> >>>> Thanks！ >>>>> >>>>> >>>>>> + if (ret) >>>>>> + return ret; >>>>>> vf->req_features = features; >>>>> >>>>> _______________________________________________ >>>>> Virtualization mailing list >>>>> Virtualization@lists.linux-foundation.org >>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization >>>> >>> >> >
On 2021/3/12 2:40 下午, Zhu, Lingshan wrote: > > > On 3/12/2021 1:52 PM, Jason Wang wrote: >> >> On 2021/3/11 3:19 下午, Zhu, Lingshan wrote: >>> >>> >>> On 3/11/2021 2:20 PM, Jason Wang wrote: >>>> >>>> On 2021/3/11 12:16 下午, Zhu Lingshan wrote: >>>>> >>>>> >>>>> On 3/11/2021 11:20 AM, Jason Wang wrote: >>>>>> >>>>>> On 2021/3/10 5:00 下午, Zhu Lingshan wrote: >>>>>>> vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit >>>>>>> examines this when set features. >>>>>>> >>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>>>>>> --- >>>>>>> drivers/vdpa/ifcvf/ifcvf_base.c | 8 ++++++++ >>>>>>> drivers/vdpa/ifcvf/ifcvf_base.h | 1 + >>>>>>> drivers/vdpa/ifcvf/ifcvf_main.c | 5 +++++ >>>>>>> 3 files changed, 14 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c >>>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.c >>>>>>> index ea6a78791c9b..58f47fdce385 100644 >>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c >>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c >>>>>>> @@ -224,6 +224,14 @@ u64 ifcvf_get_features(struct ifcvf_hw *hw) >>>>>>> return hw->hw_features; >>>>>>> } >>>>>>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw) >>>>>>> +{ >>>>>>> + if (!(hw->hw_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM))) >>>>>>> + return -EINVAL; >>>>>>> + >>>>>>> + return 0; >>>>>>> +} >>>>>>> + >>>>>>> void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset, >>>>>>> void *dst, int length) >>>>>>> { >>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h >>>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h >>>>>>> index dbb8c10aa3b1..91c5735d4dc9 100644 >>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h >>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h >>>>>>> @@ -123,6 +123,7 @@ 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_hw_features(struct ifcvf_hw *hw); >>>>>>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw); >>>>>>> u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid); >>>>>>> int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num); >>>>>>> struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw); >>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c >>>>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c >>>>>>> index 25fb9dfe23f0..f624f202447d 100644 >>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c >>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c >>>>>>> @@ -179,6 +179,11 @@ static u64 ifcvf_vdpa_get_features(struct >>>>>>> vdpa_device *vdpa_dev) >>>>>>> static int ifcvf_vdpa_set_features(struct vdpa_device >>>>>>> *vdpa_dev, u64 features) >>>>>>> { >>>>>>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); >>>>>>> + int ret; >>>>>>> + >>>>>>> + ret = ifcvf_verify_min_features(vf); >>>>>> >>>>>> >>>>>> So this validate device features instead of driver which is the >>>>>> one we really want to check? >>>>>> >>>>>> Thanks >>>>> >>>>> Hi Jason, >>>>> >>>>> Here we check device feature bits to make sure the device support >>>>> ACCESS_PLATFORM. >>>> >>>> >>>> If you want to check device features, you need to do that during >>>> probe() and fail the probing if without the feature. But I think >>>> you won't ship cards without ACCESS_PLATFORM. >>> Yes, there are no reasons ship a card without ACCESS_PLATFORM >>>> >>>> >>>>> In get_features(), >>>>> it will return a intersection of device features bit and driver >>>>> supported features bits(which includes ACCESS_PLATFORM). >>>>> Other components like QEMU should not set features bits more than >>>>> this intersection of bits. so we can make sure if this >>>>> ifcvf_verify_min_features() passed, both device and driver support >>>>> ACCESS_PLATFORM. >>>>> >>>>> Are you suggesting check driver feature bits in >>>>> ifcvf_verify_min_features() in the meantime as well? >>>> >>>> >>>> So it really depends on your hardware. If you hardware can always >>>> offer ACCESS_PLATFORM, you just need to check driver features. This >>>> is how vdpa_sim and mlx5_vdpa work. >>> Yes, we always support ACCESS_PLATFORM, so it is hard coded in the >>> macro IFCVF_SUPPORTED_FEATURES. >> >> >> That's not what I read from the code: >> >> features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES; > ifcvf_get_features() reads device feature bits(which should always has > ACCSSS_PLATFORM) and IFCVF_SUPPORTED_FEATURES is the driver supported > feature bits For "driver" you probably mean IFCVF. So there's some misunderstanding before, what I meant for "driver" is virtio driver that do feature negotaitation with the device. I wonder what features are supported by the device but not the IFCVF driver? Thanks > which hard coded ACCESS_PLATFORM, so the intersection should include > ACCESS_PLATFORM. > the intersection "features" is returned in get_features(), qemu should > set features according to it. >> >> >>> Now we check whether device support this feature bit as a double >>> conformation, are you suggesting we should check whether >>> ACCESS_PLATFORM & IFCVF_SUPPORTED_FEATURES >>> in set_features() as well? >> >> >> If we know device will always offer ACCESS_PLATFORM, there's no need >> to check it again. What we should check if whether driver set that, >> and if it doesn't we need to fail set_features(). I think there's >> little chance that IFCVF can work when IOMMU_PLATFORM is not negotiated. > Agree, will check the features bit to set instead of device feature > bits. Thanks! >> >> >> >>> I prefer check both device and IFCVF_SUPPORTED_FEATURES both, more >>> reliable. >> >> >> So again, if you want to check device features, set_features() is not >> the proper place. We need to fail the probe in this case. >> >> Thanks >> >> >>> >>> Thanks! >>>> >>>> Thanks >>>> >>>> >>>>> >>>>> Thanks！ >>>>>> >>>>>> >>>>>>> + if (ret) >>>>>>> + return ret; >>>>>>> vf->req_features = features; >>>>>> >>>>>> _______________________________________________ >>>>>> Virtualization mailing list >>>>>> Virtualization@lists.linux-foundation.org >>>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization >>>>> >>>> >>> >> >
On 3/12/2021 3:00 PM, Jason Wang wrote: > > On 2021/3/12 2:40 下午, Zhu, Lingshan wrote: >> >> >> On 3/12/2021 1:52 PM, Jason Wang wrote: >>> >>> On 2021/3/11 3:19 下午, Zhu, Lingshan wrote: >>>> >>>> >>>> On 3/11/2021 2:20 PM, Jason Wang wrote: >>>>> >>>>> On 2021/3/11 12:16 下午, Zhu Lingshan wrote: >>>>>> >>>>>> >>>>>> On 3/11/2021 11:20 AM, Jason Wang wrote: >>>>>>> >>>>>>> On 2021/3/10 5:00 下午, Zhu Lingshan wrote: >>>>>>>> vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit >>>>>>>> examines this when set features. >>>>>>>> >>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>>>>>>> --- >>>>>>>> drivers/vdpa/ifcvf/ifcvf_base.c | 8 ++++++++ >>>>>>>> drivers/vdpa/ifcvf/ifcvf_base.h | 1 + >>>>>>>> drivers/vdpa/ifcvf/ifcvf_main.c | 5 +++++ >>>>>>>> 3 files changed, 14 insertions(+) >>>>>>>> >>>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c >>>>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.c >>>>>>>> index ea6a78791c9b..58f47fdce385 100644 >>>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c >>>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c >>>>>>>> @@ -224,6 +224,14 @@ u64 ifcvf_get_features(struct ifcvf_hw *hw) >>>>>>>> return hw->hw_features; >>>>>>>> } >>>>>>>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw) >>>>>>>> +{ >>>>>>>> + if (!(hw->hw_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM))) >>>>>>>> + return -EINVAL; >>>>>>>> + >>>>>>>> + return 0; >>>>>>>> +} >>>>>>>> + >>>>>>>> void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset, >>>>>>>> void *dst, int length) >>>>>>>> { >>>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h >>>>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h >>>>>>>> index dbb8c10aa3b1..91c5735d4dc9 100644 >>>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h >>>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h >>>>>>>> @@ -123,6 +123,7 @@ 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_hw_features(struct ifcvf_hw *hw); >>>>>>>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw); >>>>>>>> u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid); >>>>>>>> int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num); >>>>>>>> struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw); >>>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c >>>>>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c >>>>>>>> index 25fb9dfe23f0..f624f202447d 100644 >>>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c >>>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c >>>>>>>> @@ -179,6 +179,11 @@ static u64 ifcvf_vdpa_get_features(struct >>>>>>>> vdpa_device *vdpa_dev) >>>>>>>> static int ifcvf_vdpa_set_features(struct vdpa_device >>>>>>>> *vdpa_dev, u64 features) >>>>>>>> { >>>>>>>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); >>>>>>>> + int ret; >>>>>>>> + >>>>>>>> + ret = ifcvf_verify_min_features(vf); >>>>>>> >>>>>>> >>>>>>> So this validate device features instead of driver which is the >>>>>>> one we really want to check? >>>>>>> >>>>>>> Thanks >>>>>> >>>>>> Hi Jason, >>>>>> >>>>>> Here we check device feature bits to make sure the device support >>>>>> ACCESS_PLATFORM. >>>>> >>>>> >>>>> If you want to check device features, you need to do that during >>>>> probe() and fail the probing if without the feature. But I think >>>>> you won't ship cards without ACCESS_PLATFORM. >>>> Yes, there are no reasons ship a card without ACCESS_PLATFORM >>>>> >>>>> >>>>>> In get_features(), >>>>>> it will return a intersection of device features bit and driver >>>>>> supported features bits(which includes ACCESS_PLATFORM). >>>>>> Other components like QEMU should not set features bits more than >>>>>> this intersection of bits. so we can make sure if this >>>>>> ifcvf_verify_min_features() passed, both device and driver >>>>>> support ACCESS_PLATFORM. >>>>>> >>>>>> Are you suggesting check driver feature bits in >>>>>> ifcvf_verify_min_features() in the meantime as well? >>>>> >>>>> >>>>> So it really depends on your hardware. If you hardware can always >>>>> offer ACCESS_PLATFORM, you just need to check driver features. >>>>> This is how vdpa_sim and mlx5_vdpa work. >>>> Yes, we always support ACCESS_PLATFORM, so it is hard coded in the >>>> macro IFCVF_SUPPORTED_FEATURES. >>> >>> >>> That's not what I read from the code: >>> >>> features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES; >> ifcvf_get_features() reads device feature bits(which should always >> has ACCSSS_PLATFORM) and IFCVF_SUPPORTED_FEATURES is the driver >> supported feature bits > > > For "driver" you probably mean IFCVF. So there's some misunderstanding > before, what I meant for "driver" is virtio driver that do feature > negotaitation with the device. > > I wonder what features are supported by the device but not the IFCVF > driver? > > Thanks we did not use TSO hardware feature bits in IFCVF driver for now. Anyway, we will check the features bits to set in set_features than hw/ifcvf driver feature bits. THanks! > > >> which hard coded ACCESS_PLATFORM, so the intersection should include >> ACCESS_PLATFORM. >> the intersection "features" is returned in get_features(), qemu >> should set features according to it. >>> >>> >>>> Now we check whether device support this feature bit as a double >>>> conformation, are you suggesting we should check whether >>>> ACCESS_PLATFORM & IFCVF_SUPPORTED_FEATURES >>>> in set_features() as well? >>> >>> >>> If we know device will always offer ACCESS_PLATFORM, there's no need >>> to check it again. What we should check if whether driver set that, >>> and if it doesn't we need to fail set_features(). I think there's >>> little chance that IFCVF can work when IOMMU_PLATFORM is not >>> negotiated. >> Agree, will check the features bit to set instead of device feature >> bits. Thanks! >>> >>> >>> >>>> I prefer check both device and IFCVF_SUPPORTED_FEATURES both, more >>>> reliable. >>> >>> >>> So again, if you want to check device features, set_features() is >>> not the proper place. We need to fail the probe in this case. >>> >>> Thanks >>> >>> >>>> >>>> Thanks! >>>>> >>>>> Thanks >>>>> >>>>> >>>>>> >>>>>> Thanks！ >>>>>>> >>>>>>> >>>>>>>> + if (ret) >>>>>>>> + return ret; >>>>>>>> vf->req_features = features; >>>>>>> >>>>>>> _______________________________________________ >>>>>>> Virtualization mailing list >>>>>>> Virtualization@lists.linux-foundation.org >>>>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization >>>>>> >>>>> >>>> >>> >> >
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c index ea6a78791c9b..58f47fdce385 100644 --- a/drivers/vdpa/ifcvf/ifcvf_base.c +++ b/drivers/vdpa/ifcvf/ifcvf_base.c @@ -224,6 +224,14 @@ u64 ifcvf_get_features(struct ifcvf_hw *hw) return hw->hw_features; } +int ifcvf_verify_min_features(struct ifcvf_hw *hw) +{ + if (!(hw->hw_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM))) + return -EINVAL; + + return 0; +} + void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset, void *dst, int length) { diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h index dbb8c10aa3b1..91c5735d4dc9 100644 --- a/drivers/vdpa/ifcvf/ifcvf_base.h +++ b/drivers/vdpa/ifcvf/ifcvf_base.h @@ -123,6 +123,7 @@ 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_hw_features(struct ifcvf_hw *hw); +int ifcvf_verify_min_features(struct ifcvf_hw *hw); u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid); int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num); struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw); diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c index 25fb9dfe23f0..f624f202447d 100644 --- a/drivers/vdpa/ifcvf/ifcvf_main.c +++ b/drivers/vdpa/ifcvf/ifcvf_main.c @@ -179,6 +179,11 @@ static u64 ifcvf_vdpa_get_features(struct vdpa_device *vdpa_dev) static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, u64 features) { struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); + int ret; + + ret = ifcvf_verify_min_features(vf); + if (ret) + return ret; vf->req_features = features;
vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit examines this when set features. Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> --- drivers/vdpa/ifcvf/ifcvf_base.c | 8 ++++++++ drivers/vdpa/ifcvf/ifcvf_base.h | 1 + drivers/vdpa/ifcvf/ifcvf_main.c | 5 +++++ 3 files changed, 14 insertions(+)