[libvirt PATCH] qemu: ramfb video device doesn't support PCI address

Jonathon Jongsma posted 1 patch 3 years, 9 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200624220612.11422-1-jjongsma@redhat.com
There is a newer version of this series
src/conf/domain_conf.c   | 7 +++++++
tests/qemuxml2argvtest.c | 1 +
2 files changed, 8 insertions(+)
[libvirt PATCH] qemu: ramfb video device doesn't support PCI address
Posted by Jonathon Jongsma 3 years, 9 months ago
Although a ramfb video device is not a PCI device, we don't currently
report an error for ramfb device definitions containing a PCI address.
However, a guest configured with such a device will fail to start:

    # virsh start test1
    error: Failed to start domain test1
    error: internal error: qemu unexpectedly closed the monitor: 2020-06-16T05:23:02.759221Z qemu-kvm: -device ramfb,id=video0,bus=pcie.0,addr=0x1: Device 'ramfb' can't go on PCIE bus

A better approach is to reject any device definitions that contain PCI
addresses.  While this is a change in behavior, any existing
configurations were non-functional.

https://bugzilla.redhat.com/show_bug.cgi?id=1847259

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
---
 src/conf/domain_conf.c   | 7 +++++++
 tests/qemuxml2argvtest.c | 1 +
 2 files changed, 8 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index fc7fcfb0c6..1a06cb3f4b 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6608,6 +6608,13 @@ virDomainVideoDefValidate(const virDomainVideoDef *video,
         return -1;
     }
 
+    if (video->type == VIR_DOMAIN_VIDEO_TYPE_RAMFB &&
+        video->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("'address' is not supported for 'ramfb' video devices"));
+        return -1;
+    }
+
     /* it doesn't make sense to pair video device type 'none' with any other
      * types, there can be only a single video device in such case
      */
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 1195f9c982..f2522fa530 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2276,6 +2276,7 @@ mymain(void)
             QEMU_CAPS_VIRTIO_GPU_MAX_OUTPUTS);
     DO_TEST_CAPS_LATEST("video-bochs-display-device");
     DO_TEST_CAPS_LATEST("video-ramfb-display-device");
+    DO_TEST_CAPS_LATEST_PARSE_ERROR("video-ramfb-display-device-pci-address");
     DO_TEST("video-none-device",
             QEMU_CAPS_VNC);
     DO_TEST_PARSE_ERROR("video-invalid-multiple-devices", NONE);
-- 
2.21.3

Re: [libvirt PATCH] qemu: ramfb video device doesn't support PCI address
Posted by Daniel Henrique Barboza 3 years, 9 months ago

On 6/24/20 7:06 PM, Jonathon Jongsma wrote:
> Although a ramfb video device is not a PCI device, we don't currently
> report an error for ramfb device definitions containing a PCI address.
> However, a guest configured with such a device will fail to start:
> 
>      # virsh start test1
>      error: Failed to start domain test1
>      error: internal error: qemu unexpectedly closed the monitor: 2020-06-16T05:23:02.759221Z qemu-kvm: -device ramfb,id=video0,bus=pcie.0,addr=0x1: Device 'ramfb' can't go on PCIE bus
> 
> A better approach is to reject any device definitions that contain PCI
> addresses.  While this is a change in behavior, any existing
> configurations were non-functional.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1847259
> 
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>


Re: [libvirt PATCH] qemu: ramfb video device doesn't support PCI address
Posted by Laine Stump 3 years, 9 months ago
On 6/24/20 6:06 PM, Jonathon Jongsma wrote:
> Although a ramfb video device is not a PCI device, we don't currently
> report an error for ramfb device definitions containing a PCI address.
> However, a guest configured with such a device will fail to start:
>
>      # virsh start test1
>      error: Failed to start domain test1
>      error: internal error: qemu unexpectedly closed the monitor: 2020-06-16T05:23:02.759221Z qemu-kvm: -device ramfb,id=video0,bus=pcie.0,addr=0x1: Device 'ramfb' can't go on PCIE bus
>
> A better approach is to reject any device definitions that contain PCI
> addresses.  While this is a change in behavior, any existing
> configurations were non-functional.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1847259
>
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
>   src/conf/domain_conf.c   | 7 +++++++
>   tests/qemuxml2argvtest.c | 1 +
>   2 files changed, 8 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index fc7fcfb0c6..1a06cb3f4b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6608,6 +6608,13 @@ virDomainVideoDefValidate(const virDomainVideoDef *video,
>           return -1;
>       }
>   
> +    if (video->type == VIR_DOMAIN_VIDEO_TYPE_RAMFB &&
> +        video->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("'address' is not supported for 'ramfb' video devices"));
> +        return -1;
> +    }
> +


