Re: [PATCH v3 04/16] virtio-scsi-test: add missing feature negotiation

[Stefan Hajnoczi]

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 <address@hidden>
> > ---
> >  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

signature.asc
Description: PGP signature