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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
