[v3] libqos: add VIRTIO PCI 1.0 support

[PATCH v3 07/16] libqos: enforce Device Initialization order

Posted by Stefan Hajnoczi 6 years ago

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);
+}
+
 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);
 }
 
-- 
2.21.0

Re: [PATCH v3 07/16] libqos: enforce Device Initialization order

Posted by Thomas Huth 6 years ago

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

Re: [PATCH v3 07/16] libqos: enforce Device Initialization order

Posted by Stefan Hajnoczi 6 years ago

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

Re: [PATCH v3 07/16] libqos: enforce Device Initialization order

Posted by Thomas Huth 6 years ago

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

Re: [PATCH v3 07/16] libqos: enforce Device Initialization order

Posted by Stefan Hajnoczi 6 years ago

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