[libvirt] [PATCH v1 4/4] domain capabilities: Expose firmware auto selection feature
Michal Privoznik
mprivozn at redhat.com
Tue Apr 9 14:27:20 UTC 2019
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
More information about the libvir-list
mailing list