Jason Wang
2021-Dec-21 09:02 UTC
[PATCH v3 03/10] vdpa: Read device configuration only if FEATURES_OKg
On Tue, Dec 21, 2021 at 5:00 PM Eli Cohen <elic at nvidia.com> wrote:> > On Tue, Dec 21, 2021 at 03:51:47PM +0800, Jason Wang wrote: > > On Tue, Dec 21, 2021 at 2:17 PM Eli Cohen <elic at nvidia.com> wrote: > > > > > > On Tue, Dec 21, 2021 at 01:51:32PM +0800, Jason Wang wrote: > > > > On Sun, Dec 19, 2021 at 10:03 PM Eli Cohen <elic at nvidia.com> wrote: > > > > > > > > > > Avoid reading device configuration during feature negotiation. Read > > > > > device status and verify that VIRTIO_CONFIG_S_FEATURES_OK is set. > > > > > Otherwise, return -EAGAIN. > > > > > > > > > > Signed-off-by: Eli Cohen <elic at nvidia.com> > > > > > --- > > > > > drivers/vdpa/vdpa.c | 7 +++++++ > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c > > > > > index 42d71d60d5dc..5749cf0a1500 100644 > > > > > --- a/drivers/vdpa/vdpa.c > > > > > +++ b/drivers/vdpa/vdpa.c > > > > > @@ -819,8 +819,15 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, > > > > > { > > > > > u32 device_id; > > > > > void *hdr; > > > > > + u8 status; > > > > > int err; > > > > > > > > > > + status = vdev->config->get_status(vdev); > > > > > + if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) { > > > > > + NL_SET_ERR_MSG_MOD(extack, "Features negotiation not completed"); > > > > > + return -EAGAIN; > > > > > + } > > > > > + > > > > > > > > I wonder how we synchronize this with set_status(), set/get_config()? > > > > > > > > > > Not sure why set/get_config() is related to this. > > > > I think guest may trigger set_config() while we are at > > vdpa_dev_config_fill(), this will lead to inconsistent values. > > > > > How about each upstream driver will take care to have a lock that > > > serializes set_status() with get_driver_features() to guarantee that we > > > always provide valid features? > > > > Yes and it looks like we've already had a per device cf_mutex, how > > about simply use this? > > Maybe do it in one place. For example, for vhost, in > drivers/vhost/vdpa.c: > > mutex_lock(&vdpa->cf_mutex); > ops->set_status(vdpa, status); > mutex_unlock(&vdpa->cf_mutex); > > same for drivers/virtio/virtio_vdpa.c > > OVerall, set_status, get_status, set_config, get_config should be > protected.Yes, maybe we can introduce new helpers in vdpa.c. Thanks> > > > > > Thanks > > > > > > > > > Thanks > > > > > > > > > hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags, > > > > > VDPA_CMD_DEV_CONFIG_GET); > > > > > if (!hdr) > > > > > -- > > > > > 2.34.1 > > > > > > > > > > > > > > >