[Qemu-devel] [PATCH v4] virtio-pci: error out when both legacy and modern modes are disabled

[Qemu-devel] [PATCH v4] virtio-pci: error out when both legacy and modern modes are disabled

[Greg Kurz]

From: Greg Kurz <gkurz@linux.vnet.ibm.com>

Without presuming if we got there because of a user mistake or some
more subtle bug in the tooling, it really does not make sense to
implement a non-functional device.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
v4: - rephrased error message and provide a hint to the user
    - split string literals to stay below 80 characters
    - added Marcel's R-b tag
---
 hw/virtio/virtio-pci.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 755f9218b77d..72c4b392ffda 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1842,6 +1842,14 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
     VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
     PCIDevice *pci_dev = &proxy->pci_dev;
 
+    if (!(virtio_pci_modern(proxy) || virtio_pci_legacy(proxy))) {
+        error_setg(errp, "device cannot work when both modern and legacy modes"
+                   " are disabled");
+        error_append_hint(errp, "Set either disable-modern or disable-legacy"
+                          " to off\n");
+        return;
+    }
+
     if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) &&
         virtio_pci_modern(proxy)) {
         pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;

Re: [Qemu-devel] [PATCH v4] virtio-pci: error out when both legacy and modern modes are disabled

[Greg Kurz]

On Thu, 21 Jul 2016 23:21:16 +0200
Greg Kurz <groug@kaod.org> wrote:

> From: Greg Kurz <gkurz@linux.vnet.ibm.com>
> 
> Without presuming if we got there because of a user mistake or some
> more subtle bug in the tooling, it really does not make sense to
> implement a non-functional device.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> v4: - rephrased error message and provide a hint to the user
>     - split string literals to stay below 80 characters
>     - added Marcel's R-b tag
> ---

Marcel,

I see that Michael has comments on your patch. If you feel this patch is valuable
for 2.7, please consider carrying and pushing it, as I'm about to take a 1-month
leave.

Thanks.

--
Greg

>  hw/virtio/virtio-pci.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 755f9218b77d..72c4b392ffda 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1842,6 +1842,14 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
>      VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
>      PCIDevice *pci_dev = &proxy->pci_dev;
>  
> +    if (!(virtio_pci_modern(proxy) || virtio_pci_legacy(proxy))) {
> +        error_setg(errp, "device cannot work when both modern and legacy modes"
> +                   " are disabled");
> +        error_append_hint(errp, "Set either disable-modern or disable-legacy"
> +                          " to off\n");
> +        return;
> +    }
> +
>      if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) &&
>          virtio_pci_modern(proxy)) {
>          pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> 
> 

Re: [Qemu-devel] [PATCH v4] virtio-pci: error out when both legacy and modern modes are disabled

[Marcel Apfelbaum]

On 07/22/2016 12:55 AM, Greg Kurz wrote:
> On Thu, 21 Jul 2016 23:21:16 +0200
> Greg Kurz <groug@kaod.org> wrote:
>
>> From: Greg Kurz <gkurz@linux.vnet.ibm.com>
>>
>> Without presuming if we got there because of a user mistake or some
>> more subtle bug in the tooling, it really does not make sense to
>> implement a non-functional device.
>>
>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
>> Signed-off-by: Greg Kurz <groug@kaod.org>
>> ---
>> v4: - rephrased error message and provide a hint to the user
>>      - split string literals to stay below 80 characters
>>      - added Marcel's R-b tag
>> ---
>
> Marcel,
>
> I see that Michael has comments on your patch. If you feel this patch is valuable
> for 2.7, please consider carrying and pushing it, as I'm about to take a 1-month
> leave.
>

I'll be sure to take it form here, thanks for the help!
Marcel

> Thanks.
>
> --
> Greg
>
>>   hw/virtio/virtio-pci.c |    8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 755f9218b77d..72c4b392ffda 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -1842,6 +1842,14 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
>>       VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
>>       PCIDevice *pci_dev = &proxy->pci_dev;
>>
>> +    if (!(virtio_pci_modern(proxy) || virtio_pci_legacy(proxy))) {
>> +        error_setg(errp, "device cannot work when both modern and legacy modes"
>> +                   " are disabled");
>> +        error_append_hint(errp, "Set either disable-modern or disable-legacy"
>> +                          " to off\n");
>> +        return;
>> +    }
>> +
>>       if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) &&
>>           virtio_pci_modern(proxy)) {
>>           pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
>>
>>
>

Re: [Qemu-devel] [PATCH v4] virtio-pci: error out when both legacy and modern modes are disabled

[Cornelia Huck]

On Thu, 21 Jul 2016 23:21:16 +0200
Greg Kurz <groug@kaod.org> wrote:

> From: Greg Kurz <gkurz@linux.vnet.ibm.com>
> 
> Without presuming if we got there because of a user mistake or some
> more subtle bug in the tooling, it really does not make sense to
> implement a non-functional device.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> v4: - rephrased error message and provide a hint to the user
>     - split string literals to stay below 80 characters
>     - added Marcel's R-b tag
> ---
>  hw/virtio/virtio-pci.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 755f9218b77d..72c4b392ffda 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1842,6 +1842,14 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
>      VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
>      PCIDevice *pci_dev = &proxy->pci_dev;
> 
> +    if (!(virtio_pci_modern(proxy) || virtio_pci_legacy(proxy))) {

I'm not sure that I didn't mess up the sequence of the realize
callbacks, but could disable_legacy still be AUTO here? In that case,
we'd fail for disable-modern=on and disable-legacy unset (i.e., AUTO),
which would be ok for pcie but not for !pcie.

> +        error_setg(errp, "device cannot work when both modern and legacy modes"
> +                   " are disabled");
> +        error_append_hint(errp, "Set either disable-modern or disable-legacy"
> +                          " to off\n");
> +        return;
> +    }
> +
>      if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) &&
>          virtio_pci_modern(proxy)) {
>          pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> 

Re: [Qemu-devel] [PATCH v4] virtio-pci: error out when both legacy and modern modes are disabled

[Greg Kurz]

On Fri, 22 Jul 2016 10:04:35 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Thu, 21 Jul 2016 23:21:16 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > From: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > 
> > Without presuming if we got there because of a user mistake or some
> > more subtle bug in the tooling, it really does not make sense to
> > implement a non-functional device.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > v4: - rephrased error message and provide a hint to the user
> >     - split string literals to stay below 80 characters
> >     - added Marcel's R-b tag
> > ---
> >  hw/virtio/virtio-pci.c |    8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index 755f9218b77d..72c4b392ffda 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -1842,6 +1842,14 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
> >      VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
> >      PCIDevice *pci_dev = &proxy->pci_dev;
> > 
> > +    if (!(virtio_pci_modern(proxy) || virtio_pci_legacy(proxy))) {  
> 
> I'm not sure that I didn't mess up the sequence of the realize
> callbacks, but could disable_legacy still be AUTO here? In that case,
> we'd fail for disable-modern=on and disable-legacy unset (i.e., AUTO),
> which would be ok for pcie but not for !pcie.
> 

Marcel made the same comment in:

https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg05225.html

If the user explicitly disables modern, she shouldn't rely on QEMU
implicitly enabling legacy, hence the suggestion in error_append_hint().

> > +        error_setg(errp, "device cannot work when both modern and legacy modes"
> > +                   " are disabled");
> > +        error_append_hint(errp, "Set either disable-modern or disable-legacy"
> > +                          " to off\n");
> > +        return;
> > +    }
> > +
> >      if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) &&
> >          virtio_pci_modern(proxy)) {
> >          pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> >   
> 

Re: [Qemu-devel] [PATCH v4] virtio-pci: error out when both legacy and modern modes are disabled

[Cornelia Huck]

On Fri, 22 Jul 2016 12:11:11 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Fri, 22 Jul 2016 10:04:35 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > On Thu, 21 Jul 2016 23:21:16 +0200
> > Greg Kurz <groug@kaod.org> wrote:
> > 
> > > From: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > 
> > > Without presuming if we got there because of a user mistake or some
> > > more subtle bug in the tooling, it really does not make sense to
> > > implement a non-functional device.
> > > 
> > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > > v4: - rephrased error message and provide a hint to the user
> > >     - split string literals to stay below 80 characters
> > >     - added Marcel's R-b tag
> > > ---
> > >  hw/virtio/virtio-pci.c |    8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > index 755f9218b77d..72c4b392ffda 100644
> > > --- a/hw/virtio/virtio-pci.c
> > > +++ b/hw/virtio/virtio-pci.c
> > > @@ -1842,6 +1842,14 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
> > >      VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
> > >      PCIDevice *pci_dev = &proxy->pci_dev;
> > > 
> > > +    if (!(virtio_pci_modern(proxy) || virtio_pci_legacy(proxy))) {  
> > 
> > I'm not sure that I didn't mess up the sequence of the realize
> > callbacks, but could disable_legacy still be AUTO here? In that case,
> > we'd fail for disable-modern=on and disable-legacy unset (i.e., AUTO),
> > which would be ok for pcie but not for !pcie.
> > 
> 
> Marcel made the same comment in:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg05225.html
> 
> If the user explicitly disables modern, she shouldn't rely on QEMU
> implicitly enabling legacy, hence the suggestion in error_append_hint().

I don't know, I'd find that a bit surprising, especially as I would end
up with a legacy-capable device if I did not specify anything in
the !pcie case.

> 
> > > +        error_setg(errp, "device cannot work when both modern and legacy modes"
> > > +                   " are disabled");

Suggest to change this wording to:

"device cannot work as neither modern nor legacy mode is enabled"

as this more accurately reflects what happened (the user did not
actively disable legacy in the case above).

> > > +        error_append_hint(errp, "Set either disable-modern or disable-legacy"
> > > +                          " to off\n");

The hint looks fine to me :)

> > > +        return;
> > > +    }
> > > +
> > >      if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) &&
> > >          virtio_pci_modern(proxy)) {
> > >          pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> > >   
> > 
> 

Re: [Qemu-devel] [PATCH v4] virtio-pci: error out when both legacy and modern modes are disabled

[Greg Kurz]

On Fri, 22 Jul 2016 12:32:24 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Fri, 22 Jul 2016 12:11:11 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > On Fri, 22 Jul 2016 10:04:35 +0200
> > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> >   
> > > On Thu, 21 Jul 2016 23:21:16 +0200
> > > Greg Kurz <groug@kaod.org> wrote:
> > >   
> > > > From: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > > 
> > > > Without presuming if we got there because of a user mistake or some
> > > > more subtle bug in the tooling, it really does not make sense to
> > > > implement a non-functional device.
> > > > 
> > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > ---
> > > > v4: - rephrased error message and provide a hint to the user
> > > >     - split string literals to stay below 80 characters
> > > >     - added Marcel's R-b tag
> > > > ---
> > > >  hw/virtio/virtio-pci.c |    8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > > index 755f9218b77d..72c4b392ffda 100644
> > > > --- a/hw/virtio/virtio-pci.c
> > > > +++ b/hw/virtio/virtio-pci.c
> > > > @@ -1842,6 +1842,14 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
> > > >      VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
> > > >      PCIDevice *pci_dev = &proxy->pci_dev;
> > > > 
> > > > +    if (!(virtio_pci_modern(proxy) || virtio_pci_legacy(proxy))) {    
> > > 
> > > I'm not sure that I didn't mess up the sequence of the realize
> > > callbacks, but could disable_legacy still be AUTO here? In that case,
> > > we'd fail for disable-modern=on and disable-legacy unset (i.e., AUTO),
> > > which would be ok for pcie but not for !pcie.
> > >   
> > 
> > Marcel made the same comment in:
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg05225.html
> > 
> > If the user explicitly disables modern, she shouldn't rely on QEMU
> > implicitly enabling legacy, hence the suggestion in error_append_hint().  
> 
> I don't know, I'd find that a bit surprising, especially as I would end
> up with a legacy-capable device if I did not specify anything in
> the !pcie case.
> 

Isn't it already what happens with legacy being the default in pre-2.7 QEMU ?

Do you think we should have separate checks for pcie and !pcie ?

> >   
> > > > +        error_setg(errp, "device cannot work when both modern and legacy modes"
> > > > +                   " are disabled");  
> 
> Suggest to change this wording to:
> 
> "device cannot work as neither modern nor legacy mode is enabled"
> 
> as this more accurately reflects what happened (the user did not
> actively disable legacy in the case above).
> 

Thanks ! This is THE wording I was looking for :)

> > > > +        error_append_hint(errp, "Set either disable-modern or disable-legacy"
> > > > +                          " to off\n");  
> 
> The hint looks fine to me :)
> 

