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@xxxxxxxxxx> > > --- > > 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
