[PATCH kvmtool 0/6] Various convenience fixes

[PATCH kvmtool 0/6] Various convenience fixes

[Andre Przywara]

As I found myself using kvmtool more often in the last weeks, I got more
and more annoyed by some smaller "ticks" that kvmtool shows.

So this is an attempt to post various smaller fixes I gathered up over
the years, but never found worth enough to send out:

- drop unnecessarily detailed debug output
- add /chosen/stdout-path node in .dtb
- honour make -s switch
- remove pointless kvmtool version number
- improve meaningless PMU error message
- introduce autodetection of supported GIC type

Please have a look!

Cheers,
Andre.

Andre Przywara (6):
  arm: turn pr_info() into pr_debug() messages
  arm: fdt: add stdout-path to /chosen node
  Makefile: support -s switch
  Makefile: Remove echoing of kvmtools version file
  arm: pmu: Improve PMU error reporting
  arm: Auto-detect guest GIC type

 Makefile                     | 16 +++++++++++++++-
 arm/fdt.c                    |  3 ++-
 arm/gic.c                    | 25 +++++++++++++++++++++++++
 arm/include/arm-common/gic.h |  1 +
 arm/kvm.c                    | 16 ++++++++--------
 arm/pmu.c                    |  2 +-
 util/KVMTOOLS-VERSION-GEN    |  1 -
 virtio/mmio.c                |  3 ++-
 8 files changed, 54 insertions(+), 13 deletions(-)

-- 
2.17.1

[PATCH kvmtool 1/6] arm: turn pr_info() into pr_debug() messages

[Andre Przywara]

For whatever reason on ARM/arm64 machines kvmtool greets us with quite
some elaborate messages:
  Info: Loaded kernel to 0x80080000 (18704896 bytes)
  Info: Placing fdt at 0x8fe00000 - 0x8fffffff
  Info: virtio-mmio.devices=0x200@0x10000:36

  Info: virtio-mmio.devices=0x200@0x10200:37

  Info: virtio-mmio.devices=0x200@0x10400:38

This is not really useful information for the casual user, so change
those lines to use pr_debug().
This also fixes the long standing line ending issue for the mmio output.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/fdt.c     |  2 +-
 arm/kvm.c     | 16 ++++++++--------
 virtio/mmio.c |  3 ++-
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/arm/fdt.c b/arm/fdt.c
index 980015b4..28ba1c2c 100644
--- a/arm/fdt.c
+++ b/arm/fdt.c
@@ -36,7 +36,7 @@ static void dump_fdt(const char *dtb_file, void *fdt)
 	if (count < 0)
 		die_perror("Failed to dump dtb");
 
-	pr_info("Wrote %d bytes to dtb %s\n", count, dtb_file);
+	pr_debug("Wrote %d bytes to dtb %s", count, dtb_file);
 	close(fd);
 }
 
diff --git a/arm/kvm.c b/arm/kvm.c
index b824f637..8dc6989c 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -113,8 +113,8 @@ bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
 		die_perror("kernel read");
 	}
 	kernel_end = pos + file_size;
-	pr_info("Loaded kernel to 0x%llx (%zd bytes)",
-		kvm->arch.kern_guest_start, file_size);
+	pr_debug("Loaded kernel to 0x%llx (%zd bytes)",
+		 kvm->arch.kern_guest_start, file_size);
 
 	/*
 	 * Now load backwards from the end of memory so the kernel
@@ -129,9 +129,9 @@ bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
 		die("fdt overlaps with kernel image.");
 
 	kvm->arch.dtb_guest_start = guest_addr;
-	pr_info("Placing fdt at 0x%llx - 0x%llx",
-		kvm->arch.dtb_guest_start,
-		host_to_guest_flat(kvm, limit));
+	pr_debug("Placing fdt at 0x%llx - 0x%llx",
+		 kvm->arch.dtb_guest_start,
+		 host_to_guest_flat(kvm, limit));
 	limit = pos;
 
 	/* ... and finally the initrd, if we have one. */
