Re: [libvirt] [PATCH 08/18] qemu: Support rng model=virtio-{non-}transitional
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