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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
