On 8/16/2022 1:29 AM, Zhu, Lingshan wrote:
On 8/16/2022 3:41 PM, Si-Wei Liu wrote:
Hi Michael,
I just noticed this patch got pulled to linux-next prematurely
without getting consensus on code review, am not sure why. Hope it
was just an oversight.
Unfortunately this introduced functionality regression to at least
two cases so far as I see:
1. (bogus) VDPA_ATTR_DEV_NEGOTIATED_FEATURES are inadvertently
exposed and displayed in "vdpa dev config show" before feature
negotiation is done. Noted the corresponding features name shown in
vdpa tool is called "negotiated_features" rather than
"driver_features". I see in no way the intended change of the patch
should break this user level expectation regardless of any spec
requirement. Do you agree on this point?
I will post a patch for iptour2, doing:
1) if iprout2 does not get driver_features from the kernel, then don't
show negotiated features in the command output
This won't work as the vdpa userspace tool won't know *when* features
are negotiated. There's no guarantee in the kernel to assume 0 will be
returned from vendor driver during negotiation. On the other hand, with
the supposed change, userspace can't tell if there's really none of
features negotiated, or the feature negotiation is over. Before the
change the userspace either gets all the attributes when feature
negotiation is over, or it gets nothing when it's ongoing, so there was
a distinction.This expectation of what "negotiated_features" represents
is established from day one, I see no reason the intended kernel change
to show other attributes should break userspace behavior and user's
expectation.
2) process and decoding the device features.
2. There was also another implicit assumption that is broken by this
patch. There could be a vdpa tool query of config via
vdpa_dev_net_config_fill()->vdpa_get_config_unlocked() that races
with the first vdpa_set_features() call from VMM e.g. QEMU. Since the
S_FEATURES_OK blocking condition is removed, if the vdpa tool query
occurs earlier than the first set_driver_features() call from VMM,
the following code will treat the guest as legacy and then trigger an
erroneous vdpa_set_features_unlocked(... , 0) call to the vdpa driver:
374 /*
375 * Config accesses aren't supposed to trigger before
features are set.
376 * If it does happen we assume a legacy guest.
377 */
378 if (!vdev->features_valid)
379 vdpa_set_features_unlocked(vdev, 0);
380 ops->get_config(vdev, offset, buf, len);
Depending on vendor driver's implementation, L380 may either return
invalid config data (or invalid endianness if on BE) or only config
fields that are valid in legacy layout. What's more severe is that,
vdpa tool query in theory shouldn't affect feature negotiation at all
by making confusing calls to the device, but now it is possible with
the patch. Fixing this would require more delicate work on the other
paths involving the cf_lock reader/write semaphore.
Not sure what you plan to do next, post the fixes for both issues and
get the community review? Or simply revert the patch in question? Let
us know.
The spec says:
The device MUST allow reading of any device-specific configuration
field before FEATURES_OK is set by
the driver. This includes fields which are conditional on feature
bits, as long as those feature bits are offered
by the device.
so whether FEATURES_OK should not block reading the device config
space. vdpa_get_config_unlocked() will read the features, I don't know
why it has a comment:
/*
* Config accesses aren't supposed to trigger before features
are set.
* If it does happen we assume a legacy guest.
*/
This conflicts with the spec.
vdpa_get_config_unlocked() checks vdev->features_valid, if not valid,
it will set the drivers_features 0, I think this intends to prevent
reading random driver_features. This function does not hold any locks,
and didn't change anything.
So what is the race?
You'll see the race if you keep 'vdpa dev config show ...' running in a
tight loop while launching a VM with the vDPA device under query.
-Siwei
Thanks
Thanks,
-Siwei
On 8/12/2022 3:44 AM, Zhu Lingshan wrote:
Users may want to query the config space of a vDPA device,
to choose a appropriate one for a certain guest. This means the
users need to read the config space before FEATURES_OK, and
the existence of config space contents does not depend on
FEATURES_OK.
The spec says:
The device MUST allow reading of any device-specific configuration
field before FEATURES_OK is set by the driver. This includes
fields which are conditional on feature bits, as long as those
feature bits are offered by the device.
Signed-off-by: Zhu Lingshan <[email protected]>
---
drivers/vdpa/vdpa.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 6eb3d972d802..bf312d9c59ab 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -855,17 +855,9 @@ vdpa_dev_config_fill(struct vdpa_device *vdev,
struct sk_buff *msg, u32 portid,
{
u32 device_id;
void *hdr;
- u8 status;
int err;
down_read(&vdev->cf_lock);
- status = vdev->config->get_status(vdev);
- if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
- NL_SET_ERR_MSG_MOD(extack, "Features negotiation not
completed");
- err = -EAGAIN;
- goto out;
- }
-
hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
VDPA_CMD_DEV_CONFIG_GET);
if (!hdr) {
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
