Re: [libvirt] [PATCH v1 4/4] domain capabilities: Expose firmware auto selection feature
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