On 7/28/2022 4:35 AM, Michael S. Tsirkin wrote:
On Thu, Jul 28, 2022 at 12:08:53AM -0700, Si-Wei Liu wrote:
On 7/27/2022 7:06 PM, Jason Wang wrote:
在 2022/7/28 08:56, Si-Wei Liu 写道:
On 7/27/2022 4:47 AM, Zhu, Lingshan wrote:
On 7/27/2022 5:43 PM, Si-Wei Liu wrote:
Sorry to chime in late in the game. For some reason I
couldn't get to most emails for this discussion (I only
subscribed to the virtualization list), while I was taking
off amongst the past few weeks.
It looks to me this patch is incomplete. Noted down the way
in vdpa_dev_net_config_fill(), we have the following:
features = vdev->config->get_driver_features(vdev);
if (nla_put_u64_64bit(msg,
VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
VDPA_ATTR_PAD))
return -EMSGSIZE;
Making call to .get_driver_features() doesn't make sense
when feature negotiation isn't complete. Neither should
present negotiated_features to userspace before negotiation
is done.
Similarly, max_vqp through vdpa_dev_net_mq_config_fill()
probably should not show before negotiation is done - it
depends on driver features negotiated.
I have another patch in this series introduces device_features
and will report device_features to the userspace even features
negotiation not done. Because the spec says we should allow
driver access the config space before FEATURES_OK.
The config space can be accessed by guest before features_ok doesn't
necessarily mean the value is valid.
It's valid as long as the device offers the feature:
"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."
I guess this statement only conveys that the field in config space can be
read before FEATURES_OK is set, though it does not *explicitly* states the
validity of field.
And looking at:
"The mac address field always exists (though is only valid if
VIRTIO_NET_F_MAC is set), and status only exists if VIRTIO_NET_F_STATUS is
set."
It appears to me there's a border line set between "exist" and "valid". If I
understand the spec wording correctly, a spec-conforming device
implementation may or may not offer valid status value in the config space
when VIRTIO_NET_F_STATUS is offered, but before the feature is negotiated.
On the other hand, config space should contain valid mac address the moment
VIRTIO_NET_F_MAC feature is offered, regardless being negotiated or not. By
that, there seems to be leeway for the device implementation to decide when
config space field may become valid, though for most of QEMU's software
virtio devices, valid value is present to config space the very first moment
when feature is offered.
"If the VIRTIO_NET_F_MAC feature bit is set, the configuration space mac
entry indicates the “physical” address of the network card, otherwise the
driver would typically generate a random local MAC address."
"If the VIRTIO_NET_F_STATUS feature bit is negotiated, the link status comes
from the bottom bit of status. Otherwise, the driver assumes it’s active."
And also there are special cases where the read of specific configuration
space field MUST be deferred to until FEATURES_OK is set:
"If the VIRTIO_BLK_F_CONFIG_WCE feature is negotiated, the cache mode can be
read or set through the writeback field. 0 corresponds to a writethrough
cache, 1 to a writeback cache11. The cache mode after reset can be either
writeback or writethrough. The actual mode can be determined by reading
writeback after feature negotiation."
"The driver MUST NOT read writeback before setting the FEATURES_OK device
status bit."
"If VIRTIO_BLK_F_CONFIG_WCE is negotiated but VIRTIO_BLK_F_FLUSH is not, the
device MUST initialize writeback to 0."
Since the spec doesn't explicitly mandate the validity of each config space
field when feature of concern is offered, to be safer we'd have to live with
odd device implementation. I know for sure QEMU software devices won't for
99% of these cases, but that's not what is currently defined in the spec.
Thanks for raising this subject. I started working on this in April:
https://urldefense.com/v3/__https://lists.oasis-open.org/archives/virtio-comment/202201/msg00068.html__;!!ACWV5N9M2RV99hQ!Os6QE_RJokx7Us9y7-5-ByVVLuy3oLuPodAdZWxwJw_aNkJY0o0H7691FI9aYSTRLVieASUD_eOu$
working now to address the comments.
Great, thank you very much!
Is there a link to the latest draft that reflects the change uptodate?
The one above with iterative feature negotiation seemed getting some
resistance because of backward incompatibility with older spec? Please
copy me in the loop when you have the draft ready. I am not in the
virtio-comment list.
Thanks,
-Siwei
You may want to double check with Michael for what he quoted earlier:
Nope:
2.5.1 Driver Requirements: Device Configuration Space
...
For optional configuration space fields, the driver MUST check
that the corresponding feature is offered
before accessing that part of the configuration space.
and how many driver bugs taking wrong assumption of the validity of
config space field without features_ok. I am not sure what use case
you want to expose config resister values for before features_ok, if
it's mostly for live migration I guess it's probably heading a wrong
direction.
I guess it's not for migration.
Then what's the other possible use case than live migration, were to expose
config space values? Troubleshooting config space discrepancy between vDPA
and the emulated virtio device in userspace? Or tracking changes in config
space across feature negotiation, but for what? It'd be beneficial to the
interface design if the specific use case can be clearly described...
For migration, a provision with the correct features/capability would be
sufficient.
Right, that's what I thought too. It doesn't need to expose config space
values, simply exporting all attributes for vdpa device creation will do the
work.
-Siwei
Thanks
Last but not the least, this "vdpa dev config" command was
not designed to display the real config space register
values in the first place. Quoting the vdpa-dev(8) man page:
vdpa dev config show - Show configuration of specific
device or all devices.
DEV - specifies the vdpa device to show its
configuration. If this argument is omitted all devices
configuration is listed.
It doesn't say anything about configuration space or
register values in config space. As long as it can convey
the config attribute when instantiating vDPA device
instance, and more importantly, the config can be easily
imported from or exported to userspace tools when trying to
reconstruct vdpa instance intact on destination host for
live migration, IMHO in my personal interpretation it
doesn't matter what the config space may present. It may be
worth while adding a new debug command to expose the real
register value, but that's another story.
I am not sure getting your points. vDPA now reports device
feature bits(device_features) and negotiated feature
bits(driver_features), and yes, the drivers features can be a
subset of the device features; and the vDPA device features can
be a subset of the management device features.
What I said is after unblocking the conditional check, you'd have to
handle the case for each of the vdpa attribute when feature
negotiation is not yet done: basically the register values you got
from config space via the vdpa_get_config_unlocked() call is not
considered to be valid before features_ok (per-spec). Although in
some case you may get sane value, such behavior is generally
undefined. If you desire to show just the device_features alone
without any config space field, which the device had advertised
*before feature negotiation is complete*, that'll be fine. But looks
to me this is not how patch has been implemented. Probably need some
more work?
Regards,
-Siwei
Having said, please consider to drop the Fixes tag, as
appears to me you're proposing a new feature rather than
fixing a real issue.
it's a new feature to report the device feature bits than only
negotiated features, however this patch is a must, or it will
block the device feature bits reporting. but I agree, the fix
tag is not a must.
Thanks,
-Siwei
On 7/1/2022 3:12 PM, Parav Pandit via Virtualization wrote:
From: Zhu Lingshan<lingshan.zhu@xxxxxxxxx>
Sent: Friday, July 1, 2022 9:28 AM
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.
Fixes: 30ef7a8ac8a07 (vdpa: Read device
configuration only if FEATURES_OK)
Fix is fine, but fixes tag needs correction described below.
Above commit id is 13 letters should be 12.
And
It should be in format
Fixes: 30ef7a8ac8a0 ("vdpa: Read device configuration
only if FEATURES_OK")
Please use checkpatch.pl script before posting the
patches to catch these errors.
There is a bot that looks at the fixes tag and
identifies the right kernel version to apply this fix.
Signed-off-by: Zhu Lingshan<lingshan.zhu@xxxxxxxxx>
---
drivers/vdpa/vdpa.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
9b0e39b2f022..d76b22b2f7ae 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -851,17 +851,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) {
--
2.31.1
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://urldefense.com/v3/__https://lists.linuxfoundation.org/mailman/listinfo/virtualization__;!!ACWV5N9M2RV99hQ!Pkwym7OAjoDucUqs2fAwchxqL8-BGd6wOl-51xcgB_yCNwPJ_cs8A1y-cYmrLTB4OBNsimnZuqJPcvQIl3g$
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
