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(a)redhat.com&gt; --- 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));

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(a)redhat.com&gt; >> --- >> 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.

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(a)redhat.com&gt; >>>> --- >>>> 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

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(a)redhat.com&gt; >>>>>>--- >>>>>> 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.

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(a)redhat.com&gt; >>>>>>> --- >>>>>>> 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.

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(a)redhat.com&gt; >>>>>>>>> --- >>>>>>>>> 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

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(a)redhat.com&gt; >>>>>> --- >>>>>> 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.

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(a)redhat.com&gt; > --- > 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