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

[Michal Privoznik]

On 4/8/19 1:33 PM, Laszlo Ersek wrote:
> 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).

Ah, right. Yeah, I want to ensure that all VIR_DOMAIN_SO_DEF_* values 
can be shifted left and still fit into uint. This is because of the way 
these are returned from qemuFirmwareGetSupported(): 1 << 
VIR_DOMAIN_OS_DEF_FIRMWARE_*. Migt as well check if 
(VIR_DOMAIN_OS_DEF_FIRMWARE_LAST * 8) <= sizeof(int).

> 
> 
> 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.

Okay, let me fix this and repost.

Thanks,
Michal