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

[Qemu-devel] [PATCH v2] 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>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
v2: - error out at realize time as suggested by Connie
    - updated title and changelog
---
 hw/virtio/virtio-pci.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index f0677b73d860..8d707aac0c21 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1838,6 +1838,12 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
     VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
     PCIDevice *pci_dev = &proxy->pci_dev;
 
+    if (proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY &&
+        proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) {
+        error_setg(errp, "device is unserviceable when both legacy and modern modes are disabled. At least one of the disable-modern or disable-legacy properties should be set to false.");
+        return;
+    }
+
     if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) &&
         !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN)) {
         pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;

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

[Marcel Apfelbaum]

On 07/21/2016 06:52 PM, Greg Kurz 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>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> v2: - error out at realize time as suggested by Connie
>      - updated title and changelog
> ---
>   hw/virtio/virtio-pci.c |    6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f0677b73d860..8d707aac0c21 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1838,6 +1838,12 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
>       VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
>       PCIDevice *pci_dev = &proxy->pci_dev;
>
> +    if (proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY &&
> +        proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) {
> +        error_setg(errp, "device is unserviceable when both legacy and modern modes are disabled. At least one of the disable-modern or disable-legacy properties should be set to false.");
> +        return;
> +    }
> +

Hi Greg,

I agree with the patch, but other patch in the mailing list
replaces the flags with standalone fields:

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

Also the error line may be too long and need to be truncated.

Thanks,
Marcel

>       if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) &&
>           !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN)) {
>           pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
>
>

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

[Greg Kurz]

On Thu, 21 Jul 2016 19:52:28 +0300
Marcel Apfelbaum <marcel.apfelbaum@gmail.com> wrote:

> On 07/21/2016 06:52 PM, Greg Kurz 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>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > v2: - error out at realize time as suggested by Connie
> >      - updated title and changelog
> > ---
> >   hw/virtio/virtio-pci.c |    6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index f0677b73d860..8d707aac0c21 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -1838,6 +1838,12 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
> >       VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
> >       PCIDevice *pci_dev = &proxy->pci_dev;
> >
> > +    if (proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY &&
> > +        proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) {
> > +        error_setg(errp, "device is unserviceable when both legacy and modern modes are disabled. At least one of the disable-modern or disable-legacy properties should be set to false.");
> > +        return;
> > +    }
> > +  
> 
> Hi Greg,
> 
> I agree with the patch, but other patch in the mailing list
> replaces the flags with standalone fields:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg04744.html
> 

v3 will be using virtio_pci_legacy() and virtio_pci_modern().

> Also the error line may be too long and need to be truncated.
> 

I'll rephrase the message. BTW is there a formal limit for the length
of error lines ? 80 chars ?

> Thanks,
> Marcel
> 
> >       if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) &&
> >           !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN)) {
> >           pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> >
> >  
> 

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

[Greg Kurz]

On Thu, 21 Jul 2016 19:02:39 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Thu, 21 Jul 2016 19:52:28 +0300
> Marcel Apfelbaum <marcel.apfelbaum@gmail.com> wrote:
> 
> > On 07/21/2016 06:52 PM, Greg Kurz 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>
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > > v2: - error out at realize time as suggested by Connie
> > >      - updated title and changelog
> > > ---
> > >   hw/virtio/virtio-pci.c |    6 ++++++
> > >   1 file changed, 6 insertions(+)
> > >
> > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > index f0677b73d860..8d707aac0c21 100644
> > > --- a/hw/virtio/virtio-pci.c
> > > +++ b/hw/virtio/virtio-pci.c
> > > @@ -1838,6 +1838,12 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
> > >       VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
> > >       PCIDevice *pci_dev = &proxy->pci_dev;
> > >
> > > +    if (proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY &&
> > > +        proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) {
> > > +        error_setg(errp, "device is unserviceable when both legacy and modern modes are disabled. At least one of the disable-modern or disable-legacy properties should be set to false.");
> > > +        return;
> > > +    }
> > > +  
> > 
> > Hi Greg,
> > 
> > I agree with the patch, but other patch in the mailing list
> > replaces the flags with standalone fields:
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg04744.html
> > 
> 
> v3 will be using virtio_pci_legacy() and virtio_pci_modern().
> 
> > Also the error line may be too long and need to be truncated.
> > 
> 
> I'll rephrase the message. BTW is there a formal limit for the length
> of error lines ? 80 chars ?
> 

Not even sure it makes sense since the final error message contains the
full -device command line option which can be of arbitrary length...

> > Thanks,
> > Marcel
> > 
> > >       if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_PCIE) &&
> > >           !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN)) {
> > >           pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> > >
> > >  
> > 
> 

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1531 bytes --]

On 07/21/2016 09:52 AM, Greg Kurz 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>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> v2: - error out at realize time as suggested by Connie
>     - updated title and changelog
> ---
>  hw/virtio/virtio-pci.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index f0677b73d860..8d707aac0c21 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1838,6 +1838,12 @@ static void virtio_pci_dc_realize(DeviceState *qdev, Error **errp)
>      VirtIOPCIProxy *proxy = VIRTIO_PCI(qdev);
>      PCIDevice *pci_dev = &proxy->pci_dev;
>  
> +    if (proxy->flags & VIRTIO_PCI_FLAG_DISABLE_LEGACY &&
> +        proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN) {
> +        error_setg(errp, "device is unserviceable when both legacy and modern modes are disabled. At least one of the disable-modern or disable-legacy properties should be set to false.");

Too long. error_setg() should be a single phrase, with no trailing '.'.
 If you need to add additional information, error_append_hint() is how
you add the second sentence.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH v2] 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>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
v2: - moved the check to virtio_pci_realize() to ensure disable_legacy is
      either ON or OFF, but not AUTO (this would cause the check to fail
      before we have a chance to know if the device can actually work).

I've kept the R-b tags, please raise a flag if this is inapropriate.

I've dropped the other patch about virtio-9p: the issue will be addressed
in a separate series.
---
 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 268fd8ebb219..551c4346bf4b 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1770,6 +1770,14 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
         proxy->disable_legacy = pcie_port ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
     }
 
+    if (!(virtio_pci_modern(proxy) || virtio_pci_legacy(proxy))) {
+        error_setg(errp, "device cannot work as neither modern nor legacy mode"
+                   " is enabled");
+        error_append_hint(errp, "Set either disable-modern or disable-legacy"
+                          " to off\n");
+        return;
+    }
+
     if (pcie_port && pci_is_express(pci_dev)) {
         int pos;