Re: [PATCH v3 03/10] vdpa: Read device configuration only if FEATURES_OK

On Tue, Dec 21, 2021 at 2:17 PM Eli Cohen [off-list ref] wrote:

On Tue, Dec 21, 2021 at 01:51:32PM +0800, Jason Wang wrote:

quoted

On Sun, Dec 19, 2021 at 10:03 PM Eli Cohen [off-list ref] wrote:

quoted

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 <redacted>
---
 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?

Thanks

quoted

Thanks

quoted

        hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
                          VDPA_CMD_DEV_CONFIG_GET);
        if (!hdr)
--
2.34.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization