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.