[libvirt] [PATCH] qemu: Support OVMF on aarch64 guests
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)); -- 2.0.4
On Wed, Nov 19, 2014 at 04:30:22PM +0100, 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));
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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 (sorry for not following up on the original patch, still haven't gotten back to doing virt-* and aarch64 testing since KVM forum. - Cole
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. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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 - Cole
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 Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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 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
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.
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. Michal
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@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. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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@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
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@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.
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.
I prefer whitelisting but it's not my call. Laszlo
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@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 -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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@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
On 11/19/14 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:
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.
How is dropping the check completely *not* predicting the future? If you remove the check, then libvirt will compose command lines (for some arches) that will *maybe* work in the future. It'll take shots in the dark.
If the invocation syntax is supported by QEMU
That's a big "if", yes. The invocation syntax was supported for x86_64 for sure, definitely not supported for a bunch of other targets, and was *maybe* going to be supported (at that time) in aarch64. (But I had seen contradicting patches too on qemu-devel; for example '-bios' had been proposed to diverge on arm from how it used to work with x86_64, ie. for UEFI usage; ie. it looked possible that aarch64 would take one -bios and one -pflash rather than two -pflash options.)
then libvirt should not be trying to block it. libvirt should focus should be on the mechanism, not the usage policy
I agree.
& this check really amounts to a usage policy check.
The check was intended as "we really don't know how the mechanism will look like (apart from x86_64), so let's keep you safe". There was absolutely no promise from qemu that aarch64 would take the same options as x86_64, and now that it does (which is pure luck), there's still no promise that further targets would take the same (once they become interested in UEFI). Thanks Laszlo
On 11/19/14 17:42, 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.
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.
I preferred the whitelisting because at that time x86_64 was the only architecture (== qemu target) that supported UEFI. A clear error message directly from libvirtd is better for users than an obscure error message from qemu (let alone a qemu that is perhaps willing to start but then does bad things). If the command line proves "universal" for the majority of qemu targets, then by all means drop the check. I'm just unaware of any qemu introspection that would clearly indicate or counter-indicate UEFI support. Thanks Laszlo
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
On 19.11.2014 16:40, 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
(sorry for not following up on the original patch, still haven't gotten back to doing virt-* and aarch64 testing since KVM forum.
- Cole
Thanks, fixed and pushed. Michal
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Laszlo Ersek -
Michal Privoznik