Jason Wang
2021-Mar-11 03:20 UTC
[PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA
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 at 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; >
Zhu Lingshan
2021-Mar-11 04:15 UTC
[PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA
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 at 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_intersectionvf(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? > > ThanksHi 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 at lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Zhu Lingshan
2021-Mar-11 04:16 UTC
[PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA
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 at 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? > > ThanksHi 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 at lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization