Re: [PATCH v3 2/6] vdpa: conditionally read STATUS in config space

[Eli Cohen]

On 06/02/2023 6:53, Si-Wei Liu wrote:
> On 2/5/2023 12:26 AM, Eli Cohen wrote:
> > On 03/02/2023 7:01, Si-Wei Liu wrote:
> > > The spec says:
> > >      status only exists if VIRTIO_NET_F_STATUS is set
> > >
> > > Similar to MAC and MTU, vdpa_dev_net_config_fill() should read
> > > STATUS conditionally depending on the feature bits.
> > >
> > > Signed-off-by: Si-Wei Liu <si-wei.liu@xxxxxxxxxx>
> > > Reviewed-by: Parav Pandit <parav@xxxxxxxxxx>
> > > ---
> > >   drivers/vdpa/vdpa.c | 20 +++++++++++++++-----
> > >   1 file changed, 15 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > > index 3a82ca78..21c8aa3 100644
> > > --- a/drivers/vdpa/vdpa.c
> > > +++ b/drivers/vdpa/vdpa.c
> > > @@ -843,18 +843,25 @@ static int vdpa_dev_net_mac_config_fill(struct 
> > > sk_buff *msg, u64 features,
> > >               sizeof(config->mac), config->mac);
> > >   }
> > >   +static int vdpa_dev_net_status_config_fill(struct sk_buff *msg, 
> > > u64 features,
> > > +                       const struct virtio_net_config *config)
> > > +{
> > > +    u16 val_u16;
> > > +
> > > +    if ((features & BIT_ULL(VIRTIO_NET_F_STATUS)) == 0)
> > > +        return 0;
> > Instead of returning 0 here, it would be better to explicitly put 0 
> > in the message field for
> >
> > VDPA_ATTR_DEV_NET_STATUS
> In light of commit 41a2ad927aa2 ("vDPA: conditionally read MTU and MAC 
> in dev cfg space"), the userspace must now show the config space field 
> presented by the device *as-is*. If the feature bit is not offered by 
> device, the relevant field will not be displayed in 'vdpa dev config' 
> output. For instance, MAC address won't be shown if the MAC feature is 
> not supported/offered by the device (note this has nothing to do with 
> negotiated features), even though the vdpa parent may have a non-zero 
> MAC address of its own. I think STATUS should not be different from 
> MAC and MTU here.

OK, I see your point.

Reviewed-by: Eli Cohen <elic@xxxxxxxxxx>

> Regards,
> -Siwei
>
> > > +
> > > +    val_u16 = __virtio16_to_cpu(true, config->status);
> > > +    return nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16);
> > > +}
> > > +
> > >   static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, 
> > > struct sk_buff *msg)
> > >   {
> > >       struct virtio_net_config config = {};
> > >       u64 features_device;
> > > -    u16 val_u16;
> > >         vdev->config->get_config(vdev, 0, &config, sizeof(config));
> > >   -    val_u16 = __virtio16_to_cpu(true, config.status);
> > > -    if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
> > > -        return -EMSGSIZE;
> > > -
> > >       features_device = vdev->config->get_device_features(vdev);
> > >         if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_FEATURES, 
> > > features_device,
> > > @@ -867,6 +874,9 @@ static int vdpa_dev_net_config_fill(struct 
> > > vdpa_device *vdev, struct sk_buff *ms
> > >       if (vdpa_dev_net_mac_config_fill(msg, features_device, &config))
> > >           return -EMSGSIZE;
> > >   +    if (vdpa_dev_net_status_config_fill(msg, features_device, 
> > > &config))
> > > +        return -EMSGSIZE;
> > > +
> > >       return vdpa_dev_net_mq_config_fill(msg, features_device, 
> > > &config);
> > >   }