[libvirt] [PATCH 08/18] qemu: Support rng model=virtio-{non-}transitional

[Cole Robinson]

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