It was the easy part :)

> > > > +        return;
> > > > +    }
> > > > +
> > > >      if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) &&
> > > >          virtio_pci_modern(proxy)) {
> > > >          pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> > > >     
> > >   
> >   
> 
> 

Re: [Qemu-devel] [PATCH v4] virtio-pci: error out when both legacy and modern modes are disabled

[Cornelia Huck]

On Fri, 22 Jul 2016 15:23:19 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Fri, 22 Jul 2016 12:32:24 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > On Fri, 22 Jul 2016 12:11:11 +0200
> > Greg Kurz <groug@kaod.org> wrote:
> > 
> > > On Fri, 22 Jul 2016 10:04:35 +0200
> > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > >   
> > > > On Thu, 21 Jul 2016 23:21:16 +0200
> > > > Greg Kurz <groug@kaod.org> wrote:
> > > >   
> > > > > From: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > > > 
> > > > > Without presuming if we got there because of a user mistake or some
> > > > > more subtle bug in the tooling, it really does not make sense to
> > > > > implement a non-functional device.
> > > > > 
> > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > > > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > > ---
> > > > > v4: - rephrased error message and provide a hint to the user
> > > > >     - split string literals to stay below 80 characters
> > > > >     - added Marcel's R-b tag
> > > > > ---
> > > > >  hw/virtio/virtio-pci.c |    8 ++++++++
> > > > >  1 file changed, 8 insertions(+)
> > > > > 
> > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > > > index 755f9218b77d..72c4b392ffda 100644
> > > > > --- a/hw/virtio/virtio-pci.c
> > > > > +++ b/hw/virtio/virtio-pci.c
> > > > > @@ -1842,6 +1842,14 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
> > > > >      VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
> > > > >      PCIDevice *pci_dev = &proxy->pci_dev;
> > > > > 
> > > > > +    if (!(virtio_pci_modern(proxy) || virtio_pci_legacy(proxy))) {    
> > > > 
> > > > I'm not sure that I didn't mess up the sequence of the realize
> > > > callbacks, but could disable_legacy still be AUTO here? In that case,
> > > > we'd fail for disable-modern=on and disable-legacy unset (i.e., AUTO),
> > > > which would be ok for pcie but not for !pcie.
> > > >   
> > > 
> > > Marcel made the same comment in:
> > > 
> > > https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg05225.html
> > > 
> > > If the user explicitly disables modern, she shouldn't rely on QEMU
> > > implicitly enabling legacy, hence the suggestion in error_append_hint().  
> > 
> > I don't know, I'd find that a bit surprising, especially as I would end
> > up with a legacy-capable device if I did not specify anything in
> > the !pcie case.
> > 
> 
> Isn't it already what happens with legacy being the default in pre-2.7 QEMU ?

Well, that is exactly my point; users may be surprised.

> 
> Do you think we should have separate checks for pcie and !pcie ?

I don't think we should overengineer this.

> 
> > >   
> > > > > +        error_setg(errp, "device cannot work when both modern and legacy modes"
> > > > > +                   " are disabled");  
> > 
> > Suggest to change this wording to:
> > 
> > "device cannot work as neither modern nor legacy mode is enabled"
> > 
> > as this more accurately reflects what happened (the user did not
> > actively disable legacy in the case above).
> > 
> 
> Thanks ! This is THE wording I was looking for :)

:)