There may be disagreement about this, and in practical terms it makes no 
difference (because no domain type other than QEMU is ever going to have 
one of these devices anyway), but since ramfb is a qemu-specific device 
(isn't it?), I think this check would be better put in 
qemu_validate.c:qemuValidateDomainDeviceDevVideo(), which is the 
qemu-specific validation function for video devices.


(I  see there is already validation in virDomainVideoDefValidate() for a 
qemu-specific (afaik) video type - i is even checking for one backend 
that is named VIR_DOMAIN_VIDEO_BACKEND_TYPE_QEMU)


I don't really have a strong opinion though, since the other hypervisor 
drivers don't seem all that concerned about fleshing out their 
validation functions, and what you have works properly for qemu :-P


>       /* it doesn't make sense to pair video device type 'none' with any other
>        * types, there can be only a single video device in such case
>        */
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 1195f9c982..f2522fa530 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -2276,6 +2276,7 @@ mymain(void)
>               QEMU_CAPS_VIRTIO_GPU_MAX_OUTPUTS);
>       DO_TEST_CAPS_LATEST("video-bochs-display-device");
>       DO_TEST_CAPS_LATEST("video-ramfb-display-device");
> +    DO_TEST_CAPS_LATEST_PARSE_ERROR("video-ramfb-display-device-pci-address");
>       DO_TEST("video-none-device",
>               QEMU_CAPS_VNC);
>       DO_TEST_PARSE_ERROR("video-invalid-multiple-devices", NONE);


Re: [libvirt PATCH] qemu: ramfb video device doesn't support PCI address
Posted by Jonathon Jongsma 3 years, 9 months ago
On Wed, 2020-06-24 at 23:21 -0400, Laine Stump wrote:
> On 6/24/20 6:06 PM, Jonathon Jongsma wrote:
> > Although a ramfb video device is not a PCI device, we don't
> > currently
> > report an error for ramfb device definitions containing a PCI
> > address.
> > However, a guest configured with such a device will fail to start:
> > 
> >      # virsh start test1
> >      error: Failed to start domain test1
> >      error: internal error: qemu unexpectedly closed the monitor:
> > 2020-06-16T05:23:02.759221Z qemu-kvm: -device
> > ramfb,id=video0,bus=pcie.0,addr=0x1: Device 'ramfb' can't go on
> > PCIE bus
> > 
> > A better approach is to reject any device definitions that contain
> > PCI
> > addresses.  While this is a change in behavior, any existing
> > configurations were non-functional.
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1847259
> > 
> > Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> > ---
> >   src/conf/domain_conf.c   | 7 +++++++
> >   tests/qemuxml2argvtest.c | 1 +
> >   2 files changed, 8 insertions(+)
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index fc7fcfb0c6..1a06cb3f4b 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -6608,6 +6608,13 @@ virDomainVideoDefValidate(const
> > virDomainVideoDef *video,
> >           return -1;
> >       }
> >   
> > +    if (video->type == VIR_DOMAIN_VIDEO_TYPE_RAMFB &&
> > +        video->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                       _("'address' is not supported for 'ramfb'
> > video devices"));
> > +        return -1;
> > +    }
> > +
> 
> There may be disagreement about this, and in practical terms it makes
> no 
> difference (because no domain type other than QEMU is ever going to
> have 
> one of these devices anyway), but since ramfb is a qemu-specific
> device 
> (isn't it?), I think this check would be better put in 
> qemu_validate.c:qemuValidateDomainDeviceDevVideo(), which is the 
> qemu-specific validation function for video devices.
> 
> 
> (I  see there is already validation in virDomainVideoDefValidate()
> for a 
> qemu-specific (afaik) video type - i is even checking for one
> backend 
> that is named VIR_DOMAIN_VIDEO_BACKEND_TYPE_QEMU)
> 
> 
> I don't really have a strong opinion though, since the other
> hypervisor 
> drivers don't seem all that concerned about fleshing out their 
> validation functions, and what you have works properly for qemu :-P

For what it's worth, I agree with you. I'll re-spin the patch.

Jonathon