[PATCH kvmtool 0/4] Makefile and virtio fixes

[PATCH kvmtool 0/4] Makefile and virtio fixes

[Jean-Philippe Brucker]

A few small fixes for kvmtool:

Patch 1 fixes an annoying issue when building kvmtool after updating
without a make clean.

Patch 2 enables passing ARCH=i386 and ARCH=x86_64.

Patch 3 fixes an issue with INTx on modern virtio-pci. That code was
written when INTx were edge-triggered, but they are now level-triggered.

Patch 4 fixes an uninitialized allocation error.

Jean-Philippe Brucker (4):
  Makefile: Add missing build dependencies
  Makefile: Fix ARCH override
  virtio/pci: Deassert IRQ line on ISR read
  virtio/rng: Zero-initialize the device

 Makefile            | 7 ++++---
 virtio/pci-modern.c | 5 +----
 virtio/rng.c        | 2 +-
 3 files changed, 6 insertions(+), 8 deletions(-)

-- 
2.37.1

[PATCH kvmtool 1/4] Makefile: Add missing build dependencies

[Jean-Philippe Brucker]

When running kvmtool after updating without doing a make clean, one
might run into strange issues such as:

  Warning: Failed init: symbol_init
  Fatal: Initialisation failed

or worse. This happens because symbol.o is not automatically rebuilt
after a change of headers, because .symbol.o.d is not in the $(DEPS)
variable. So if the layout of struct kvm_config changes, for example,
symbols.o that was built for an older version will try to read
kvm->vmlinux from the wrong location in struct kvm, and lkvm will die.

Add all .d files to $(DEPS). Also include $(STATIC_DEPS) which was
previously set but not used.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 1f9903d8..f0df76f4 100644
--- a/Makefile
+++ b/Makefile
@@ -383,7 +383,7 @@ comma = ,
 # The dependency file for the current target
 depfile = $(subst $(comma),_,$(dir $@).$(notdir $@).d)
 
