| Message ID | 20191019063810.6944-8-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | libqos: add VIRTIO PCI 1.0 support | expand |
On 19/10/2019 08.38, Stefan Hajnoczi wrote: > According to VIRTIO 1.1 "3.1.1 Driver Requirements: Device > Initialization", configuration space and virtqueues cannot be accessed > before features have been negotiated. Enforce this requirement. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > tests/libqos/virtio.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c > index 4f7e6bb8a1..2593996c98 100644 > --- a/tests/libqos/virtio.c > +++ b/tests/libqos/virtio.c > @@ -13,23 +13,33 @@ > #include "standard-headers/linux/virtio_config.h" > #include "standard-headers/linux/virtio_ring.h" > > +/* Features must be negotiated before config space or virtqueue access */ > +static void check_features_negotiated(QVirtioDevice *d) > +{ > + g_assert_cmphex(d->features, !=, 0); > +} Isn't it "legal" to negotiate 0 feature bits, too (for legacy devices)? Thomas
On Mon, Oct 21, 2019 at 02:15:53PM +0200, Thomas Huth wrote: > On 19/10/2019 08.38, Stefan Hajnoczi wrote: > > According to VIRTIO 1.1 "3.1.1 Driver Requirements: Device > > Initialization", configuration space and virtqueues cannot be accessed > > before features have been negotiated. Enforce this requirement. > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > tests/libqos/virtio.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c > > index 4f7e6bb8a1..2593996c98 100644 > > --- a/tests/libqos/virtio.c > > +++ b/tests/libqos/virtio.c > > @@ -13,23 +13,33 @@ > > #include "standard-headers/linux/virtio_config.h" > > #include "standard-headers/linux/virtio_ring.h" > > > > +/* Features must be negotiated before config space or virtqueue access */ > > +static void check_features_negotiated(QVirtioDevice *d) > > +{ > > + g_assert_cmphex(d->features, !=, 0); > > +} > > Isn't it "legal" to negotiate 0 feature bits, too (for legacy devices)? Yes, it's possible for Legacy devices. If someone ever does that they'll need to extend this code, but it's unlikely so I'd rather not complicate this. Stefan
On 22/10/2019 17.48, Stefan Hajnoczi wrote: > On Mon, Oct 21, 2019 at 02:15:53PM +0200, Thomas Huth wrote: >> On 19/10/2019 08.38, Stefan Hajnoczi wrote: >>> According to VIRTIO 1.1 "3.1.1 Driver Requirements: Device >>> Initialization", configuration space and virtqueues cannot be accessed >>> before features have been negotiated. Enforce this requirement. >>> >>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> >>> --- >>> tests/libqos/virtio.c | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c >>> index 4f7e6bb8a1..2593996c98 100644 >>> --- a/tests/libqos/virtio.c >>> +++ b/tests/libqos/virtio.c >>> @@ -13,23 +13,33 @@ >>> #include "standard-headers/linux/virtio_config.h" >>> #include "standard-headers/linux/virtio_ring.h" >>> >>> +/* Features must be negotiated before config space or virtqueue access */ >>> +static void check_features_negotiated(QVirtioDevice *d) >>> +{ >>> + g_assert_cmphex(d->features, !=, 0); >>> +} >> >> Isn't it "legal" to negotiate 0 feature bits, too (for legacy devices)? > > Yes, it's possible for Legacy devices. If someone ever does that > they'll need to extend this code, but it's unlikely so I'd rather not > complicate this. Could you please add at least a comment here with that explanation? Thanks, Thomas
On Tue, Oct 22, 2019 at 08:48:31PM +0200, Thomas Huth wrote: > On 22/10/2019 17.48, Stefan Hajnoczi wrote: > > On Mon, Oct 21, 2019 at 02:15:53PM +0200, Thomas Huth wrote: > >> On 19/10/2019 08.38, Stefan Hajnoczi wrote: > >>> According to VIRTIO 1.1 "3.1.1 Driver Requirements: Device > >>> Initialization", configuration space and virtqueues cannot be accessed > >>> before features have been negotiated. Enforce this requirement. > >>> > >>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > >>> --- > >>> tests/libqos/virtio.c | 11 +++++++++++ > >>> 1 file changed, 11 insertions(+) > >>> > >>> diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c > >>> index 4f7e6bb8a1..2593996c98 100644 > >>> --- a/tests/libqos/virtio.c > >>> +++ b/tests/libqos/virtio.c > >>> @@ -13,23 +13,33 @@ > >>> #include "standard-headers/linux/virtio_config.h" > >>> #include "standard-headers/linux/virtio_ring.h" > >>> > >>> +/* Features must be negotiated before config space or virtqueue access */ > >>> +static void check_features_negotiated(QVirtioDevice *d) > >>> +{ > >>> + g_assert_cmphex(d->features, !=, 0); > >>> +} > >> > >> Isn't it "legal" to negotiate 0 feature bits, too (for legacy devices)? > > > > Yes, it's possible for Legacy devices. If someone ever does that > > they'll need to extend this code, but it's unlikely so I'd rather not > > complicate this. > > Could you please add at least a comment here with that explanation? I'll try adding a bool field in v4 so there are no problems with a 0 feature bit set. Stefan
diff --git a/tests/libqos/virtio.c b/tests/libqos/virtio.c index 4f7e6bb8a1..2593996c98 100644 --- a/tests/libqos/virtio.c +++ b/tests/libqos/virtio.c @@ -13,23 +13,33 @@ #include "standard-headers/linux/virtio_config.h" #include "standard-headers/linux/virtio_ring.h" +/* Features must be negotiated before config space or virtqueue access */ +static void check_features_negotiated(QVirtioDevice *d) +{ + g_assert_cmphex(d->features, !=, 0); +} + uint8_t qvirtio_config_readb(QVirtioDevice *d, uint64_t addr) { + check_features_negotiated(d); return d->bus->config_readb(d, addr); } uint16_t qvirtio_config_readw(QVirtioDevice *d, uint64_t addr) { + check_features_negotiated(d); return d->bus->config_readw(d, addr); } uint32_t qvirtio_config_readl(QVirtioDevice *d, uint64_t addr) { + check_features_negotiated(d); return d->bus->config_readl(d, addr); } uint64_t qvirtio_config_readq(QVirtioDevice *d, uint64_t addr) { + check_features_negotiated(d); return d->bus->config_readq(d, addr); } @@ -47,6 +57,7 @@ void qvirtio_set_features(QVirtioDevice *d, uint64_t features) QVirtQueue *qvirtqueue_setup(QVirtioDevice *d, QGuestAllocator *alloc, uint16_t index) { + check_features_negotiated(d); return d->bus->virtqueue_setup(d, alloc, index); }
According to VIRTIO 1.1 "3.1.1 Driver Requirements: Device Initialization", configuration space and virtqueues cannot be accessed before features have been negotiated. Enforce this requirement. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- tests/libqos/virtio.c | 11 +++++++++++ 1 file changed, 11 insertions(+)