I'm fine with the patch with the changed wording, as it less confusing
for the user.

<I'll be offline next week so please just attach my

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

if the only thing you change is the message>

Re: [Qemu-devel] [PATCH v4] virtio-pci: error out when both legacy and modern modes are disabled

[Greg Kurz]

On Fri, 22 Jul 2016 15:42:48 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> [...]
> > > > > On Thu, 21 Jul 2016 23:21:16 +0200
> > > > > Greg Kurz <groug@kaod.org> wrote:
> > > > >     
> > > > > > From: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > > > > 
> > > > > > Without presuming if we got there because of a user mistake or some
> > > > > > more subtle bug in the tooling, it really does not make sense to
> > > > > > implement a non-functional device.
> > > > > > 
> > > > > > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > > > > > Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
> > > > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > > > ---
> > > > > > v4: - rephrased error message and provide a hint to the user
> > > > > >     - split string literals to stay below 80 characters
> > > > > >     - added Marcel's R-b tag
> > > > > > ---
> > > > > >  hw/virtio/virtio-pci.c |    8 ++++++++
> > > > > >  1 file changed, 8 insertions(+)
> > > > > > 
> > > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > > > > index 755f9218b77d..72c4b392ffda 100644
> > > > > > --- a/hw/virtio/virtio-pci.c
> > > > > > +++ b/hw/virtio/virtio-pci.c
> > > > > > @@ -1842,6 +1842,14 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
> > > > > >      VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
> > > > > >      PCIDevice *pci_dev = &proxy->pci_dev;
> > > > > > 
> > > > > > +    if (!(virtio_pci_modern(proxy) || virtio_pci_legacy(proxy))) {      
> > > > > 
> > > > > I'm not sure that I didn't mess up the sequence of the realize
> > > > > callbacks, but could disable_legacy still be AUTO here? In that case,
> > > > > we'd fail for disable-modern=on and disable-legacy unset (i.e., AUTO),
> > > > > which would be ok for pcie but not for !pcie.
> > > > >     
> > > > 
> > > > Marcel made the same comment in:
> > > > 
> > > > https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg05225.html
> > > > 
> > > > If the user explicitly disables modern, she shouldn't rely on QEMU
> > > > implicitly enabling legacy, hence the suggestion in error_append_hint().    
> > > 
> > > I don't know, I'd find that a bit surprising, especially as I would end
> > > up with a legacy-capable device if I did not specify anything in
> > > the !pcie case.
> > >   
> > 
> > Isn't it already what happens with legacy being the default in pre-2.7 QEMU ?  
> 
> Well, that is exactly my point; users may be surprised.
> 

One day legacy will be hopefully buried :)

> > 
> > Do you think we should have separate checks for pcie and !pcie ?  
> 
> I don't think we should overengineer this.
> 

Agreed.

> >   
> > > >     
> > > > > > +        error_setg(errp, "device cannot work when both modern and legacy modes"
> > > > > > +                   " are disabled");    
> > > 
> > > Suggest to change this wording to:
> > > 
> > > "device cannot work as neither modern nor legacy mode is enabled"
> > > 
> > > as this more accurately reflects what happened (the user did not
> > > actively disable legacy in the case above).
> > >   
> > 
> > Thanks ! This is THE wording I was looking for :)  
> 
> :)
> 
> I'm fine with the patch with the changed wording, as it less confusing
> for the user.
> 
> <I'll be offline next week so please just attach my
> 
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> if the only thing you change is the message>
> 

I'll do this right away as I'll be offline for 1 month starting... just after
I post v5 :)

Cheers.

--
Greg