-DEPS	:= $(foreach obj,$(OBJS),\
+DEPS	:= $(foreach obj,$(OBJS) $(OBJS_DYNOPT) $(OTHEROBJS) $(GUEST_OBJS),\
 		$(subst $(comma),_,$(dir $(obj)).$(notdir $(obj)).d))
 
 DEFINES	+= -D_FILE_OFFSET_BITS=64
@@ -590,6 +590,7 @@ cscope:
 # Escape redundant work on cleaning up
 ifneq ($(MAKECMDGOALS),clean)
 -include $(DEPS)
+-include $(STATIC_DEPS)
 
 KVMTOOLS-VERSION-FILE:
 	@$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
-- 
2.37.1

Re: [PATCH kvmtool 1/4] Makefile: Add missing build dependencies

[Alexandru Elisei]

Hi,

On Fri, Jul 22, 2022 at 03:17:29PM +0100, Jean-Philippe Brucker wrote:
> When running kvmtool after updating without doing a make clean, one
> might run into strange issues such as:
> 
>   Warning: Failed init: symbol_init
>   Fatal: Initialisation failed
> 
> or worse. This happens because symbol.o is not automatically rebuilt
> after a change of headers, because .symbol.o.d is not in the $(DEPS)
> variable. So if the layout of struct kvm_config changes, for example,
> symbols.o that was built for an older version will try to read
> kvm->vmlinux from the wrong location in struct kvm, and lkvm will die.
> 
> Add all .d files to $(DEPS). Also include $(STATIC_DEPS) which was
> previously set but not used.

This makes sense to me. And hopefully it should fix some weird errors that
went away for me by doing a make clean before a rebuild.

> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 1f9903d8..f0df76f4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -383,7 +383,7 @@ comma = ,
>  # The dependency file for the current target
>  depfile = $(subst $(comma),_,$(dir $@).$(notdir $@).d)
>  
> -DEPS	:= $(foreach obj,$(OBJS),\
> +DEPS	:= $(foreach obj,$(OBJS) $(OBJS_DYNOPT) $(OTHEROBJS) $(GUEST_OBJS),\

Checked and those are indeed all the objects.

>  		$(subst $(comma),_,$(dir $(obj)).$(notdir $(obj)).d))
>  
>  DEFINES	+= -D_FILE_OFFSET_BITS=64
> @@ -590,6 +590,7 @@ cscope:
>  # Escape redundant work on cleaning up
>  ifneq ($(MAKECMDGOALS),clean)
>  -include $(DEPS)
> +-include $(STATIC_DEPS)

In the spirit in keeping the makefile as small as possible and reading
fewer files, maybe STATIC_DEPS should be included only if the target is
$(PROGRAM)-static.

Regardless of how you want to handle that, the patch looks correct to me:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,
Alex

>  
>  KVMTOOLS-VERSION-FILE:
>  	@$(SHELL_PATH) util/KVMTOOLS-VERSION-GEN $(OUTPUT)
> -- 
> 2.37.1
> 

Re: [PATCH kvmtool 1/4] Makefile: Add missing build dependencies

[Jean-Philippe Brucker]

On Mon, Jul 25, 2022 at 11:26:15AM +0100, Alexandru Elisei wrote:
> > @@ -590,6 +590,7 @@ cscope:
> >  # Escape redundant work on cleaning up
> >  ifneq ($(MAKECMDGOALS),clean)
> >  -include $(DEPS)
> > +-include $(STATIC_DEPS)
> 
> In the spirit in keeping the makefile as small as possible and reading
> fewer files, maybe STATIC_DEPS should be included only if the target is
> $(PROGRAM)-static.

Right this adds a measurable overhead, about 2-5% on a make invocation
without anything to do. It's not noticeable since the feature tests still
take most of the time, but optimizing it may be worthwhile

Thanks,
Jean

[PATCH kvmtool 2/4] Makefile: Fix ARCH override

[Jean-Philippe Brucker]

Variables set on the command-line are not overridden by normal
assignments. So when passing ARCH=x86_64 on the command-line, build
fails:

Makefile:227: *** This architecture (x86_64) is not supported in kvmtool.

Use the 'override' directive to force the ARCH reassignment.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index f0df76f4..faae0da2 100644
--- a/Makefile
+++ b/Makefile
@@ -115,11 +115,11 @@ ARCH ?= $(shell uname -m | sed -e s/i.86/i386/ -e s/ppc.*/powerpc/ \
 	  -e s/riscv64/riscv/ -e s/riscv32/riscv/)
 
 ifeq ($(ARCH),i386)
-	ARCH         := x86
+	override ARCH = x86
 	DEFINES      += -DCONFIG_X86_32
 endif
 ifeq ($(ARCH),x86_64)
-	ARCH         := x86
+	override ARCH = x86
 	DEFINES      += -DCONFIG_X86_64
 	ARCH_PRE_INIT = x86/init.S
 endif
-- 
2.37.1

Re: [PATCH kvmtool 2/4] Makefile: Fix ARCH override

[Alexandru Elisei]

Hi,

Thank you for fixing this, I've come across it several times in the past.

On Fri, Jul 22, 2022 at 03:17:30PM +0100, Jean-Philippe Brucker wrote:
> Variables set on the command-line are not overridden by normal
> assignments. So when passing ARCH=x86_64 on the command-line, build
> fails:
> 
> Makefile:227: *** This architecture (x86_64) is not supported in kvmtool.
> 
> Use the 'override' directive to force the ARCH reassignment.
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index f0df76f4..faae0da2 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -115,11 +115,11 @@ ARCH ?= $(shell uname -m | sed -e s/i.86/i386/ -e s/ppc.*/powerpc/ \
>  	  -e s/riscv64/riscv/ -e s/riscv32/riscv/)
>  
>  ifeq ($(ARCH),i386)

As far as I know, there are several versions of the x86 architecture: i386,
i486, i586 and i686.

It looks rather arbitrary to have i386 as a valid ARCH value, but not the other
ix86 versions. I wonder if we should just drop it and keep only x86 and x86_64,
like the kernel.

Regardless, the patch looks good to me:

Tested-by: Alexandru Elisei <alexandru.elisei@arm.com>
Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,
Alex

> -	ARCH         := x86
> +	override ARCH = x86
>  	DEFINES      += -DCONFIG_X86_32
>  endif
>  ifeq ($(ARCH),x86_64)
> -	ARCH         := x86
> +	override ARCH = x86
>  	DEFINES      += -DCONFIG_X86_64
>  	ARCH_PRE_INIT = x86/init.S
>  endif
> -- 
> 2.37.1
> 

Re: [PATCH kvmtool 2/4] Makefile: Fix ARCH override

[Jean-Philippe Brucker]

On Mon, Jul 25, 2022 at 12:06:38PM +0100, Alexandru Elisei wrote:
> Hi,
> 
> Thank you for fixing this, I've come across it several times in the past.
> 
> On Fri, Jul 22, 2022 at 03:17:30PM +0100, Jean-Philippe Brucker wrote:
> > Variables set on the command-line are not overridden by normal
> > assignments. So when passing ARCH=x86_64 on the command-line, build
> > fails:
> > 
> > Makefile:227: *** This architecture (x86_64) is not supported in kvmtool.
> > 
> > Use the 'override' directive to force the ARCH reassignment.
> > 
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> >  Makefile | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index f0df76f4..faae0da2 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -115,11 +115,11 @@ ARCH ?= $(shell uname -m | sed -e s/i.86/i386/ -e s/ppc.*/powerpc/ \
> >  	  -e s/riscv64/riscv/ -e s/riscv32/riscv/)
> >  
> >  ifeq ($(ARCH),i386)
> 
> As far as I know, there are several versions of the x86 architecture: i386,
> i486, i586 and i686.

The discovery from "uname -m" does "sed -e s/i.86/i386/" to catch those

> It looks rather arbitrary to have i386 as a valid ARCH value, but not the other
> ix86 versions. I wonder if we should just drop it and keep only x86 and x86_64,
> like the kernel.

"i386" does seem to be the conventional way of selecting 32-bit x86 in the
kernel, since it has a i386_defconfig selected by ARCH=i386.

> 
> Regardless, the patch looks good to me:
> 
> Tested-by: Alexandru Elisei <alexandru.elisei@arm.com>
> Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks!
Jean

> 
> Thanks,
> Alex
> 
> > -	ARCH         := x86
> > +	override ARCH = x86
> >  	DEFINES      += -DCONFIG_X86_32
> >  endif
> >  ifeq ($(ARCH),x86_64)
> > -	ARCH         := x86
> > +	override ARCH = x86
> >  	DEFINES      += -DCONFIG_X86_64
> >  	ARCH_PRE_INIT = x86/init.S
> >  endif
> > -- 
> > 2.37.1
> > 

Re: [PATCH kvmtool 2/4] Makefile: Fix ARCH override

[Alexandru Elisei]

Hi,

On Tue, Aug 09, 2022 at 10:56:50AM +0100, Jean-Philippe Brucker wrote:
> On Mon, Jul 25, 2022 at 12:06:38PM +0100, Alexandru Elisei wrote:
> > Hi,
> > 
> > Thank you for fixing this, I've come across it several times in the past.
> > 
> > On Fri, Jul 22, 2022 at 03:17:30PM +0100, Jean-Philippe Brucker wrote:
> > > Variables set on the command-line are not overridden by normal
> > > assignments. So when passing ARCH=x86_64 on the command-line, build
> > > fails:
> > > 
> > > Makefile:227: *** This architecture (x86_64) is not supported in kvmtool.
> > > 
> > > Use the 'override' directive to force the ARCH reassignment.
> > > 
> > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > > ---
> > >  Makefile | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/Makefile b/Makefile
> > > index f0df76f4..faae0da2 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -115,11 +115,11 @@ ARCH ?= $(shell uname -m | sed -e s/i.86/i386/ -e s/ppc.*/powerpc/ \
> > >  	  -e s/riscv64/riscv/ -e s/riscv32/riscv/)
> > >  
> > >  ifeq ($(ARCH),i386)
> > 
> > As far as I know, there are several versions of the x86 architecture: i386,
> > i486, i586 and i686.
> 
> The discovery from "uname -m" does "sed -e s/i.86/i386/" to catch those

I was referring when the user sets ARCH to something other than i386:

$ make ARCH=i486 clean
Makefile:233: *** This architecture (i486) is not supported in kvmtool.  Stop.

Looks like that's because ARCH is assigned to the output of that uname -m
oneliner with the ?= operator, which according to gnu make: "This is called
a conditional variable assignment operator, because it only has an effect
if the variable is not yet defined".

But yeah, I checked for the kernel and only i386 works (never compiled the
kernel for something other than x86_64), so I guess it's OK to leave
kvmtool's Makefile as it is.

Thanks,
Alex

> 
> > It looks rather arbitrary to have i386 as a valid ARCH value, but not the other
> > ix86 versions. I wonder if we should just drop it and keep only x86 and x86_64,
> > like the kernel.
> 
> "i386" does seem to be the conventional way of selecting 32-bit x86 in the
> kernel, since it has a i386_defconfig selected by ARCH=i386.
> 
> > 
> > Regardless, the patch looks good to me:
> > 
> > Tested-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
> 
> Thanks!
> Jean
> 
> > 
> > Thanks,
> > Alex
> > 
> > > -	ARCH         := x86
> > > +	override ARCH = x86
> > >  	DEFINES      += -DCONFIG_X86_32
> > >  endif
> > >  ifeq ($(ARCH),x86_64)
> > > -	ARCH         := x86
> > > +	override ARCH = x86
> > >  	DEFINES      += -DCONFIG_X86_64
> > >  	ARCH_PRE_INIT = x86/init.S
> > >  endif
> > > -- 
> > > 2.37.1
> > > 

[PATCH kvmtool 3/4] virtio/pci: Deassert IRQ line on ISR read

[Jean-Philippe Brucker]

Since commit 2108c86d0623 ("virtio/pci: Signal INTx interrupts as level
instead of edge"), virtio uses level-triggered IRQs. Bring the modern
device up to date, by deasserting the IRQ line when the guest reads the
interrupt status register.

Fixes: 3bf79498e6d5 ("virtio: Add support for modern virtio-pci")
Reported-by: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 virtio/pci-modern.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/virtio/pci-modern.c b/virtio/pci-modern.c
index 753e95bd..c5b4bc50 100644
--- a/virtio/pci-modern.c
+++ b/virtio/pci-modern.c
@@ -245,10 +245,7 @@ static bool virtio_pci__isr_read(struct virtio_device *vdev,
 		return false;
 
 	ioport__write8(data, vpci->isr);
-	/*
-	 * Interrupts are edge triggered (yes, going against the PCI and virtio
-	 * specs), so no need to deassert the IRQ line.
-	 */
+	kvm__irq_line(vpci->kvm, vpci->legacy_irq_line, VIRTIO_IRQ_LOW);
 	vpci->isr = 0;
 
 	return 0;
-- 
2.37.1

[PATCH kvmtool 4/4] virtio/rng: Zero-initialize the device

[Jean-Philippe Brucker]

Use calloc() to avoid uninitialized fields in the rng device.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 virtio/rng.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virtio/rng.c b/virtio/rng.c
index f9d607f6..63ab8fce 100644
--- a/virtio/rng.c
+++ b/virtio/rng.c
@@ -162,7 +162,7 @@ int virtio_rng__init(struct kvm *kvm)
 	if (!kvm->cfg.virtio_rng)
 		return 0;
 
-	rdev = malloc(sizeof(*rdev));
+	rdev = calloc(1, sizeof(*rdev));
 	if (rdev == NULL)
 		return -ENOMEM;
 
-- 
2.37.1

Re: [PATCH kvmtool 0/4] Makefile and virtio fixes

[Will Deacon]

On Fri, 22 Jul 2022 15:17:28 +0100, Jean-Philippe Brucker wrote:
> A few small fixes for kvmtool:
> 
> Patch 1 fixes an annoying issue when building kvmtool after updating
> without a make clean.
> 
> Patch 2 enables passing ARCH=i386 and ARCH=x86_64.
> 
> [...]

I wasn't sure whether you were going to respin patch 2 based on Alexandru's
comment, but I actually ran into the legacy IRQ issue so I went ahead and
applied the series. I can, of course, queue extra stuff on top if you like!

Applied to kvmtool (master), thanks!

[1/4] Makefile: Add missing build dependencies
      https://git.kernel.org/will/kvmtool/c/3863f34bd767
[2/4] Makefile: Fix ARCH override
      https://git.kernel.org/will/kvmtool/c/ae22ac7a81e5
[3/4] virtio/pci: Deassert IRQ line on ISR read
      https://git.kernel.org/will/kvmtool/c/fe2182731b72
[4/4] virtio/rng: Zero-initialize the device
      https://git.kernel.org/will/kvmtool/c/6c88c26f701f

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

Re: [PATCH kvmtool 0/4] Makefile and virtio fixes

[Jean-Philippe Brucker]

On Thu, Aug 04, 2022 at 04:02:27PM +0100, Will Deacon wrote:
> On Fri, 22 Jul 2022 15:17:28 +0100, Jean-Philippe Brucker wrote:
> > A few small fixes for kvmtool:
> > 
> > Patch 1 fixes an annoying issue when building kvmtool after updating
> > without a make clean.
> > 
> > Patch 2 enables passing ARCH=i386 and ARCH=x86_64.
> > 
> > [...]
> 
> I wasn't sure whether you were going to respin patch 2 based on Alexandru's
> comment, but I actually ran into the legacy IRQ issue so I went ahead and
> applied the series. I can, of course, queue extra stuff on top if you like!

Right I was away, thanks for picking those up. I don't think patch 2 needs
any change but I may send an optimization for patch 1

Thanks,
Jean