[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 11/19/2014 11:42 AM, 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 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.

True, I didn't think of it that way. In this instance loader=rw is kind of a higher level interface that could map to different cli per architecture. In practice though there aren't many instances of that in qemu_commmand.c that I can think of


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

- Cole

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.


IMO qemuCaps should just be used for determining how to format the command line depending on what qemu features are available, or avoiding situations that we know qemu does the wrong thing or doesn't error out correctly.

But using it to error like 'libvirt: this qemu doesn't support FOO' instead of 'libvirt: qemu-system-x86_64: -device FOO: 'FOO' is not a valid device model name' or similar is rarely useful IMO, of which there is plenty of usages in qemu_command.c

- Cole


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]