| Message ID | 28817417f19a1301bcc38ccb0ff893395b320137.1547746867.git.crobinso@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu: virtio-{non-}transitional support | expand |
On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: [...] > VIR_ENUM_IMPL(virDomainRNGModel, > VIR_DOMAIN_RNG_MODEL_LAST, > - "virtio"); > + "virtio", > + "virtio-transitional", > + "virtio-non-transitional"); Since you're touching the definition, you can make it VIR_ENUM_IMPL(virDomainRNGModel, ... "virtio-non-transitional", ); so that if and when we'll need to add another model the diff will look nicer. [...] > @@ -1122,6 +1124,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { > {"virtio-net-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_NET_NON_TRANSITIONAL}, > {"vhost-scsi-pci-transitional", QEMU_CAPS_DEVICE_VHOST_SCSI_TRANSITIONAL}, > {"vhost-scsi-pci-non-transitional", QEMU_CAPS_DEVICE_VHOST_SCSI_NON_TRANSITIONAL}, > + {"virtio-rng-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_RNG_TRANSITIONAL}, > + {"virtio-rng-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_RNG_NON_TRANSITIONAL}, > }; Same comments as for the other capabilities. [...] > @@ -5990,7 +5995,7 @@ qemuBuildRNGDevStr(const virDomainDef *def, > { > virBuffer buf = VIR_BUFFER_INITIALIZER; > > - if (dev->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || > + if (dev->model == VIR_DOMAIN_RNG_MODEL_VIRTIO && > !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_RNG)) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("this qemu doesn't support RNG device type '%s'"), Idea for a possible follow-up series: wouldn't it be great if we centralized capabilities checking for VirtIO devices in a function that can be called during device validation? Delaying such checks until command line generation is not optimal, and neither is having some of them centralized and some of them sprinkled around the various qemuBuild*DevStr() functions... [...] > @@ -2290,8 +2295,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, > > /* VirtIO RNG */ > for (i = 0; i < def->nrngs; i++) { > - if (def->rngs[i]->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || > - !virDeviceInfoPCIAddressIsWanted(&def->rngs[i]->info)) > + if (!virDeviceInfoPCIAddressIsWanted(&def->rngs[i]->info)) > continue; I'm not convinced you can just remove the model check here... Shouldn't you extend it to include MODEL_VIRTIO_TRANSITIONAL and MODEL_VIRTIO_NON_TRANSITIONAL instead? -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/21/2019 08:13 AM, Andrea Bolognani wrote: > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: > [...] >> VIR_ENUM_IMPL(virDomainRNGModel, >> VIR_DOMAIN_RNG_MODEL_LAST, >> - "virtio"); >> + "virtio", >> + "virtio-transitional", >> + "virtio-non-transitional"); > > Since you're touching the definition, you can make it > > VIR_ENUM_IMPL(virDomainRNGModel, > ... > "virtio-non-transitional", > ); > > so that if and when we'll need to add another model the diff > will look nicer. > Okay I'll adjust it. FWIW I like this style better but it's not well represented in domain_conf.c. I explicitly didn't make a change like this before posting because I had no idea if it would generate a complaint or not. IMO if this is the recommended style then we should fix all instances in one sweep and add a syntax-check for it > [...] >> @@ -1122,6 +1124,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { >> {"virtio-net-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_NET_NON_TRANSITIONAL}, >> {"vhost-scsi-pci-transitional", QEMU_CAPS_DEVICE_VHOST_SCSI_TRANSITIONAL}, >> {"vhost-scsi-pci-non-transitional", QEMU_CAPS_DEVICE_VHOST_SCSI_NON_TRANSITIONAL}, >> + {"virtio-rng-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_RNG_TRANSITIONAL}, >> + {"virtio-rng-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_RNG_NON_TRANSITIONAL}, >> }; > > Same comments as for the other capabilities. > > [...] >> @@ -5990,7 +5995,7 @@ qemuBuildRNGDevStr(const virDomainDef *def, >> { >> virBuffer buf = VIR_BUFFER_INITIALIZER; >> >> - if (dev->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || >> + if (dev->model == VIR_DOMAIN_RNG_MODEL_VIRTIO && >> !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_RNG)) { >> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >> _("this qemu doesn't support RNG device type '%s'"), > > Idea for a possible follow-up series: wouldn't it be great if we > centralized capabilities checking for VirtIO devices in a function > that can be called during device validation? Delaying such checks > until command line generation is not optimal, and neither is having > some of them centralized and some of them sprinkled around the > various qemuBuild*DevStr() functions... > Yeah the validation checks sprinkled all around are a pain, I hit it quite a few times in this series but I tried to resist the temptation of the rabbit hole... > [...] >> @@ -2290,8 +2295,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, >> >> /* VirtIO RNG */ >> for (i = 0; i < def->nrngs; i++) { >> - if (def->rngs[i]->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || >> - !virDeviceInfoPCIAddressIsWanted(&def->rngs[i]->info)) >> + if (!virDeviceInfoPCIAddressIsWanted(&def->rngs[i]->info)) >> continue; > > I'm not convinced you can just remove the model check here... > Shouldn't you extend it to include MODEL_VIRTIO_TRANSITIONAL and > MODEL_VIRTIO_NON_TRANSITIONAL instead? > It seemed redundant... the only enum value for rng model prior to this patch was MODEL_VIRTIO, so any rng device here will by definition be MODEL_VIRTIO. Same after this patch, but plus 2 more virtio models. It's future proof if a non-PCI rng device model is added, but it causes more work when a PCI rng device model is added, so it's a toss up Elsewhere in this function, some devices use a whitelist approach (watchdog, memballoon although it's equally redundant), some use a blacklist approach (sound cards, controllers, basically everything else). By deleting the line here it essentially moves rng address allocation to a blacklist approach. So AFAICT it's safe to do, but I can move the change to a separate patch and add a comment, similar to the comment for FS devices near here Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, 2019-01-21 at 19:01 -0500, Cole Robinson wrote: > On 01/21/2019 08:13 AM, Andrea Bolognani wrote: > > On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: > > [...] > > > VIR_ENUM_IMPL(virDomainRNGModel, > > > VIR_DOMAIN_RNG_MODEL_LAST, > > > - "virtio"); > > > + "virtio", > > > + "virtio-transitional", > > > + "virtio-non-transitional"); > > > > Since you're touching the definition, you can make it > > > > VIR_ENUM_IMPL(virDomainRNGModel, > > ... > > "virtio-non-transitional", > > ); > > > > so that if and when we'll need to add another model the diff > > will look nicer. > > Okay I'll adjust it. FWIW I like this style better but it's not well > represented in domain_conf.c. I explicitly didn't make a change like > this before posting because I had no idea if it would generate a > complaint or not. IMO if this is the recommended style then we should > fix all instances in one sweep and add a syntax-check for it I'm not sure I would qualify this style as "recommended", but I certainly prefer it and there are instances of the pattern around the codebase, so I'll switch to it whenever I happen to be touching a VIR_ENUM_IMPL() call. I haven't had any trouble getting such changes past reviewers so far :) > > [...] > > > @@ -2290,8 +2295,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, > > > > > > /* VirtIO RNG */ > > > for (i = 0; i < def->nrngs; i++) { > > > - if (def->rngs[i]->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || > > > - !virDeviceInfoPCIAddressIsWanted(&def->rngs[i]->info)) > > > + if (!virDeviceInfoPCIAddressIsWanted(&def->rngs[i]->info)) > > > continue; > > > > I'm not convinced you can just remove the model check here... > > Shouldn't you extend it to include MODEL_VIRTIO_TRANSITIONAL and > > MODEL_VIRTIO_NON_TRANSITIONAL instead? > > It seemed redundant... the only enum value for rng model prior to this > patch was MODEL_VIRTIO, so any rng device here will by definition be > MODEL_VIRTIO. Same after this patch, but plus 2 more virtio models. It's > future proof if a non-PCI rng device model is added, but it causes more > work when a PCI rng device model is added, so it's a toss up When in doubt, I usually go for the safest / more explicit version. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/22/2019 07:03 AM, Andrea Bolognani wrote: > On Mon, 2019-01-21 at 19:01 -0500, Cole Robinson wrote: >> On 01/21/2019 08:13 AM, Andrea Bolognani wrote: >>> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote: >>> [...] >>>> VIR_ENUM_IMPL(virDomainRNGModel, >>>> VIR_DOMAIN_RNG_MODEL_LAST, >>>> - "virtio"); >>>> + "virtio", >>>> + "virtio-transitional", >>>> + "virtio-non-transitional"); >>> >>> Since you're touching the definition, you can make it >>> >>> VIR_ENUM_IMPL(virDomainRNGModel, >>> ... >>> "virtio-non-transitional", >>> ); >>> >>> so that if and when we'll need to add another model the diff >>> will look nicer. >> >> Okay I'll adjust it. FWIW I like this style better but it's not well >> represented in domain_conf.c. I explicitly didn't make a change like >> this before posting because I had no idea if it would generate a >> complaint or not. IMO if this is the recommended style then we should >> fix all instances in one sweep and add a syntax-check for it > > I'm not sure I would qualify this style as "recommended", but I > certainly prefer it and there are instances of the pattern around > the codebase, so I'll switch to it whenever I happen to be touching > a VIR_ENUM_IMPL() call. I haven't had any trouble getting such > changes past reviewers so far :) > Noted, I'll do the same >>> [...] >>>> @@ -2290,8 +2295,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, >>>> >>>> /* VirtIO RNG */ >>>> for (i = 0; i < def->nrngs; i++) { >>>> - if (def->rngs[i]->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || >>>> - !virDeviceInfoPCIAddressIsWanted(&def->rngs[i]->info)) >>>> + if (!virDeviceInfoPCIAddressIsWanted(&def->rngs[i]->info)) >>>> continue; >>> >>> I'm not convinced you can just remove the model check here... >>> Shouldn't you extend it to include MODEL_VIRTIO_TRANSITIONAL and >>> MODEL_VIRTIO_NON_TRANSITIONAL instead? >> >> It seemed redundant... the only enum value for rng model prior to this >> patch was MODEL_VIRTIO, so any rng device here will by definition be >> MODEL_VIRTIO. Same after this patch, but plus 2 more virtio models. It's >> future proof if a non-PCI rng device model is added, but it causes more >> work when a PCI rng device model is added, so it's a toss up > > When in doubt, I usually go for the safest / more explicit version. > I mean, it's arguably safer and more explicit to duplicate every DomainValidate device whitelist check in qemu_command.c too, but it comes at the cost of more code and increased developer effort when extending related code. In this particular case it took me extra time when writing this code to figure out why the new rng models were not generating the expected addresses. How about I add a prep patch that moves the RNG model check from qemu_command.c into the qemu*Validate path, that way we are enforcing earlier that no non-virtio RNG model can even enter this part of the qemu driver. Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2019-01-22 at 11:24 -0500, Cole Robinson wrote: > On 01/22/2019 07:03 AM, Andrea Bolognani wrote: > > On Mon, 2019-01-21 at 19:01 -0500, Cole Robinson wrote: > > > > > @@ -2290,8 +2295,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, > > > > > > > > > > /* VirtIO RNG */ > > > > > for (i = 0; i < def->nrngs; i++) { > > > > > - if (def->rngs[i]->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || > > > > > - !virDeviceInfoPCIAddressIsWanted(&def->rngs[i]->info)) > > > > > + if (!virDeviceInfoPCIAddressIsWanted(&def->rngs[i]->info)) > > > > > continue; > > > > > > > > I'm not convinced you can just remove the model check here... > > > > Shouldn't you extend it to include MODEL_VIRTIO_TRANSITIONAL and > > > > MODEL_VIRTIO_NON_TRANSITIONAL instead? > > > > > > It seemed redundant... the only enum value for rng model prior to this > > > patch was MODEL_VIRTIO, so any rng device here will by definition be > > > MODEL_VIRTIO. Same after this patch, but plus 2 more virtio models. It's > > > future proof if a non-PCI rng device model is added, but it causes more > > > work when a PCI rng device model is added, so it's a toss up > > > > When in doubt, I usually go for the safest / more explicit version. > > I mean, it's arguably safer and more explicit to duplicate every > DomainValidate device whitelist check in qemu_command.c too, but it > comes at the cost of more code and increased developer effort when > extending related code. In this particular case it took me extra time > when writing this code to figure out why the new rng models were not > generating the expected addresses. > > How about I add a prep patch that moves the RNG model check from > qemu_command.c into the qemu*Validate path, that way we are enforcing > earlier that no non-virtio RNG model can even enter this part of the > qemu driver. Looking at the code for rng and balloon more closely it's pretty clear that, unlike what happens with most other devices, they've been implemented with the assumption that there will only ever be one model, so I guess what you suggest above would already be an improvement over the status quo while preparing them for further models would be out of scope at best, pointless at worst... Just go with the above, then. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/23/2019 10:40 AM, Andrea Bolognani wrote: > On Tue, 2019-01-22 at 11:24 -0500, Cole Robinson wrote: >> On 01/22/2019 07:03 AM, Andrea Bolognani wrote: >>> On Mon, 2019-01-21 at 19:01 -0500, Cole Robinson wrote: >>>>>> @@ -2290,8 +2295,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, >>>>>> >>>>>> /* VirtIO RNG */ >>>>>> for (i = 0; i < def->nrngs; i++) { >>>>>> - if (def->rngs[i]->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || >>>>>> - !virDeviceInfoPCIAddressIsWanted(&def->rngs[i]->info)) >>>>>> + if (!virDeviceInfoPCIAddressIsWanted(&def->rngs[i]->info)) >>>>>> continue; >>>>> >>>>> I'm not convinced you can just remove the model check here... >>>>> Shouldn't you extend it to include MODEL_VIRTIO_TRANSITIONAL and >>>>> MODEL_VIRTIO_NON_TRANSITIONAL instead? >>>> >>>> It seemed redundant... the only enum value for rng model prior to this >>>> patch was MODEL_VIRTIO, so any rng device here will by definition be >>>> MODEL_VIRTIO. Same after this patch, but plus 2 more virtio models. It's >>>> future proof if a non-PCI rng device model is added, but it causes more >>>> work when a PCI rng device model is added, so it's a toss up >>> >>> When in doubt, I usually go for the safest / more explicit version. >> >> I mean, it's arguably safer and more explicit to duplicate every >> DomainValidate device whitelist check in qemu_command.c too, but it >> comes at the cost of more code and increased developer effort when >> extending related code. In this particular case it took me extra time >> when writing this code to figure out why the new rng models were not >> generating the expected addresses. >> >> How about I add a prep patch that moves the RNG model check from >> qemu_command.c into the qemu*Validate path, that way we are enforcing >> earlier that no non-virtio RNG model can even enter this part of the >> qemu driver. > > Looking at the code for rng and balloon more closely it's pretty > clear that, unlike what happens with most other devices, they've > been implemented with the assumption that there will only ever be > one model, so I guess what you suggest above would already be an > improvement over the status quo while preparing them for further > models would be out of scope at best, pointless at worst... Just > go with the above, then. > Okay, thank you. I'll also add a comment pointing out the situation, along the lines of what fs models have: /* Only support VirtIO-9p-pci so far. If that changes, * we might need to skip devices here */ Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 36dce863ac..04e6dc2721 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -7971,6 +7971,8 @@ qemu-kvm -net nic,model=? /dev/null </p> <ul> <li>'virtio' - supported by qemu and virtio-rng kernel module</li> + <li>'virtio-transitional' <span class='since'>Since 5.1.0</span></li> + <li>'virtio-non-transitional' <span class='since'>Since 5.1.0</span></li> </ul> </dd> <dt><code>rate</code></dt> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 532a78ce32..040fa29914 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -5451,7 +5451,11 @@ <define name="rng"> <element name="rng"> <attribute name="model"> - <value>virtio</value> + <choice> + <value>virtio</value> + <value>virtio-transitional</value> + <value>virtio-non-transitional</value> + </choice> </attribute> <interleave> <ref name="rng-backend"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3776079e1d..cc48599dd1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -858,7 +858,9 @@ VIR_ENUM_IMPL(virDomainDiskTray, VIR_DOMAIN_DISK_TRAY_LAST, VIR_ENUM_IMPL(virDomainRNGModel, VIR_DOMAIN_RNG_MODEL_LAST, - "virtio"); + "virtio", + "virtio-transitional", + "virtio-non-transitional"); VIR_ENUM_IMPL(virDomainRNGBackend, VIR_DOMAIN_RNG_BACKEND_LAST, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 36ab544dd3..dc49cbe4d9 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2125,6 +2125,8 @@ struct _virBlkioDevice { typedef enum { VIR_DOMAIN_RNG_MODEL_VIRTIO, + VIR_DOMAIN_RNG_MODEL_VIRTIO_TRANSITIONAL, + VIR_DOMAIN_RNG_MODEL_VIRTIO_NON_TRANSITIONAL, VIR_DOMAIN_RNG_MODEL_LAST } virDomainRNGModel; diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 70fc510cdb..0241353006 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -528,6 +528,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "virtio-net-pci-non-transitional", "vhost-scsi-pci-transitional", "vhost-scsi-pci-non-transitional", + "virtio-rng-pci-transitional", + "virtio-rng-pci-non-transitional", ); @@ -1122,6 +1124,8 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { {"virtio-net-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_NET_NON_TRANSITIONAL}, {"vhost-scsi-pci-transitional", QEMU_CAPS_DEVICE_VHOST_SCSI_TRANSITIONAL}, {"vhost-scsi-pci-non-transitional", QEMU_CAPS_DEVICE_VHOST_SCSI_NON_TRANSITIONAL}, + {"virtio-rng-pci-transitional", QEMU_CAPS_DEVICE_VIRTIO_RNG_TRANSITIONAL}, + {"virtio-rng-pci-non-transitional", QEMU_CAPS_DEVICE_VIRTIO_RNG_NON_TRANSITIONAL}, }; static struct virQEMUCapsStringFlags virQEMUCapsDevicePropsVirtioBalloon[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index f213ad98cc..b01578d88a 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -512,6 +512,8 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_DEVICE_VIRTIO_NET_NON_TRANSITIONAL, /* -device virtio-net-pci-non-transitional */ QEMU_CAPS_DEVICE_VHOST_SCSI_TRANSITIONAL, /* -device vhost-scsi-pci-transitional */ QEMU_CAPS_DEVICE_VHOST_SCSI_NON_TRANSITIONAL, /* -device vhost-scsi-pci-non-transitional */ + QEMU_CAPS_DEVICE_VIRTIO_RNG_TRANSITIONAL, /* -device virtio-blk-rng-transitional */ + QEMU_CAPS_DEVICE_VIRTIO_RNG_NON_TRANSITIONAL, /* -device virtio-rng-pci-non-transitional */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7cdbd215a6..73f145dcd7 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -477,6 +477,12 @@ qemuBuildVirtioTransitional(virBufferPtr buf, tmodel_cap = QEMU_CAPS_DEVICE_VHOST_SCSI_TRANSITIONAL; ntmodel_cap = QEMU_CAPS_DEVICE_VHOST_SCSI_NON_TRANSITIONAL; break; + case VIR_DOMAIN_DEVICE_RNG: + has_tmodel = model == VIR_DOMAIN_RNG_MODEL_VIRTIO_TRANSITIONAL; + has_ntmodel = model == VIR_DOMAIN_RNG_MODEL_VIRTIO_NON_TRANSITIONAL; + tmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_RNG_TRANSITIONAL; + ntmodel_cap = QEMU_CAPS_DEVICE_VIRTIO_RNG_NON_TRANSITIONAL; + break; case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: @@ -496,7 +502,6 @@ qemuBuildVirtioTransitional(virBufferPtr buf, case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: - case VIR_DOMAIN_DEVICE_RNG: case VIR_DOMAIN_DEVICE_MEMORY: case VIR_DOMAIN_DEVICE_IOMMU: case VIR_DOMAIN_DEVICE_VSOCK: @@ -5990,7 +5995,7 @@ qemuBuildRNGDevStr(const virDomainDef *def, { virBuffer buf = VIR_BUFFER_INITIALIZER; - if (dev->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || + if (dev->model == VIR_DOMAIN_RNG_MODEL_VIRTIO && !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_RNG)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("this qemu doesn't support RNG device type '%s'"), @@ -6002,7 +6007,10 @@ qemuBuildRNGDevStr(const virDomainDef *def, dev->source.file)) goto error; - if (qemuBuildVirtioDevStr(&buf, "virtio-rng", dev->info.type) < 0) + if (qemuBuildVirtioTransitional(&buf, "virtio-rng", qemuCaps, + dev->info.type, + dev->model, NULL, + VIR_DOMAIN_DEVICE_RNG) < 0) goto error; virBufferAsprintf(&buf, ",rng=obj%s,id=%s", diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index c334ba441f..a5491a1c0e 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -366,7 +366,9 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def, def->memballoon->info.type = type; for (i = 0; i < def->nrngs; i++) { - if (def->rngs[i]->model == VIR_DOMAIN_RNG_MODEL_VIRTIO && + if ((def->rngs[i]->model == VIR_DOMAIN_RNG_MODEL_VIRTIO || + def->rngs[i]->model == VIR_DOMAIN_RNG_MODEL_VIRTIO_TRANSITIONAL || + def->rngs[i]->model == VIR_DOMAIN_RNG_MODEL_VIRTIO_NON_TRANSITIONAL) && def->rngs[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) def->rngs[i]->info.type = type; } @@ -861,7 +863,10 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev, case VIR_DOMAIN_DEVICE_RNG: switch ((virDomainRNGModel) dev->data.rng->model) { case VIR_DOMAIN_RNG_MODEL_VIRTIO: + case VIR_DOMAIN_RNG_MODEL_VIRTIO_NON_TRANSITIONAL: return virtioFlags; + case VIR_DOMAIN_RNG_MODEL_VIRTIO_TRANSITIONAL: + return pciFlags; case VIR_DOMAIN_RNG_MODEL_LAST: return 0; @@ -2290,8 +2295,7 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, /* VirtIO RNG */ for (i = 0; i < def->nrngs; i++) { - if (def->rngs[i]->model != VIR_DOMAIN_RNG_MODEL_VIRTIO || - !virDeviceInfoPCIAddressIsWanted(&def->rngs[i]->info)) + if (!virDeviceInfoPCIAddressIsWanted(&def->rngs[i]->info)) continue; if (qemuDomainPCIAddressReserveNextAddr(addrs, &def->rngs[i]->info) < 0) diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml index c5079c4028..e54fe53590 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml @@ -218,6 +218,8 @@ <flag name='virtio-net-pci-non-transitional'/> <flag name='vhost-scsi-pci-transitional'/> <flag name='vhost-scsi-pci-non-transitional'/> + <flag name='virtio-rng-pci-transitional'/> + <flag name='virtio-rng-pci-non-transitional'/> <version>3001050</version> <kvmVersion>0</kvmVersion> <microcodeVersion>446361</microcodeVersion> diff --git a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args index 5ab8560377..a3c81828c0 100644 --- a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args +++ b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-3.1.0.args @@ -28,6 +28,7 @@ addr=0x1 \ -device pcie-root-port,port=0x9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \ -device pcie-root-port,port=0xa,chassis=3,id=pci.3,bus=pcie.0,addr=0x1.0x2 \ -device pcie-root-port,port=0xb,chassis=4,id=pci.4,bus=pcie.0,addr=0x1.0x3 \ +-device pcie-root-port,port=0xc,chassis=5,id=pci.5,bus=pcie.0,addr=0x1.0x4 \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \ -device virtio-blk-pci,disable-legacy=on,scsi=off,bus=pci.2,addr=0x0,\ drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \ @@ -36,6 +37,9 @@ drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \ mac=00:11:22:33:44:55,bus=pci.1,addr=0x0 \ -device vhost-scsi-pci,disable-legacy=on,wwpn=naa.5123456789abcde0,vhostfd=3,\ id=hostdev0,bus=pci.3,addr=0x0 \ +-object rng-random,id=objrng0,filename=/dev/urandom \ +-device virtio-rng-pci,disable-legacy=on,rng=objrng0,id=rng0,bus=pci.4,\ +addr=0x0 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ -msg timestamp=on diff --git a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args index c8dbffda65..a0fc475c2f 100644 --- a/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args +++ b/tests/qemuxml2argvdata/virtio-non-transitional.x86_64-latest.args @@ -28,6 +28,7 @@ addr=0x1 \ -device pcie-root-port,port=0x9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \ -device pcie-root-port,port=0xa,chassis=3,id=pci.3,bus=pcie.0,addr=0x1.0x2 \ -device pcie-root-port,port=0xb,chassis=4,id=pci.4,bus=pcie.0,addr=0x1.0x3 \ +-device pcie-root-port,port=0xc,chassis=5,id=pci.5,bus=pcie.0,addr=0x1.0x4 \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-virtio-disk0 \ -device virtio-blk-pci-non-transitional,scsi=off,bus=pci.2,addr=0x0,\ drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \ @@ -36,6 +37,8 @@ drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \ mac=00:11:22:33:44:55,bus=pci.1,addr=0x0 \ -device vhost-scsi-pci-non-transitional,wwpn=naa.5123456789abcde0,vhostfd=3,\ id=hostdev0,bus=pci.3,addr=0x0 \ +-object rng-random,id=objrng0,filename=/dev/urandom \ +-device virtio-rng-pci-non-transitional,rng=objrng0,id=rng0,bus=pci.4,addr=0x0 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ -msg timestamp=on diff --git a/tests/qemuxml2argvdata/virtio-non-transitional.xml b/tests/qemuxml2argvdata/virtio-non-transitional.xml index 32d2bdc638..2075ccbf57 100644 --- a/tests/qemuxml2argvdata/virtio-non-transitional.xml +++ b/tests/qemuxml2argvdata/virtio-non-transitional.xml @@ -19,6 +19,9 @@ <hostdev mode='subsystem' type='scsi_host' managed='no' model='virtio-non-transitional'> <source protocol='vhost' wwpn='naa.5123456789abcde0'/> </hostdev> + <rng model='virtio-non-transitional'> + <backend model='random'>/dev/urandom</backend> + </rng> <controller type='usb' index='0' model='none'/> <memballoon model='none'/> </devices> diff --git a/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args b/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args index 38a9e348b3..dc830d21b1 100644 --- a/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args +++ b/tests/qemuxml2argvdata/virtio-transitional.x86_64-3.1.0.args @@ -35,6 +35,8 @@ id=virtio-disk0,bootindex=1 \ addr=0x1 \ -device vhost-scsi-pci,wwpn=naa.5123456789abcde0,vhostfd=3,id=hostdev0,\ bus=pci.2,addr=0x3 \ +-object rng-random,id=objrng0,filename=/dev/urandom \ +-device virtio-rng-pci,rng=objrng0,id=rng0,bus=pci.2,addr=0x4 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ -msg timestamp=on diff --git a/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args b/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args index ab2c35514d..64fb4153fd 100644 --- a/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args +++ b/tests/qemuxml2argvdata/virtio-transitional.x86_64-latest.args @@ -35,6 +35,8 @@ drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \ mac=00:11:22:33:44:55,bus=pci.2,addr=0x1 \ -device vhost-scsi-pci-transitional,wwpn=naa.5123456789abcde0,vhostfd=3,\ id=hostdev0,bus=pci.2,addr=0x3 \ +-object rng-random,id=objrng0,filename=/dev/urandom \ +-device virtio-rng-pci-transitional,rng=objrng0,id=rng0,bus=pci.2,addr=0x4 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ -msg timestamp=on diff --git a/tests/qemuxml2argvdata/virtio-transitional.xml b/tests/qemuxml2argvdata/virtio-transitional.xml index eddc1ce9f5..82535c84d6 100644 --- a/tests/qemuxml2argvdata/virtio-transitional.xml +++ b/tests/qemuxml2argvdata/virtio-transitional.xml @@ -19,6 +19,9 @@ <hostdev mode='subsystem' type='scsi_host' managed='no' model='virtio-transitional'> <source protocol='vhost' wwpn='naa.5123456789abcde0'/> </hostdev> + <rng model='virtio-transitional'> + <backend model='random'>/dev/urandom</backend> + </rng> <controller type='usb' index='0' model='none'/> <memballoon model='none'/> </devices> diff --git a/tests/qemuxml2xmloutdata/virtio-non-transitional.xml b/tests/qemuxml2xmloutdata/virtio-non-transitional.xml index 2af5195dfd..4a315050ab 100644 --- a/tests/qemuxml2xmloutdata/virtio-non-transitional.xml +++ b/tests/qemuxml2xmloutdata/virtio-non-transitional.xml @@ -45,6 +45,11 @@ <target chassis='4' port='0xb'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x3'/> </controller> + <controller type='pci' index='5' model='pcie-root-port'> + <model name='pcie-root-port'/> + <target chassis='5' port='0xc'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x4'/> + </controller> <interface type='user'> <mac address='00:11:22:33:44:55'/> <model type='virtio-non-transitional'/> @@ -57,5 +62,9 @@ <address type='pci' domain='0x0000' bus='0x03' slot='0x00' function='0x0'/> </hostdev> <memballoon model='none'/> + <rng model='virtio-non-transitional'> + <backend model='random'>/dev/urandom</backend> + <address type='pci' domain='0x0000' bus='0x04' slot='0x00' function='0x0'/> + </rng> </devices> </domain> diff --git a/tests/qemuxml2xmloutdata/virtio-transitional.xml b/tests/qemuxml2xmloutdata/virtio-transitional.xml index 8c1baced0e..ae3789ad93 100644 --- a/tests/qemuxml2xmloutdata/virtio-transitional.xml +++ b/tests/qemuxml2xmloutdata/virtio-transitional.xml @@ -51,5 +51,9 @@ <address type='pci' domain='0x0000' bus='0x02' slot='0x03' function='0x0'/> </hostdev> <memballoon model='none'/> + <rng model='virtio-transitional'> + <backend model='random'>/dev/urandom</backend> + <address type='pci' domain='0x0000' bus='0x02' slot='0x04' function='0x0'/> + </rng> </devices> </domain>
Add new <rng> model values for virtio transitional devices. Ex: <rng model='virtio-transitional'> ... </rng> * "virtio-transitional" maps to qemu "virtio-rng-pci-transitional" * "virtio-non-transitional" maps to qemu "virtio-rng-pci-non-transitional" Signed-off-by: Cole Robinson <crobinso@redhat.com> --- docs/formatdomain.html.in | 2 ++ docs/schemas/domaincommon.rng | 6 +++++- src/conf/domain_conf.c | 4 +++- src/conf/domain_conf.h | 2 ++ src/qemu/qemu_capabilities.c | 4 ++++ src/qemu/qemu_capabilities.h | 2 ++ src/qemu/qemu_command.c | 14 +++++++++++--- src/qemu/qemu_domain_address.c | 10 +++++++--- tests/qemucapabilitiesdata/caps_4.0.0.x86_64.xml | 2 ++ .../virtio-non-transitional.x86_64-3.1.0.args | 4 ++++ .../virtio-non-transitional.x86_64-latest.args | 3 +++ tests/qemuxml2argvdata/virtio-non-transitional.xml | 3 +++ .../virtio-transitional.x86_64-3.1.0.args | 2 ++ .../virtio-transitional.x86_64-latest.args | 2 ++ tests/qemuxml2argvdata/virtio-transitional.xml | 3 +++ .../qemuxml2xmloutdata/virtio-non-transitional.xml | 9 +++++++++ tests/qemuxml2xmloutdata/virtio-transitional.xml | 4 ++++ 17 files changed, 68 insertions(+), 8 deletions(-) -- 2.20.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list