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

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(a)redhat.com&gt; > --- > 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