| Message ID | 20220909085712.46006-3-lingshan.zhu@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Conditionally read fields in dev cfg space | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote: > > vdpa_dev_net_config_fill() should only report driver features > to userspace after features negotiation is done. > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > --- > drivers/vdpa/vdpa.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c > index 798a02c7aa94..29d7e8858e6f 100644 > --- a/drivers/vdpa/vdpa.c > +++ b/drivers/vdpa/vdpa.c > @@ -819,6 +819,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms > struct virtio_net_config config = {}; > u64 features_device, features_driver; > u16 val_u16; > + u8 status; > > vdev->config->get_config(vdev, 0, &config, sizeof(config)); > > @@ -834,10 +835,14 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms > if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) > return -EMSGSIZE; > > - features_driver = vdev->config->get_driver_features(vdev); > - if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, > - VDPA_ATTR_PAD)) > - return -EMSGSIZE; > + /* only read driver features after the feature negotiation is done */ > + status = vdev->config->get_status(vdev); > + if (status & VIRTIO_CONFIG_S_FEATURES_OK) { Any reason this is not checked in its caller as what it used to do before? Thanks > + features_driver = vdev->config->get_driver_features(vdev); > + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, > + VDPA_ATTR_PAD)) > + return -EMSGSIZE; > + } > > features_device = vdev->config->get_device_features(vdev); > > -- > 2.31.1 >
On 9/20/2022 10:16 AM, Jason Wang wrote: > On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote: >> vdpa_dev_net_config_fill() should only report driver features >> to userspace after features negotiation is done. >> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >> --- >> drivers/vdpa/vdpa.c | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c >> index 798a02c7aa94..29d7e8858e6f 100644 >> --- a/drivers/vdpa/vdpa.c >> +++ b/drivers/vdpa/vdpa.c >> @@ -819,6 +819,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms >> struct virtio_net_config config = {}; >> u64 features_device, features_driver; >> u16 val_u16; >> + u8 status; >> >> vdev->config->get_config(vdev, 0, &config, sizeof(config)); >> >> @@ -834,10 +835,14 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms >> if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) >> return -EMSGSIZE; >> >> - features_driver = vdev->config->get_driver_features(vdev); >> - if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, >> - VDPA_ATTR_PAD)) >> - return -EMSGSIZE; >> + /* only read driver features after the feature negotiation is done */ >> + status = vdev->config->get_status(vdev); >> + if (status & VIRTIO_CONFIG_S_FEATURES_OK) { > Any reason this is not checked in its caller as what it used to do before? will check the existence of vdev->config->get_status before calling it in V2 Thanks, Zhu Lingshan > > Thanks > >> + features_driver = vdev->config->get_driver_features(vdev); >> + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, >> + VDPA_ATTR_PAD)) >> + return -EMSGSIZE; >> + } >> >> features_device = vdev->config->get_device_features(vdev); >> >> -- >> 2.31.1 >>
On Tue, Sep 20, 2022 at 1:46 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote: > > > > On 9/20/2022 10:16 AM, Jason Wang wrote: > > On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote: > >> vdpa_dev_net_config_fill() should only report driver features > >> to userspace after features negotiation is done. > >> > >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > >> --- > >> drivers/vdpa/vdpa.c | 13 +++++++++---- > >> 1 file changed, 9 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c > >> index 798a02c7aa94..29d7e8858e6f 100644 > >> --- a/drivers/vdpa/vdpa.c > >> +++ b/drivers/vdpa/vdpa.c > >> @@ -819,6 +819,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms > >> struct virtio_net_config config = {}; > >> u64 features_device, features_driver; > >> u16 val_u16; > >> + u8 status; > >> > >> vdev->config->get_config(vdev, 0, &config, sizeof(config)); > >> > >> @@ -834,10 +835,14 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms > >> if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) > >> return -EMSGSIZE; > >> > >> - features_driver = vdev->config->get_driver_features(vdev); > >> - if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, > >> - VDPA_ATTR_PAD)) > >> - return -EMSGSIZE; > >> + /* only read driver features after the feature negotiation is done */ > >> + status = vdev->config->get_status(vdev); > >> + if (status & VIRTIO_CONFIG_S_FEATURES_OK) { > > Any reason this is not checked in its caller as what it used to do before? > will check the existence of vdev->config->get_status before calling it in V2 Just to clarify, I meant to check FEAUTRES_OK in the caller - vdpa_dev_config_fill() otherwise each type needs to repeat this in their specific codes. Thanks > > Thanks, > Zhu Lingshan > > > > Thanks > > > >> + features_driver = vdev->config->get_driver_features(vdev); > >> + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, > >> + VDPA_ATTR_PAD)) > >> + return -EMSGSIZE; > >> + } > >> > >> features_device = vdev->config->get_device_features(vdev); > >> > >> -- > >> 2.31.1 > >> >
On 9/21/2022 10:14 AM, Jason Wang wrote: > On Tue, Sep 20, 2022 at 1:46 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote: >> >> >> On 9/20/2022 10:16 AM, Jason Wang wrote: >>> On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote: >>>> vdpa_dev_net_config_fill() should only report driver features >>>> to userspace after features negotiation is done. >>>> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>>> --- >>>> drivers/vdpa/vdpa.c | 13 +++++++++---- >>>> 1 file changed, 9 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c >>>> index 798a02c7aa94..29d7e8858e6f 100644 >>>> --- a/drivers/vdpa/vdpa.c >>>> +++ b/drivers/vdpa/vdpa.c >>>> @@ -819,6 +819,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms >>>> struct virtio_net_config config = {}; >>>> u64 features_device, features_driver; >>>> u16 val_u16; >>>> + u8 status; >>>> >>>> vdev->config->get_config(vdev, 0, &config, sizeof(config)); >>>> >>>> @@ -834,10 +835,14 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms >>>> if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) >>>> return -EMSGSIZE; >>>> >>>> - features_driver = vdev->config->get_driver_features(vdev); >>>> - if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, >>>> - VDPA_ATTR_PAD)) >>>> - return -EMSGSIZE; >>>> + /* only read driver features after the feature negotiation is done */ >>>> + status = vdev->config->get_status(vdev); >>>> + if (status & VIRTIO_CONFIG_S_FEATURES_OK) { >>> Any reason this is not checked in its caller as what it used to do before? >> will check the existence of vdev->config->get_status before calling it in V2 > Just to clarify, I meant to check FEAUTRES_OK in the caller - > vdpa_dev_config_fill() otherwise each type needs to repeat this in > their specific codes. if we check FEATURES_OK in the caller vdpa_dev_config_fill(), then !FEATURES_OK will block reporting all attributions, for example the device features and virtio device config space fields in this series and device status. Currently only driver features needs to check FEATURES_OK. Or did I missed anything? Thanks > > Thanks > >> Thanks, >> Zhu Lingshan >>> Thanks >>> >>>> + features_driver = vdev->config->get_driver_features(vdev); >>>> + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, >>>> + VDPA_ATTR_PAD)) >>>> + return -EMSGSIZE; >>>> + } >>>> >>>> features_device = vdev->config->get_device_features(vdev); >>>> >>>> -- >>>> 2.31.1 >>>>
On Wed, Sep 21, 2022 at 1:39 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote: > > > > On 9/21/2022 10:14 AM, Jason Wang wrote: > > On Tue, Sep 20, 2022 at 1:46 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote: > >> > >> > >> On 9/20/2022 10:16 AM, Jason Wang wrote: > >>> On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote: > >>>> vdpa_dev_net_config_fill() should only report driver features > >>>> to userspace after features negotiation is done. > >>>> > >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> > >>>> --- > >>>> drivers/vdpa/vdpa.c | 13 +++++++++---- > >>>> 1 file changed, 9 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c > >>>> index 798a02c7aa94..29d7e8858e6f 100644 > >>>> --- a/drivers/vdpa/vdpa.c > >>>> +++ b/drivers/vdpa/vdpa.c > >>>> @@ -819,6 +819,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms > >>>> struct virtio_net_config config = {}; > >>>> u64 features_device, features_driver; > >>>> u16 val_u16; > >>>> + u8 status; > >>>> > >>>> vdev->config->get_config(vdev, 0, &config, sizeof(config)); > >>>> > >>>> @@ -834,10 +835,14 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms > >>>> if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) > >>>> return -EMSGSIZE; > >>>> > >>>> - features_driver = vdev->config->get_driver_features(vdev); > >>>> - if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, > >>>> - VDPA_ATTR_PAD)) > >>>> - return -EMSGSIZE; > >>>> + /* only read driver features after the feature negotiation is done */ > >>>> + status = vdev->config->get_status(vdev); > >>>> + if (status & VIRTIO_CONFIG_S_FEATURES_OK) { > >>> Any reason this is not checked in its caller as what it used to do before? > >> will check the existence of vdev->config->get_status before calling it in V2 > > Just to clarify, I meant to check FEAUTRES_OK in the caller - > > vdpa_dev_config_fill() otherwise each type needs to repeat this in > > their specific codes. > if we check FEATURES_OK in the caller vdpa_dev_config_fill(), then > !FEATURES_OK will block reporting all attributions, for example > the device features and virtio device config space fields in this series > and device status. > Currently only driver features needs to check FEATURES_OK. > Or did I missed anything? I don't see much difference, we just move the following part to the caller, it is not the config but the VDPA_ATTR_DEV_NEGOTIATED_FEATURES is blocked. Thanks > > Thanks > > > > Thanks > > > >> Thanks, > >> Zhu Lingshan > >>> Thanks > >>> > >>>> + features_driver = vdev->config->get_driver_features(vdev); > >>>> + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, > >>>> + VDPA_ATTR_PAD)) > >>>> + return -EMSGSIZE; > >>>> + } > >>>> > >>>> features_device = vdev->config->get_device_features(vdev); > >>>> > >>>> -- > >>>> 2.31.1 > >>>> >
On 9/21/2022 3:43 PM, Jason Wang wrote: > On Wed, Sep 21, 2022 at 1:39 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote: >> >> >> On 9/21/2022 10:14 AM, Jason Wang wrote: >>> On Tue, Sep 20, 2022 at 1:46 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote: >>>> >>>> On 9/20/2022 10:16 AM, Jason Wang wrote: >>>>> On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote: >>>>>> vdpa_dev_net_config_fill() should only report driver features >>>>>> to userspace after features negotiation is done. >>>>>> >>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> >>>>>> --- >>>>>> drivers/vdpa/vdpa.c | 13 +++++++++---- >>>>>> 1 file changed, 9 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c >>>>>> index 798a02c7aa94..29d7e8858e6f 100644 >>>>>> --- a/drivers/vdpa/vdpa.c >>>>>> +++ b/drivers/vdpa/vdpa.c >>>>>> @@ -819,6 +819,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms >>>>>> struct virtio_net_config config = {}; >>>>>> u64 features_device, features_driver; >>>>>> u16 val_u16; >>>>>> + u8 status; >>>>>> >>>>>> vdev->config->get_config(vdev, 0, &config, sizeof(config)); >>>>>> >>>>>> @@ -834,10 +835,14 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms >>>>>> if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) >>>>>> return -EMSGSIZE; >>>>>> >>>>>> - features_driver = vdev->config->get_driver_features(vdev); >>>>>> - if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, >>>>>> - VDPA_ATTR_PAD)) >>>>>> - return -EMSGSIZE; >>>>>> + /* only read driver features after the feature negotiation is done */ >>>>>> + status = vdev->config->get_status(vdev); >>>>>> + if (status & VIRTIO_CONFIG_S_FEATURES_OK) { >>>>> Any reason this is not checked in its caller as what it used to do before? >>>> will check the existence of vdev->config->get_status before calling it in V2 >>> Just to clarify, I meant to check FEAUTRES_OK in the caller - >>> vdpa_dev_config_fill() otherwise each type needs to repeat this in >>> their specific codes. >> if we check FEATURES_OK in the caller vdpa_dev_config_fill(), then >> !FEATURES_OK will block reporting all attributions, for example >> the device features and virtio device config space fields in this series >> and device status. >> Currently only driver features needs to check FEATURES_OK. >> Or did I missed anything? > I don't see much difference, we just move the following part to the > caller, it is not the config but the VDPA_ATTR_DEV_NEGOTIATED_FEATURES > is blocked. OK, I get you. V2 will check FEATURES_OK and report driver features in vdpa_dev_config_fill() Thanks Zhu Lingshan > > Thanks > >> Thanks >>> Thanks >>> >>>> Thanks, >>>> Zhu Lingshan >>>>> Thanks >>>>> >>>>>> + features_driver = vdev->config->get_driver_features(vdev); >>>>>> + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, >>>>>> + VDPA_ATTR_PAD)) >>>>>> + return -EMSGSIZE; >>>>>> + } >>>>>> >>>>>> features_device = vdev->config->get_device_features(vdev); >>>>>> >>>>>> -- >>>>>> 2.31.1 >>>>>>
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index 798a02c7aa94..29d7e8858e6f 100644 --- a/drivers/vdpa/vdpa.c +++ b/drivers/vdpa/vdpa.c @@ -819,6 +819,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms struct virtio_net_config config = {}; u64 features_device, features_driver; u16 val_u16; + u8 status; vdev->config->get_config(vdev, 0, &config, sizeof(config)); @@ -834,10 +835,14 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16)) return -EMSGSIZE; - features_driver = vdev->config->get_driver_features(vdev); - if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, - VDPA_ATTR_PAD)) - return -EMSGSIZE; + /* only read driver features after the feature negotiation is done */ + status = vdev->config->get_status(vdev); + if (status & VIRTIO_CONFIG_S_FEATURES_OK) { + features_driver = vdev->config->get_driver_features(vdev); + if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver, + VDPA_ATTR_PAD)) + return -EMSGSIZE; + } features_device = vdev->config->get_device_features(vdev);
vdpa_dev_net_config_fill() should only report driver features to userspace after features negotiation is done. Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com> --- drivers/vdpa/vdpa.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)