@@ -159,9 +159,9 @@ bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
 
 		kvm->arch.initrd_guest_start = initrd_start;
 		kvm->arch.initrd_size = file_size;
-		pr_info("Loaded initrd to 0x%llx (%llu bytes)",
-			kvm->arch.initrd_guest_start,
-			kvm->arch.initrd_size);
+		pr_debug("Loaded initrd to 0x%llx (%llu bytes)",
+			 kvm->arch.initrd_guest_start,
+			 kvm->arch.initrd_size);
 	} else {
 		kvm->arch.initrd_size = 0;
 	}
diff --git a/virtio/mmio.c b/virtio/mmio.c
index 8c0f1cc1..9e0cd05d 100644
--- a/virtio/mmio.c
+++ b/virtio/mmio.c
@@ -307,7 +307,8 @@ int virtio_mmio_init(struct kvm *kvm, void *dev, struct virtio_device *vdev,
 	 *
 	 * virtio_mmio.devices=0x200@0xd2000000:5,0x200@0xd2000200:6
 	 */
-	pr_info("virtio-mmio.devices=0x%x@0x%x:%d\n", VIRTIO_MMIO_IO_SIZE, vmmio->addr, vmmio->irq);
+	pr_debug("virtio-mmio.devices=0x%x@0x%x:%d", VIRTIO_MMIO_IO_SIZE,
+		 vmmio->addr, vmmio->irq);
 
 	return 0;
 }
-- 
2.17.1

[PATCH kvmtool 2/6] arm: fdt: add stdout-path to /chosen node

[Andre Przywara]

The DT spec describes the stdout-path property in the /chosen node to
contain the DT path for a default device usable for outputting characters.
The Linux kernel uses this for earlycon (without further parameters),
other DT users might rely on this as well.

Add a property containing the path to our emulated 8250 serial device.

Even when we use the virtio console, the serial console is still there
and works, so we can expose this unconditionally. Putting the virtio
console path in there will not work anyway.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/fdt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arm/fdt.c b/arm/fdt.c
index 28ba1c2c..8cda3ded 100644
--- a/arm/fdt.c
+++ b/arm/fdt.c
@@ -143,6 +143,7 @@ static int setup_fdt(struct kvm *kvm)
 	_FDT(fdt_property_cell(fdt, "linux,pci-probe-only", 1));
 	_FDT(fdt_property_string(fdt, "bootargs", kvm->cfg.real_cmdline));
 	_FDT(fdt_property_u64(fdt, "kaslr-seed", kvm->cfg.arch.kaslr_seed));
