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://lists.oasis-open.org/archives/virtio-comment/202201/msg00068.html working now to address the comments. > > > > > > > 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
