[libvirt] [PATCH v1 4/4] domain capabilities: Expose firmware auto selection feature

[Laszlo Ersek]

On 04/05/19 10:44, Michal Privoznik wrote:
> On 4/5/19 10:31 AM, Daniel P. Berrangé wrote:
>> On Fri, Apr 05, 2019 at 09:19:48AM +0200, Michal Privoznik wrote:
>>> If a management application wants to use firmware auto selection
>>> feature it can't currently know if the libvirtd it's talking to
>>> support is or not. Moreover, it doesn't know which values that
>>> are accepted for the @firmware attribute of <os/> when parsing
>>> will allow successful start of the domain later, i.e. if the mgmt
>>> application wants to use 'bios' whether there exists a FW
>>> descriptor in the system that describes bios.
>>>
>>> This commit then adds 'firmware' enum to <os/> element in
>>> <domainCapabilities/> XML like this:
>>>
>>>    <enum name='firmware'>
>>>      <value>bios</value>
>>>      <value>efi</value>
>>>    </enum>
>>>
>>> We can see both 'bios' and 'efi' listed which means that there
>>> are descriptors for both found in the system (matched with the
>>> machine type and architecture reported in the domain capabilities
>>> earlier and not shown here).
>>
>> I wonder if we should also have a enum for the "secure" attribute
>> of <loader> to deal with SecureBoot
>>
>>     <enum name='secure'>
>>       <value>yes</value>
>>       <value>no</value>
>>     </enum>
>>
>> Always report "no", only report "yes" if there is at least one
>> firmware file we see that can do SecureBoot.
>>
>> Yes, in theory that is a matrix against the firmware attribute
>> value, but we have many such dependancies between attributes
>> and it is not practical to fully express all valid combinations
>> in the capabilities, so taking the simple approach is valid
>> IMHO.
>
> Makes sense, I'll put it on my TODO list for v2.

With the above, the series looks good to me as well (mostly ready to
ACK).

I have a low-level question though. In patch #3:

  verify(sizeof(unsigned int) >= ((1ULL << VIR_DOMAIN_OS_DEF_FIRMWARE_LAST) >> 2));

are you trying to express that all non-LAST enum values are <= 31?

I'm probably missing something, but on the LHS, you have a number of
bytes, while on the RHS, you have a bitmask with the "LAST" bit set,
divided by 4 (not 8).


A related question for patch #4:

  1 << VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS

Apparently, we'd like to store such bitmasks in "unsigned int" objects
however (not in "int"s), so all similar expressions should be written
like

  1u << VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS

Because otherwise, if (1 << VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS) is
unrepresentable as a signed int (e.g. int is int32_t and
VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS has value 31), then the behavior is
undefined.


... Honestly I'd just go with "uint64_t" -- it's an optional type, per
standard C, but libvirt already uses that type elsewhere,
unconditionally. Then you could use:

  verify(VIR_DOMAIN_OS_DEF_FIRMWARE_LAST <= 64);

(assuming you never want to set the "LAST" bit in the mask)

and

  UINT64_C(1) << VIR_DOMAIN_OS_DEF_FIRMWARE_BIOS

for creating the single-bit masks.

Thanks
Laszlo