On 8/16/2022 2:13 PM, Michael S. Tsirkin wrote:
On Tue, Aug 16, 2022 at 12:41:21AM -0700, 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?
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.
Thanks,
-Siwei
I'm not sure who you are asking. I didn't realize this is so
controversial. If you feel it should be reverted I suggest
you post a revert patch with a detailed motivation and this
will get the discussion going.
Leave it around then, until the next person shout out aloud. I don't
mind taking personal time to help, though my impression of the past
conversation is that this is less productive way of cooperation and
collaboration.
Please safely ignore me from now on.
-Siwei
It will also help if you stress whether you describe theoretical
issues or something observed in practice above
discussion does not make this clear.
