[libvirt] [PATCH 1/2] Add armv6l Support as guest

[Andrea Bolognani]

On Sun, 2018-11-25 at 21:09 +0000, infos at nafets.de wrote:
> From: Stefan Schallenberg <nafets227 at users.noreply.github.com>
> 
> Support for armv6l qemu guests has been added.
> Tested with arm1176 CPU on x86.

What OS did you run inside the guest? I would like to give this
a try myself.

> Signed-off-by: Stefan Schallenberg <infos at nafets.de>

The S-o-b should contain a proper, non-obfuscated email address.

[...]
> +      <change>
> +        <summary>
> +          Add armv6l Support as guest
> +        </summary>
> +        <description>
> +          Support for armv6l qemu guests has been added.
> +          Tested with arm1176 CPU on x86.
> +        </description>
> +      </change>

Changes to the release notes should be in a separate commit.

I would also put this in the "New features" section, and tweak it
slightly for style so that it looks along the lines of

  <change>
    <summary>
      qemu: Add support for ARMv6l guests
    </summary>
  </change>

The change is pretty self-explanatory, and how the feature was
tested is good information to have in the commit message but not
really release notes material :)

[...]
> +++ b/docs/schemas/basictypes.rng
> @@ -407,6 +407,7 @@
>        <value>aarch64</value>
>        <value>alpha</value>
>        <value>armv7l</value>
> +      <value>armv6l</value>

Entries are sorted, so armv6l should go *before* armv7l.

[...]
> @@ -2199,7 +2199,7 @@ static const char *preferredMachines[] =
>  {
>      NULL, /* VIR_ARCH_NONE (not a real arch :) */
>      "clipper", /* VIR_ARCH_ALPHA */
> -    NULL, /* VIR_ARCH_ARMV6L (no QEMU impl) */
> +    "versatilepb", /* VIR_ARCH_ARMV6L */
>      "integratorcp", /* VIR_ARCH_ARMV7L */
>      "integratorcp", /* VIR_ARCH_ARMV7B */

How did you arrive to the conclusion that versatilepb should be
the default machine type for the architecture? Given how QEMU
works, I would expect it to match the ARMv7 default.

Note that there's a patch on the list that changes all defaults
for ARM architectures to NULL, so based on whether that patch is
ultimately merged you might want to do the same.

[...]
> @@ -4177,6 +4177,7 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
>  
>      /* GIC capabilities, eg. available GIC versions */
>      if ((qemuCaps->arch == VIR_ARCH_AARCH64 ||
> +         qemuCaps->arch == VIR_ARCH_ARMV6L ||
>           qemuCaps->arch == VIR_ARCH_ARMV7L) &&

This, and many similar conditions, could basically be rewritten
as

  if (ARCH_IS_ARM(...))

were it not for the fact that ARCH_IS_ARM() also includes ARMv7b.

I wonder if it would make sense to have a few more fine-grained
macros such as

  ARCH_IS_ARM64() / ARCH_IS_ARM32()
  ARCH_IS_ARMLE() / ARCH_IS_ARMBE()

and whether we should introduce

  VIR_ARCH_AARCH64BE / VIR_ARCH_ARMV6B

to cover all bases - at least as far as I know, all CPUs dating
back to ARMv6 can work in both Little Endian and Big Endian mode,
so limiting the latter to ARMv7 only seems wrong.

This is just me thinking out loud about the bigger picture, though:
for the purpose of your patch, the way you're changing the various
conditions is just fine.

[...]
> --- a/tests/capabilityschemadata/caps-qemu-kvm.xml
> +++ b/tests/capabilityschemadata/caps-qemu-kvm.xml
> @@ -81,6 +81,16 @@
>      </features>
>    </guest>
>  
> +<guest>
> +    <os_type>hvm</os_type>
> +    <arch name='armv7l'>
> +      <wordsize>32</wordsize>
> +      <emulator>/usr/bin/qemu-system-arm</emulator>
> +      <machine>versatilepb</machine>
> +      <domain type='qemu'/>
> +    </arch>
> +  </guest>

This looks very wrong - you added support for the ARMv6l
architecture, and suddenly an entry for the *ARMv7l* architecture
popped up!

It's not indented properly, either. Did you add this manually?

-- 
Andrea Bolognani / Red Hat / Virtualization