[libvirt] [PATCH] qemu: Support OVMF on aarch64 guests

[Michal Privoznik]

On 19.11.2014 18:02, Daniel P. Berrange wrote:
> On Wed, Nov 19, 2014 at 05:57:24PM +0100, Laszlo Ersek wrote:
>> On 11/19/14 17:46, Daniel P. Berrange wrote:
>>> On Wed, Nov 19, 2014 at 05:42:35PM +0100, Michal Privoznik wrote:
>>>> On 19.11.2014 17:28, Cole Robinson wrote:
>>>>> On 11/19/2014 11:22 AM, Daniel P. Berrange wrote:
>>>>>> On Wed, Nov 19, 2014 at 11:17:30AM -0500, Cole Robinson wrote:
>>>>>>> On 11/19/2014 11:13 AM, Daniel P. Berrange wrote:
>>>>>>>> On Wed, Nov 19, 2014 at 10:40:09AM -0500, Cole Robinson wrote:
>>>>>>>>> On 11/19/2014 10:30 AM, Michal Privoznik wrote:
>>>>>>>>>> Currently, we are whitelisting architectures, that we know how to run
>>>>>>>>>> OVMF on. So far, only x86_64 was enabled. However, looking at qemu
>>>>>>>>>> code, the same commandline can be used to enable OVMF for aarch64.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>>>>>>>>> ---
>>>>>>>>>>   src/qemu/qemu_command.c | 3 ++-
>>>>>>>>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>>>>>>>>> index d2e6991..ca57e35 100644
>>>>>>>>>> --- a/src/qemu/qemu_command.c
>>>>>>>>>> +++ b/src/qemu/qemu_command.c
>>>>>>>>>> @@ -7749,7 +7749,8 @@
>>>>>>>>>> qemuBuildDomainLoaderCommandLine(virCommandPtr cmd,
>>>>>>>>>>
>>>>>>>>>>       case VIR_DOMAIN_LOADER_TYPE_PFLASH:
>>>>>>>>>>           /* UEFI is supported only for x86_64 currently */
>>>>>>>>>> -        if (def->os.arch != VIR_ARCH_X86_64) {
>>>>>>>>>> +        if (def->os.arch != VIR_ARCH_X86_64 &&
>>>>>>>>>> +            def->os.arch != VIR_ARCH_AARCH64) {
>>>>>>>>>>               virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>>>>>>>>>                              _("pflash is not supported for %s
>>>>>>>>>> guest architecture"),
>>>>>>>>>>                              virArchToString(def->os.arch));
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Please add armv7hl as well, it should work completely identically
>>>>>>>>> (if/when
>>>>>>>>> we have an OS that supports it). ACK with that
>>>>>>>>
>>>>>>>> Really ? I thought ARM7 world was going to use its legacy BIOS
>>>>>>>> approach forever, only AArch64 going for UEFI approach.
>>>>>>>
>>>>>>> There is arm32 support in UEFI, but I don't know if distros are ever
>>>>>>> going
>>>>>>> to do the work of adopting it, because real hw is all u-boot based.
>>>>>>>
>>>>>>> But -M virt is very similar regardless of aarch64 or arm32, so _if_
>>>>>>> anyone
>>>>>>> ever produces an arm32 disk image with uefi boot support, the qemu
>>>>>>> command
>>>>>>> line should be identical to the aarch64 WRT uefi/nvram/pflash. That's my
>>>>>>> understanding anyways
>>>>>>
>>>>>> Ok, I guess it doesn't hurt to have it enabled for arm7 then, even if
>>>>>> no one is likely to use it
>>>>>>
>>>>>
>>>>> Agreed
>>>>>
>>>>> though frankly I don't really understand the point of restricting it in
>>>>> libvirt code to x86 in the first place. if we hadn't done that, we
>>>>> wouldn't need this patch for aarch64. Hence my original patch to just
>>>>> drop the arch check entirely
>>>>
>>>> I believe there was this concern that other architectures may require
>>>> different cmd line to use UEFI. On x86_64 the UEFI firmware and NVRAM store
>>>> are passed as flash devices that qemu maps into guest memory (at some
>>>> specific address). And other arches may have different approach and thus
>>>> different command line. So I've decided to be explicit which architectures
>>>> we support UEFI on.
>>>>
>>>>> I understand sometimes detecting error conditions in libvirt before qemu
>>>>> can throw an error is important for improving error reporting. But we
>>>>> should be careful about trying to get into the game of predicting what
>>>>> will and won't work with qemu, it's just more code that needs to be
>>>>> maintained and kept up to date. Just my 2 cents
>>>>
>>>> I see your point, although we are already in that game. When building
>>>> command line qemuCaps is consulted heavily to predict what will work and
>>>> what will not.
>>>
>>> The difference there is that qemuCaps is populated based on what
>>> we've actually queried from QEMU.
>>>
>>> This arch check for OVMF is an arbitrary check placed in libvirt
>>> code which is not related to the current QEMU binary in any way.
>>
>> It communicates what we had looked at and had determined to work. I
>> preferred not to enable targets that (a) had been known not to work with
>> UEFI, (b) had not been known to work or not with UEFI.
>>
>> At that time noone knew that (1) edk2's ArmVirtualizationQemu platform
>> would be born soon (in other words, a UEFI binary for
>> 'qemu-system-aarch64 -M virt' would be implemented soon), (2) what
>> switches 'qemu-system-aarch64 -M virt' would actually take for it to work.
>
> That's exactly why this check is wrong in libvirt IMHO. Because it is not
> based on any detected feature from QEMU it amounts to an attempt to predict
> the future. If the invocation syntax is supported by QEMU then libvirt
> should not be trying to block it. libvirt should focus should be on the
> mechanism, not the usage policy & this check really amounts to a usage
> policy check.
>
> Regards,
> Daniel
>

Okay, you've persuaded me. Let me propose a patch, that drops the checks.

Michal