+	_FDT(fdt_property_string(fdt, "stdout-path", "/U6_16550A@3f8"));
 
 	/* Initrd */
 	if (kvm->arch.initrd_size != 0) {
-- 
2.17.1

Re: [PATCH kvmtool 2/6] arm: fdt: add stdout-path to /chosen node

[Will Deacon]

On Fri, Jan 25, 2019 at 06:07:57PM +0000, Andre Przywara wrote:
> The DT spec describes the stdout-path property in the /chosen node to
> contain the DT path for a default device usable for outputting characters.
> The Linux kernel uses this for earlycon (without further parameters),
> other DT users might rely on this as well.
> 
> Add a property containing the path to our emulated 8250 serial device.
> 
> Even when we use the virtio console, the serial console is still there
> and works, so we can expose this unconditionally. Putting the virtio
> console path in there will not work anyway.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  arm/fdt.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arm/fdt.c b/arm/fdt.c
> index 28ba1c2c..8cda3ded 100644
> --- a/arm/fdt.c
> +++ b/arm/fdt.c
> @@ -143,6 +143,7 @@ static int setup_fdt(struct kvm *kvm)
>  	_FDT(fdt_property_cell(fdt, "linux,pci-probe-only", 1));
>  	_FDT(fdt_property_string(fdt, "bootargs", kvm->cfg.real_cmdline));
>  	_FDT(fdt_property_u64(fdt, "kaslr-seed", kvm->cfg.arch.kaslr_seed));
> +	_FDT(fdt_property_string(fdt, "stdout-path", "/U6_16550A@3f8"));

Since the last string here has to match the one in hw/serial.c, I think we
should be retrieving it from there rather than hardcoding it here.

Will

Re: [PATCH kvmtool 2/6] arm: fdt: add stdout-path to /chosen node

[Andre Przywara]

On Wed, 30 Jan 2019 18:20:19 +0000
Will Deacon <will.deacon@arm.com> wrote:

Hi,

> On Fri, Jan 25, 2019 at 06:07:57PM +0000, Andre Przywara wrote:
> > The DT spec describes the stdout-path property in the /chosen node
> > to contain the DT path for a default device usable for outputting
> > characters. The Linux kernel uses this for earlycon (without
> > further parameters), other DT users might rely on this as well.
> > 
> > Add a property containing the path to our emulated 8250 serial
> > device.
> > 
> > Even when we use the virtio console, the serial console is still
> > there and works, so we can expose this unconditionally. Putting the
> > virtio console path in there will not work anyway.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  arm/fdt.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arm/fdt.c b/arm/fdt.c
> > index 28ba1c2c..8cda3ded 100644
> > --- a/arm/fdt.c
> > +++ b/arm/fdt.c
> > @@ -143,6 +143,7 @@ static int setup_fdt(struct kvm *kvm)
> >  	_FDT(fdt_property_cell(fdt, "linux,pci-probe-only", 1));
> >  	_FDT(fdt_property_string(fdt, "bootargs",
> > kvm->cfg.real_cmdline)); _FDT(fdt_property_u64(fdt, "kaslr-seed",
> > kvm->cfg.arch.kaslr_seed));
> > +	_FDT(fdt_property_string(fdt, "stdout-path",
> > "/U6_16550A@3f8"));  
> 
> Since the last string here has to match the one in hw/serial.c, I
> think we should be retrieving it from there rather than hardcoding it
> here.

Are you thinking about something like setting a "char
*primary_console_path" in hw/serial.c:serial8250_generate_fdt_node(),
then using this here in arm/fdt.c?
Or shall the generate function directly set the stdout-path?

Cheers,
Andre.

Re: [PATCH kvmtool 2/6] arm: fdt: add stdout-path to /chosen node

[Will Deacon]

On Thu, Jan 31, 2019 at 02:57:11PM +0000, Andre Przywara wrote:
> On Wed, 30 Jan 2019 18:20:19 +0000
> Will Deacon <will.deacon@arm.com> wrote:
> > On Fri, Jan 25, 2019 at 06:07:57PM +0000, Andre Przywara wrote:
> > > The DT spec describes the stdout-path property in the /chosen node
> > > to contain the DT path for a default device usable for outputting
> > > characters. The Linux kernel uses this for earlycon (without
> > > further parameters), other DT users might rely on this as well.
> > > 
> > > Add a property containing the path to our emulated 8250 serial
> > > device.
> > > 
> > > Even when we use the virtio console, the serial console is still
> > > there and works, so we can expose this unconditionally. Putting the
> > > virtio console path in there will not work anyway.
> > > 
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > ---
> > >  arm/fdt.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/arm/fdt.c b/arm/fdt.c
> > > index 28ba1c2c..8cda3ded 100644
> > > --- a/arm/fdt.c
> > > +++ b/arm/fdt.c
> > > @@ -143,6 +143,7 @@ static int setup_fdt(struct kvm *kvm)
> > >  	_FDT(fdt_property_cell(fdt, "linux,pci-probe-only", 1));
> > >  	_FDT(fdt_property_string(fdt, "bootargs",
> > > kvm->cfg.real_cmdline)); _FDT(fdt_property_u64(fdt, "kaslr-seed",
> > > kvm->cfg.arch.kaslr_seed));
> > > +	_FDT(fdt_property_string(fdt, "stdout-path",
> > > "/U6_16550A@3f8"));  
> > 
> > Since the last string here has to match the one in hw/serial.c, I
> > think we should be retrieving it from there rather than hardcoding it
> > here.
> 
> Are you thinking about something like setting a "char
> *primary_console_path" in hw/serial.c:serial8250_generate_fdt_node(),
> then using this here in arm/fdt.c?
> Or shall the generate function directly set the stdout-path?

It's probably a bit dodgy doing it from the generate function, because I
think we'd then be relying on the the /chosen node being created before the
serial node (which is true, but I wouldn't like to rely on it).

So I think either have a way to register the primary console device, or a
way to squirrel the string away somewhere. Is there a variant of stdout-path
which uses a phandle instead of a string?  That might end up being cleaner
to implement.

Will

Re: [PATCH kvmtool 2/6] arm: fdt: add stdout-path to /chosen node

[Andre Przywara]

On Fri, 1 Feb 2019 06:26:58 +0000
Will Deacon <will.deacon@arm.com> wrote:

Hi,

> On Thu, Jan 31, 2019 at 02:57:11PM +0000, Andre Przywara wrote:
> > On Wed, 30 Jan 2019 18:20:19 +0000
> > Will Deacon <will.deacon@arm.com> wrote:  
> > > On Fri, Jan 25, 2019 at 06:07:57PM +0000, Andre Przywara wrote:  
> > > > The DT spec describes the stdout-path property in the /chosen
> > > > node to contain the DT path for a default device usable for
> > > > outputting characters. The Linux kernel uses this for earlycon
> > > > (without further parameters), other DT users might rely on this
> > > > as well.
> > > > 
> > > > Add a property containing the path to our emulated 8250 serial
> > > > device.
> > > > 
> > > > Even when we use the virtio console, the serial console is still
> > > > there and works, so we can expose this unconditionally. Putting
> > > > the virtio console path in there will not work anyway.
> > > > 
> > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > > ---
> > > >  arm/fdt.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/arm/fdt.c b/arm/fdt.c
> > > > index 28ba1c2c..8cda3ded 100644
> > > > --- a/arm/fdt.c
> > > > +++ b/arm/fdt.c
> > > > @@ -143,6 +143,7 @@ static int setup_fdt(struct kvm *kvm)
> > > >  	_FDT(fdt_property_cell(fdt, "linux,pci-probe-only",
> > > > 1)); _FDT(fdt_property_string(fdt, "bootargs",
> > > > kvm->cfg.real_cmdline)); _FDT(fdt_property_u64(fdt,
> > > > "kaslr-seed", kvm->cfg.arch.kaslr_seed));
> > > > +	_FDT(fdt_property_string(fdt, "stdout-path",
> > > > "/U6_16550A@3f8"));    
> > > 
> > > Since the last string here has to match the one in hw/serial.c, I
> > > think we should be retrieving it from there rather than
> > > hardcoding it here.  
> > 
> > Are you thinking about something like setting a "char
> > *primary_console_path" in
> > hw/serial.c:serial8250_generate_fdt_node(), then using this here in
> > arm/fdt.c? Or shall the generate function directly set the
> > stdout-path?  
> 
> It's probably a bit dodgy doing it from the generate function,
> because I think we'd then be relying on the the /chosen node being
> created before the serial node (which is true, but I wouldn't like to
> rely on it).

Agreed.

> So I think either have a way to register the primary console device,
> or a way to squirrel the string away somewhere. Is there a variant of
> stdout-path which uses a phandle instead of a string?  That might end
> up being cleaner to implement.

stdout-path must be a string, so a phandle will not work. However that
string can be an alias, so I now let stdout-path always be "serial0",
then add an aliases node at the end of the FDT generation, assigning
serial0 to the path of the first instantiated serial device.

I will post a v2 with that.

Cheers,
Andre.

[PATCH kvmtool 3/6] Makefile: support -s switch

[Andre Przywara]

"make -s" suppresses normal output, just shows warnings and errors.
But since we explicitly override the make output with our fancy concise
version, we miss out on this feature.

Do as the kernel does and explicitly suppress every normal output when -s
is given. This helps to spot warnings that scroll out of the terminal
window too quickly.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 Makefile | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index c4faff66..a68cfcd5 100644
--- a/Makefile
+++ b/Makefile
@@ -2,8 +2,22 @@
 # Define WERROR=0 to disable -Werror.
 #
 
+ifneq ($(filter 4.%,$(MAKE_VERSION)),)  # make-4
+ifneq ($(filter %s ,$(firstword x$(MAKEFLAGS))),)
+  silent=1
+endif
+else                                    # make-3.8x
+ifneq ($(filter s% -s%,$(MAKEFLAGS)),)
+  silent=1
+endif
+endif
+
 ifeq ($(strip $(V)),)
-	E = @echo
+	ifeq ($(silent),)
+		E = @echo
+	else
+		E = @\#
+	endif
 	Q = @
 else
 	E = @\#
-- 
2.17.1

Re: [PATCH kvmtool 3/6] Makefile: support -s switch

[Will Deacon]

On Fri, Jan 25, 2019 at 06:07:58PM +0000, Andre Przywara wrote:
> "make -s" suppresses normal output, just shows warnings and errors.
> But since we explicitly override the make output with our fancy concise
> version, we miss out on this feature.
> 
> Do as the kernel does and explicitly suppress every normal output when -s
> is given. This helps to spot warnings that scroll out of the terminal
> window too quickly.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  Makefile | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index c4faff66..a68cfcd5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2,8 +2,22 @@
>  # Define WERROR=0 to disable -Werror.
>  #
>  
> +ifneq ($(filter 4.%,$(MAKE_VERSION)),)  # make-4
> +ifneq ($(filter %s ,$(firstword x$(MAKEFLAGS))),)
> +  silent=1
> +endif
> +else                                    # make-3.8x
> +ifneq ($(filter s% -s%,$(MAKEFLAGS)),)
> +  silent=1
> +endif
> +endif

Why do we need to do this differently for different versions of Make?
afaict, the kernel manages this using findstring and filter-out.

Will

Re: [PATCH kvmtool 3/6] Makefile: support -s switch

[Andre Przywara]

On Wed, 30 Jan 2019 18:20:28 +0000
Will Deacon <will.deacon@arm.com> wrote:

Hi Will,

thanks for having a look!

> On Fri, Jan 25, 2019 at 06:07:58PM +0000, Andre Przywara wrote:
> > "make -s" suppresses normal output, just shows warnings and errors.
> > But since we explicitly override the make output with our fancy
> > concise version, we miss out on this feature.
> > 
> > Do as the kernel does and explicitly suppress every normal output
> > when -s is given. This helps to spot warnings that scroll out of
> > the terminal window too quickly.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  Makefile | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index c4faff66..a68cfcd5 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2,8 +2,22 @@
> >  # Define WERROR=0 to disable -Werror.
> >  #
> >  
> > +ifneq ($(filter 4.%,$(MAKE_VERSION)),)  # make-4
> > +ifneq ($(filter %s ,$(firstword x$(MAKEFLAGS))),)
> > +  silent=1
> > +endif
> > +else                                    # make-3.8x
> > +ifneq ($(filter s% -s%,$(MAKEFLAGS)),)
> > +  silent=1
> > +endif
> > +endif  
> 
> Why do we need to do this differently for different versions of Make?
> afaict, the kernel manages this using findstring and filter-out.

Indeed, thanks for the heads up, this was a Linux change in 2017, after
I originally made this patch.
I changed it now to the new version and checked that it also works on
make 3.8.

Cheers,
Andre.

[PATCH kvmtool 4/6] Makefile: Remove echoing of kvmtools version file

[Andre Przywara]

On every build we report the kvmtool "version" number, which isn't
meaningful at all anymore.

Remove the line from the KVMTOOLS-VERSION-GEN script to drop a
pointless message.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 util/KVMTOOLS-VERSION-GEN | 1 -
 1 file changed, 1 deletion(-)

diff --git a/util/KVMTOOLS-VERSION-GEN b/util/KVMTOOLS-VERSION-GEN
index f0dcfdea..91ee2c2f 100755
--- a/util/KVMTOOLS-VERSION-GEN
+++ b/util/KVMTOOLS-VERSION-GEN
@@ -35,6 +35,5 @@ else
 	VC=unset
 fi
 test "$VN" = "$VC" || {
-	echo >&2 "KVMTOOLS_VERSION = $VN"
 	echo "KVMTOOLS_VERSION = $VN" >$GVF
 }
-- 
2.17.1

Re: [PATCH kvmtool 4/6] Makefile: Remove echoing of kvmtools version file

[Will Deacon]

On Fri, Jan 25, 2019 at 06:07:59PM +0000, Andre Przywara wrote:
> On every build we report the kvmtool "version" number, which isn't
> meaningful at all anymore.
> 
> Remove the line from the KVMTOOLS-VERSION-GEN script to drop a
> pointless message.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  util/KVMTOOLS-VERSION-GEN | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/util/KVMTOOLS-VERSION-GEN b/util/KVMTOOLS-VERSION-GEN
> index f0dcfdea..91ee2c2f 100755
> --- a/util/KVMTOOLS-VERSION-GEN
> +++ b/util/KVMTOOLS-VERSION-GEN
> @@ -35,6 +35,5 @@ else
>  	VC=unset
>  fi
>  test "$VN" = "$VC" || {
> -	echo >&2 "KVMTOOLS_VERSION = $VN"
>  	echo "KVMTOOLS_VERSION = $VN" >$GVF

It's probably fine, but can you check this doesn't break the debian
packaging please?

Will

Re: [PATCH kvmtool 4/6] Makefile: Remove echoing of kvmtools version file

[Andre Przywara]

On Wed, 30 Jan 2019 18:20:36 +0000
Will Deacon <will.deacon@arm.com> wrote:

> On Fri, Jan 25, 2019 at 06:07:59PM +0000, Andre Przywara wrote:
> > On every build we report the kvmtool "version" number, which isn't
> > meaningful at all anymore.
> > 
> > Remove the line from the KVMTOOLS-VERSION-GEN script to drop a
> > pointless message.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> >  util/KVMTOOLS-VERSION-GEN | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/util/KVMTOOLS-VERSION-GEN b/util/KVMTOOLS-VERSION-GEN
> > index f0dcfdea..91ee2c2f 100755
> > --- a/util/KVMTOOLS-VERSION-GEN
> > +++ b/util/KVMTOOLS-VERSION-GEN
> > @@ -35,6 +35,5 @@ else
> >  	VC=unset
> >  fi
> >  test "$VN" = "$VC" || {
> > -	echo >&2 "KVMTOOLS_VERSION = $VN"
> >  	echo "KVMTOOLS_VERSION = $VN" >$GVF  
> 
> It's probably fine, but can you check this doesn't break the debian
> packaging please?

Debian uses some date-based versioning (0.20170904-1) for kvmtool. I
built it with debuild and this patch in the patches directory, that
seemed to be fine (from my rather clueless Debian packaging point of
view, at least).

Cheers,
Andre.

[PATCH kvmtool 5/6] arm: pmu: Improve PMU error reporting

[Andre Przywara]

The KVM ioctls mostly just return -1 in the error case, leaving the
actual error code in errno.

Change the output of the PMU error message to actually print this error
code instead of the generic -1.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/pmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index 69c37fae..ffd152e2 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -18,7 +18,7 @@ static int set_pmu_attr(struct kvm *kvm, int vcpu_idx,
 	if (!ret) {
 		ret = ioctl(fd, KVM_SET_DEVICE_ATTR, attr);
 		if (ret)
-			pr_err("PMU KVM_SET_DEVICE_ATTR failed (%d)\n", ret);
+			perror("PMU KVM_SET_DEVICE_ATTR failed");
 	} else {
 		pr_err("Unsupported PMU on vcpu%d\n", vcpu_idx);
 	}
-- 
2.17.1

[PATCH kvmtool 6/6] arm: Auto-detect guest GIC type

[Andre Przywara]

At the moment kvmtool always tries to instantiate a virtual GICv2 for
the guest, and fails with some scary error message if that doesn't work.
The user has then to manually specify "--irqchip=gicv3", which is not
really obvious.
With the advent of more GICv3-only machines, let's try to be more
clever and implement some auto-detection of the GIC type needed:
- We try GICv3 first. On GICv3-only hosts this will be the only working
option, so we don't loose anything. On GICv2-backwards compatible GICv3
machines GICv3 is probably the better choice these days.
- If that fails, we try GICv2.
- If that fails, we ran out out options and bail out.

We deduce the choice between "ITS vs. pure GICv3" and "GICv2m vs. GICv2" by
the presence of PCI devices, they would be the only MSI users anyway.

This algorithm is in effect is there is no explicit --irqchip parameter
on the command line. We still allow the GIC type to be set explicitly.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arm/gic.c                    | 25 +++++++++++++++++++++++++
 arm/include/arm-common/gic.h |  1 +
 2 files changed, 26 insertions(+)

diff --git a/arm/gic.c b/arm/gic.c
index abcbcc09..e6b66047 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -182,6 +182,8 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 		gic_device.type = KVM_DEV_TYPE_ARM_VGIC_V3;
 		dist_attr.attr  = KVM_VGIC_V3_ADDR_TYPE_DIST;
 		break;
+	case IRQCHIP_AUTO:
+		return -ENODEV;
 	}
 
 	err = ioctl(kvm->vm_fd, KVM_CREATE_DEVICE, &gic_device);
@@ -199,6 +201,8 @@ static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
 	case IRQCHIP_GICV3:
 		err = ioctl(gic_fd, KVM_SET_DEVICE_ATTR, &redist_attr);
 		break;
+	case IRQCHIP_AUTO:
+		return -ENODEV;
 	}
 	if (err)
 		goto out_err;
@@ -249,9 +253,30 @@ static int gic__create_irqchip(struct kvm *kvm)
 
 int gic__create(struct kvm *kvm, enum irqchip_type type)
 {
+	enum irqchip_type try;
+	bool needs_msis;
 	int err;
 
 	switch (type) {
+	case IRQCHIP_AUTO:
+		needs_msis = kvm->cfg.arch.virtio_trans_pci;
+		if (needs_msis)
+			try = IRQCHIP_GICV3_ITS;
+		else
+			try = IRQCHIP_GICV3;
+		err = gic__create(kvm, try);
+		if (!err) {
+			kvm->cfg.arch.irqchip = try;
+			return 0;
+		}
+		if (needs_msis)
+			try = IRQCHIP_GICV2M;
+		else
+			try = IRQCHIP_GICV2;
+		err = gic__create(kvm, try);
+		if (!err)
+			kvm->cfg.arch.irqchip = try;
+		return err;
 	case IRQCHIP_GICV2M:
 		gic_msi_size = KVM_VGIC_V2M_SIZE;
 		gic_msi_base = ARM_GIC_CPUI_BASE - gic_msi_size;
diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
index 1125d601..ec9cf31a 100644
--- a/arm/include/arm-common/gic.h
+++ b/arm/include/arm-common/gic.h
@@ -24,6 +24,7 @@
 #define KVM_VGIC_V2M_SIZE		0x1000
 
 enum irqchip_type {
+	IRQCHIP_AUTO,
 	IRQCHIP_GICV2,
 	IRQCHIP_GICV2M,
 	IRQCHIP_GICV3,
-- 
2.17.1

Re: [PATCH kvmtool 6/6] arm: Auto-detect guest GIC type

[Will Deacon]

On Fri, Jan 25, 2019 at 06:08:01PM +0000, Andre Przywara wrote:
> At the moment kvmtool always tries to instantiate a virtual GICv2 for
> the guest, and fails with some scary error message if that doesn't work.
> The user has then to manually specify "--irqchip=gicv3", which is not
> really obvious.
> With the advent of more GICv3-only machines, let's try to be more
> clever and implement some auto-detection of the GIC type needed:
> - We try GICv3 first. On GICv3-only hosts this will be the only working
> option, so we don't loose anything. On GICv2-backwards compatible GICv3
> machines GICv3 is probably the better choice these days.

Could you elaborate on "probably the better choice" please?

> - If that fails, we try GICv2.
> - If that fails, we ran out out options and bail out.
> 
> We deduce the choice between "ITS vs. pure GICv3" and "GICv2m vs. GICv2" by
> the presence of PCI devices, they would be the only MSI users anyway.

This feels really flakey. Why don't we just try, in order:

  v3 + ITS
  v3
  v2m
  v2

regardless of the VM configuration?

Will

Re: [PATCH kvmtool 6/6] arm: Auto-detect guest GIC type

[Andre Przywara]

On Wed, 30 Jan 2019 18:20:46 +0000
Will Deacon <will.deacon@arm.com> wrote:

> On Fri, Jan 25, 2019 at 06:08:01PM +0000, Andre Przywara wrote:
> > At the moment kvmtool always tries to instantiate a virtual GICv2
> > for the guest, and fails with some scary error message if that
> > doesn't work. The user has then to manually specify
> > "--irqchip=gicv3", which is not really obvious.
> > With the advent of more GICv3-only machines, let's try to be more
> > clever and implement some auto-detection of the GIC type needed:
> > - We try GICv3 first. On GICv3-only hosts this will be the only
> > working option, so we don't loose anything. On GICv2-backwards
> > compatible GICv3 machines GICv3 is probably the better choice these
> > days.  
> 
> Could you elaborate on "probably the better choice" please?

When we introduced the GICv3 emulation and the kvmtool support,
it was deemed less mature than the GICv2 support. This was one of the
reasons we were happy with GICv2-on-v3 as the default. Now with GICv3
support being stable, we should use the advantages like the system
register interface.

> > - If that fails, we try GICv2.
> > - If that fails, we ran out out options and bail out.
> > 
> > We deduce the choice between "ITS vs. pure GICv3" and "GICv2m vs.
> > GICv2" by the presence of PCI devices, they would be the only MSI
> > users anyway.  
> 
> This feels really flakey. Why don't we just try, in order:
> 
>   v3 + ITS
>   v3
>   v2m
>   v2

I think v2m will fail only rarely (doesn't rely on any kernel
support), so in practise we will likely never get a "pure" GICv2. But
since we always have the --irqchip option to override this, that's
probably OK.

Will change it to that effect.

Cheers,
Andre.

Re: [PATCH kvmtool 0/6] Various convenience fixes

[Will Deacon]

On Fri, Jan 25, 2019 at 06:07:55PM +0000, Andre Przywara wrote:
> As I found myself using kvmtool more often in the last weeks, I got more
> and more annoyed by some smaller "ticks" that kvmtool shows.
> 
> So this is an attempt to post various smaller fixes I gathered up over
> the years, but never found worth enough to send out:
> 
> - drop unnecessarily detailed debug output
> - add /chosen/stdout-path node in .dtb
> - honour make -s switch
> - remove pointless kvmtool version number
> - improve meaningless PMU error message
> - introduce autodetection of supported GIC type

I've applied (1) and (5), leaving comments on the others.

Cheers,

Will