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

[Cole Robinson]

On 11/19/2014 11:46 AM, 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.
> I think that's fairly dubious in general and I'd be in favour of
> just removing this arch check entirely, unless there's a way to
> actually probe support from the QEMU binary to control this.

Hmm, I also just noticed that the x86_64 check is reproduced in 
qemu_capabilities as well, for populating domcapabilities output. So a small 
additional patch is needed (to satisfy virt-manager at least)

- Cole