| Message ID | 20191019063810.6944-5-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libqos: add VIRTIO PCI 1.0 support | expand |
On 19/10/2019 08.37, Stefan Hajnoczi wrote: > VIRTIO Device Initialization requires feature negotiation. Currently > virtio-scsi-test.c is non-compliant. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > tests/virtio-scsi-test.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c > index 7c8f9b27f8..0415e75876 100644 > --- a/tests/virtio-scsi-test.c > +++ b/tests/virtio-scsi-test.c > @@ -123,10 +123,16 @@ static QVirtioSCSIQueues *qvirtio_scsi_init(QVirtioDevice *dev) > QVirtioSCSIQueues *vs; > const uint8_t test_unit_ready_cdb[VIRTIO_SCSI_CDB_SIZE] = {}; > struct virtio_scsi_cmd_resp resp; > + uint64_t features; > int i; > > vs = g_new0(QVirtioSCSIQueues, 1); > vs->dev = dev; > + > + features = qvirtio_get_features(dev); > + features &= ~(QVIRTIO_F_BAD_FEATURE | (1ull << VIRTIO_RING_F_EVENT_IDX)); > + qvirtio_set_features(dev, features); I wonder whether this get_feat + "&=" + set_feat is really the right way here? Maybe we should rather only set the feature bits that we care about instead of setting all but BAD_FEATURE and RING_F_EVENT_IDX? Otherwise, please mention in the commit message why you've chosen to disable just these two bits. Thanks, Thomas
On Mon, Oct 21, 2019 at 02:08:34PM +0200, Thomas Huth wrote: > On 19/10/2019 08.37, Stefan Hajnoczi wrote: > > VIRTIO Device Initialization requires feature negotiation. Currently > > virtio-scsi-test.c is non-compliant. > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > tests/virtio-scsi-test.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c > > index 7c8f9b27f8..0415e75876 100644 > > --- a/tests/virtio-scsi-test.c > > +++ b/tests/virtio-scsi-test.c > > @@ -123,10 +123,16 @@ static QVirtioSCSIQueues *qvirtio_scsi_init(QVirtioDevice *dev) > > QVirtioSCSIQueues *vs; > > const uint8_t test_unit_ready_cdb[VIRTIO_SCSI_CDB_SIZE] = {}; > > struct virtio_scsi_cmd_resp resp; > > + uint64_t features; > > int i; > > > > vs = g_new0(QVirtioSCSIQueues, 1); > > vs->dev = dev; > > + > > + features = qvirtio_get_features(dev); > > + features &= ~(QVIRTIO_F_BAD_FEATURE | (1ull << VIRTIO_RING_F_EVENT_IDX)); > > + qvirtio_set_features(dev, features); > > I wonder whether this get_feat + "&=" + set_feat is really the right way > here? Maybe we should rather only set the feature bits that we care > about instead of setting all but BAD_FEATURE and RING_F_EVENT_IDX? > > Otherwise, please mention in the commit message why you've chosen to > disable just these two bits. Enabling all features (except BAD_FEATURE and EVENT_IDX) is standard practice in virtio qtests that I didn't want to diverge from. I can mention this in the commit description and explain that BAD_FEATURE must never be set (i.e. drivers that do so are considered broken) and EVENT_IDX isn't supported by the libqos virtio code. Stefan
diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c index 7c8f9b27f8..0415e75876 100644 --- a/tests/virtio-scsi-test.c +++ b/tests/virtio-scsi-test.c @@ -123,10 +123,16 @@ static QVirtioSCSIQueues *qvirtio_scsi_init(QVirtioDevice *dev) QVirtioSCSIQueues *vs; const uint8_t test_unit_ready_cdb[VIRTIO_SCSI_CDB_SIZE] = {}; struct virtio_scsi_cmd_resp resp; + uint64_t features; int i; vs = g_new0(QVirtioSCSIQueues, 1); vs->dev = dev; + + features = qvirtio_get_features(dev); + features &= ~(QVIRTIO_F_BAD_FEATURE | (1ull << VIRTIO_RING_F_EVENT_IDX)); + qvirtio_set_features(dev, features); + vs->num_queues = qvirtio_config_readl(dev, 0); g_assert_cmpint(vs->num_queues, <, MAX_NUM_QUEUES); @@ -135,6 +141,8 @@ static QVirtioSCSIQueues *qvirtio_scsi_init(QVirtioDevice *dev) vs->vq[i] = qvirtqueue_setup(dev, alloc, i); } + qvirtio_set_driver_ok(dev); + /* Clear the POWER ON OCCURRED unit attention */ g_assert_cmpint(virtio_scsi_do_command(vs, test_unit_ready_cdb, NULL, 0, NULL, 0, &resp),
VIRTIO Device Initialization requires feature negotiation. Currently virtio-scsi-test.c is non-compliant. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- tests/virtio-scsi-test.c | 8 ++++++++ 1 file changed, 8 insertions(+)