Re: [PATCH] selftests: KVM: Handle compiler optimizations in ucall

2022-06-16 Thread oliver . upton
June 16, 2022 11:48 AM, "David Laight"  wrote:
> No wonder I was confused.
> It's not surprising the compiler optimises it all away.
> 
> It doesn't seem right to be 'abusing' WRITE_ONCE() here.
> Just adding barrier() should be enough and much more descriptive.

I had the same thought, although I do not believe barrier() is sufficient
on its own. barrier_data() with a pointer to uc passed through
is required to keep clang from eliminating the dead store.

--
Thanks,
Oliver
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH] KVM: arm64: Prevent kmemleak from accessing pKVM memory

2022-06-16 Thread Catalin Marinas
On Thu, Jun 16, 2022 at 04:11:34PM +, Quentin Perret wrote:
> Commit a7259df76702 ("memblock: make memblock_find_in_range method
> private") changed the API using which memory is reserved for the pKVM
> hypervisor. However, it seems that memblock_phys_alloc() differs
> from the original API in terms of kmemleak semantics -- the old one
> excluded the reserved regions from kmemleak scans when the new one
> doesn't seem to. Unfortunately, when protected KVM is enabled, all
> kernel accesses to pKVM-private memory result in a fatal exception,
> which can now happen because of kmemleak scans:
> 
> $ echo scan > /sys/kernel/debug/kmemleak
> [   34.991354] kvm [304]: nVHE hyp BUG at: [] 
> __kvm_nvhe_handle_host_mem_abort+0x270/0x290!
> [   34.991580] kvm [304]: Hyp Offset: 0xfffe8be807e0
> [   34.991813] Kernel panic - not syncing: HYP panic:
> [   34.991813] PS:63c9 PC:f418011a3750 ESR:f2000800
> [   34.991813] FAR:00043920 HPFAR:04792000 
> PAR:
> [   34.991813] VCPU:
> [   34.993660] CPU: 0 PID: 304 Comm: bash Not tainted 5.19.0-rc2 #102
> [   34.994059] Hardware name: linux,dummy-virt (DT)
> [   34.994452] Call trace:
> [   34.994641]  dump_backtrace.part.0+0xcc/0xe0
> [   34.994932]  show_stack+0x18/0x6c
> [   34.995094]  dump_stack_lvl+0x68/0x84
> [   34.995276]  dump_stack+0x18/0x34
> [   34.995484]  panic+0x16c/0x354
> [   34.995673]  __hyp_pgtable_total_pages+0x0/0x60
> [   34.995933]  scan_block+0x74/0x12c
> [   34.996129]  scan_gray_list+0xd8/0x19c
> [   34.996332]  kmemleak_scan+0x2c8/0x580
> [   34.996535]  kmemleak_write+0x340/0x4a0
> [   34.996744]  full_proxy_write+0x60/0xbc
> [   34.996967]  vfs_write+0xc4/0x2b0
> [   34.997136]  ksys_write+0x68/0xf4
> [   34.997311]  __arm64_sys_write+0x20/0x2c
> [   34.997532]  invoke_syscall+0x48/0x114
> [   34.997779]  el0_svc_common.constprop.0+0x44/0xec
> [   34.998029]  do_el0_svc+0x2c/0xc0
> [   34.998205]  el0_svc+0x2c/0x84
> [   34.998421]  el0t_64_sync_handler+0xf4/0x100
> [   34.998653]  el0t_64_sync+0x18c/0x190
> [   34.999252] SMP: stopping secondary CPUs
> [   35.34] Kernel Offset: disabled
> [   35.000261] CPU features: 0x800,7831,1086
> [   35.000642] Memory Limit: none
> [   35.001329] ---[ end Kernel panic - not syncing: HYP panic:
> [   35.001329] PS:63c9 PC:f418011a3750 ESR:f2000800
> [   35.001329] FAR:00043920 HPFAR:04792000 
> PAR:
> [   35.001329] VCPU: ]---
> 
> Fix this by explicitly excluding the hypervisor's memory pool from
> kmemleak like we already do for the hyp BSS.
> 
> Cc: Mike Rapoport 
> Fixes: a7259df76702 ("memblock: make memblock_find_in_range method private")
> Signed-off-by: Quentin Perret 
> ---
> An alternative could be to actually exclude memory allocated using
> memblock_phys_alloc_range() from kmemleak scans to revert back to the
> old behaviour. But nobody else has complained about this AFAIK, so I'd
> be inclined to keep this local to pKVM. No strong opinion.

This works for me, I haven't heard anyone else complaining.

Acked-by: Catalin Marinas 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v4 kvmtool 08/12] builtin_run: Allow standard size specifiers for memory

2022-06-16 Thread Andre Przywara
On Thu, 16 Jun 2022 14:48:24 +0100
Alexandru Elisei  wrote:

> From: Suzuki K Poulose 
> 
> Allow the user to use the standard B (bytes), K (kilobytes), M (megabytes),
> G (gigabytes), T (terabytes) and P (petabytes) suffixes for memory size.
> When none are specified, the default is megabytes.
> 
> Also raise an error if the guest specifies 0 as the memory size, instead
> of treating it as uninitialized, as kvmtool has done so far.
> 
> Signed-off-by: Suzuki K Poulose 
> Signed-off-by: Alexandru Elisei 

Thanks, looks good now!

Reviewed-by: Andre Przywara 

Cheers,
Andre

> ---
>  builtin-run.c | 59 ++-
>  1 file changed, 54 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin-run.c b/builtin-run.c
> index dcd08f739469..8b4e865f0a0e 100644
> --- a/builtin-run.c
> +++ b/builtin-run.c
> @@ -49,9 +49,11 @@
>  #include 
>  #include 
>  
> -#define MB_SHIFT (20)
>  #define KB_SHIFT (10)
> +#define MB_SHIFT (20)
>  #define GB_SHIFT (30)
> +#define TB_SHIFT (40)
> +#define PB_SHIFT (50)
>  
>  __thread struct kvm_cpu *current_kvm_cpu;
>  
> @@ -87,6 +89,54 @@ void kvm_run_set_wrapper_sandbox(void)
>   kvm_run_wrapper = KVM_RUN_SANDBOX;
>  }
>  
> +static int parse_mem_unit(char **next)
> +{
> + switch (**next) {
> + case 'B': case 'b': (*next)++; return 0;
> + case 'K': case 'k': (*next)++; return KB_SHIFT;
> + case 'M': case 'm': (*next)++; return MB_SHIFT;
> + case 'G': case 'g': (*next)++; return GB_SHIFT;
> + case 'T': case 't': (*next)++; return TB_SHIFT;
> + case 'P': case 'p': (*next)++; return PB_SHIFT;
> + }
> +
> + return MB_SHIFT;
> +}
> +
> +static u64 parse_mem_option(const char *nptr, char **next)
> +{
> + u64 shift;
> + u64 val;
> +
> + errno = 0;
> + val = strtoull(nptr, next, 10);
> + if (errno == ERANGE)
> + die("Memory too large: %s", nptr);
> + if (*next == nptr)
> + die("Invalid memory specifier: %s", nptr);
> +
> + shift = parse_mem_unit(next);
> + if ((val << shift) < val)
> + die("Memory too large: %s", nptr);
> +
> + return val << shift;
> +}
> +
> +static int mem_parser(const struct option *opt, const char *arg, int unset)
> +{
> + struct kvm *kvm = opt->ptr;
> + char *next;
> +
> + kvm->cfg.ram_size = parse_mem_option(arg, &next);
> + if (kvm->cfg.ram_size == 0)
> + die("Invalid RAM size: %s", arg);
> +
> + if (*next != '\0')
> + die("Invalid memory specifier: %s", arg);
> +
> + return 0;
> +}
> +
>  #ifndef OPT_ARCH_RUN
>  #define OPT_ARCH_RUN(...)
>  #endif
> @@ -97,8 +147,9 @@ void kvm_run_set_wrapper_sandbox(void)
>   OPT_STRING('\0', "name", &(cfg)->guest_name, "guest name",  \
>   "A name for the guest"),\
>   OPT_INTEGER('c', "cpus", &(cfg)->nrcpus, "Number of CPUs"), \
> - OPT_U64('m', "mem", &(cfg)->ram_size, "Virtual machine memory"  \
> - " size in MB."),\
> + OPT_CALLBACK('m', "mem", NULL, "size[BKMGTP]",  \
> +  "Virtual machine memory size, by default measured" \
> +  " in megabytes (M)", mem_parser, kvm), \
>   OPT_CALLBACK('d', "disk", kvm, "image or rootfs_dir", "Disk "   \
>   " image or rootfs directory", img_name_parser,  \
>   kvm),   \
> @@ -522,8 +573,6 @@ static void kvm_run_validate_cfg(struct kvm *kvm)
>   pr_warning("Ignoring initrd file when loading a firmware 
> image");
>  
>   if (kvm->cfg.ram_size) {
> - /* User specifies RAM size in megabytes. */
> - kvm->cfg.ram_size <<= MB_SHIFT;
>   available_ram = host_ram_size();
>   if (available_ram && kvm->cfg.ram_size > available_ram) {
>   pr_warning("Guest memory size %lluMB exceeds host 
> physical RAM size %lluMB",

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v4 kvmtool 02/12] builtin-run: Always use RAM size in bytes

2022-06-16 Thread Andre Przywara
On Thu, 16 Jun 2022 14:48:18 +0100
Alexandru Elisei  wrote:

> The user can specify the virtual machine memory size in MB, which is saved
> in cfg->ram_size. kvmtool validates it against the host memory size,
> converted from bytes to MB. ram_size is then converted to bytes, and this
> is how it is used throughout the rest of kvmtool.
> 
> To avoid any confusion about the unit of measurement, especially once the
> user is allowed to specify the unit of measurement, always use ram_size in
> bytes.
> 
> Signed-off-by: Alexandru Elisei 

That looks good to me now, many thanks for the changes!
I ran a few large VMs (up to 1TB guest), also tested on an ARM32 host, it
all seems to still work fine, and no UBSAN messages.

Reviewed-by: Andre Przywara 

Cheers,
Andre


> ---
>  builtin-run.c| 19 ++-
>  include/kvm/kvm-config.h |  7 ---
>  include/kvm/kvm.h|  2 +-
>  3 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/builtin-run.c b/builtin-run.c
> index 0126c9fbcba6..2bf93fe13c92 100644
> --- a/builtin-run.c
> +++ b/builtin-run.c
> @@ -36,6 +36,7 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -264,7 +265,7 @@ static u64 host_ram_size(void)
>   return 0;
>   }
>  
> - return (nr_pages * page_size) >> MB_SHIFT;
> + return (u64)nr_pages * page_size;
>  }
>  
>  /*
> @@ -278,11 +279,11 @@ static u64 get_ram_size(int nr_cpus)
>   u64 available;
>   u64 ram_size;
>  
> - ram_size= 64 * (nr_cpus + 3);
> + ram_size= (u64)SZ_64M * (nr_cpus + 3);
>  
>   available   = host_ram_size() * RAM_SIZE_RATIO;
>   if (!available)
> - available = MIN_RAM_SIZE_MB;
> + available = MIN_RAM_SIZE;
>  
>   if (ram_size > available)
>   ram_size= available;
> @@ -595,13 +596,13 @@ static struct kvm *kvm_cmd_run_init(int argc, const 
> char **argv)
>  
>   if (!kvm->cfg.ram_size)
>   kvm->cfg.ram_size = get_ram_size(kvm->cfg.nrcpus);
> + else
> + kvm->cfg.ram_size <<= MB_SHIFT;
>  
>   if (kvm->cfg.ram_size > host_ram_size())
>   pr_warning("Guest memory size %lluMB exceeds host physical RAM 
> size %lluMB",
> - (unsigned long long)kvm->cfg.ram_size,
> - (unsigned long long)host_ram_size());
> -
> - kvm->cfg.ram_size <<= MB_SHIFT;
> + (unsigned long long)kvm->cfg.ram_size >> MB_SHIFT,
> + (unsigned long long)host_ram_size() >> MB_SHIFT);
>  
>   if (!kvm->cfg.dev)
>   kvm->cfg.dev = DEFAULT_KVM_DEV;
> @@ -676,12 +677,12 @@ static struct kvm *kvm_cmd_run_init(int argc, const 
> char **argv)
>   if (kvm->cfg.kernel_filename) {
>   printf("  # %s run -k %s -m %Lu -c %d --name %s\n", 
> KVM_BINARY_NAME,
>  kvm->cfg.kernel_filename,
> -(unsigned long long)kvm->cfg.ram_size / 1024 / 1024,
> +(unsigned long long)kvm->cfg.ram_size >> MB_SHIFT,
>  kvm->cfg.nrcpus, kvm->cfg.guest_name);
>   } else if (kvm->cfg.firmware_filename) {
>   printf("  # %s run --firmware %s -m %Lu -c %d --name %s\n", 
> KVM_BINARY_NAME,
>  kvm->cfg.firmware_filename,
> -(unsigned long long)kvm->cfg.ram_size / 1024 / 1024,
> +(unsigned long long)kvm->cfg.ram_size >> MB_SHIFT,
>  kvm->cfg.nrcpus, kvm->cfg.guest_name);
>   }
>  
> diff --git a/include/kvm/kvm-config.h b/include/kvm/kvm-config.h
> index 6a5720c4c7d4..31bc89520d52 100644
> --- a/include/kvm/kvm-config.h
> +++ b/include/kvm/kvm-config.h
> @@ -5,6 +5,8 @@
>  #include "kvm/vfio.h"
>  #include "kvm/kvm-config-arch.h"
>  
> +#include 
> +
>  #define DEFAULT_KVM_DEV  "/dev/kvm"
>  #define DEFAULT_CONSOLE  "serial"
>  #define DEFAULT_NETWORK  "user"
> @@ -15,14 +17,13 @@
>  #define DEFAULT_SCRIPT   "none"
>  #define DEFAULT_SANDBOX_FILENAME "guest/sandbox.sh"
>  
> -#define MIN_RAM_SIZE_MB  (64ULL)
> -#define MIN_RAM_SIZE_BYTE(MIN_RAM_SIZE_MB << MB_SHIFT)
> +#define MIN_RAM_SIZE SZ_64M
>  
>  struct kvm_config {
>   struct kvm_config_arch arch;
>   struct disk_image_params disk_image[MAX_DISK_IMAGES];
>   struct vfio_device_params *vfio_devices;
> - u64 ram_size;
> + u64 ram_size;   /* Guest memory size, in bytes */
>   u8 num_net_devices;
>   u8 num_vfio_devices;
>   u64 vsock_cid;
> diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
> index ad732e56f5ed..7b14b33b50ca 100644
> --- a/include/kvm/kvm.h
> +++ b/include/kvm/kvm.h
> @@ -87,7 +87,7 @@ struct kvm {
>   struct kvm_cpu  **cpus;
>  
>   u32 mem_slots;  /* for 
> KVM_SET_USER_MEMORY_REGION */
> - u64 ram_size;
> + u64 ram_size

Re: [PATCH v4 kvmtool 04/12] builtin-run: Add arch hook to validate VM configuration

2022-06-16 Thread Andre Przywara
On Thu, 16 Jun 2022 14:48:20 +0100
Alexandru Elisei  wrote:

> Architectures are free to set their own command line options. Add an
> architecture specific hook to validate these options.
> 
> For now, the hook does nothing, but it will be used in later patches.
> 
> Signed-off-by: Alexandru Elisei 

Reviewed-by: Andre Przywara 

Cheers,
Andre

> ---
>  Makefile  | 1 +
>  arm/aarch32/kvm.c | 5 +
>  arm/aarch64/kvm.c | 4 
>  builtin-run.c | 2 ++
>  include/kvm/kvm.h | 1 +
>  mips/kvm.c| 4 
>  powerpc/kvm.c | 4 
>  riscv/kvm.c   | 4 
>  x86/kvm.c | 4 
>  9 files changed, 29 insertions(+)
>  create mode 100644 arm/aarch32/kvm.c
> 
> diff --git a/Makefile b/Makefile
> index 6464446a9f24..64bb9c95b6f6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -170,6 +170,7 @@ ifeq ($(ARCH), arm)
>   OBJS+= $(OBJS_ARM_COMMON)
>   OBJS+= arm/aarch32/arm-cpu.o
>   OBJS+= arm/aarch32/kvm-cpu.o
> + OBJS+= arm/aarch32/kvm.o
>   ARCH_INCLUDE:= $(HDRS_ARM_COMMON)
>   ARCH_INCLUDE+= -Iarm/aarch32/include
>   CFLAGS  += -march=armv7-a
> diff --git a/arm/aarch32/kvm.c b/arm/aarch32/kvm.c
> new file mode 100644
> index ..ae33ac92479a
> --- /dev/null
> +++ b/arm/aarch32/kvm.c
> @@ -0,0 +1,5 @@
> +#include "kvm/kvm.h"
> +
> +void kvm__arch_validate_cfg(struct kvm *kvm)
> +{
> +}
> diff --git a/arm/aarch64/kvm.c b/arm/aarch64/kvm.c
> index f3fe854e0b3f..ca348f118a56 100644
> --- a/arm/aarch64/kvm.c
> +++ b/arm/aarch64/kvm.c
> @@ -37,6 +37,10 @@ int vcpu_affinity_parser(const struct option *opt, const 
> char *arg, int unset)
>   return 0;
>  }
>  
> +void kvm__arch_validate_cfg(struct kvm *kvm)
> +{
> +}
> +
>  /*
>   * Return the TEXT_OFFSET value that the guest kernel expects. Note
>   * that pre-3.17 kernels expose this value using the native endianness
> diff --git a/builtin-run.c b/builtin-run.c
> index e1770b3c9df2..dcd08f739469 100644
> --- a/builtin-run.c
> +++ b/builtin-run.c
> @@ -531,6 +531,8 @@ static void kvm_run_validate_cfg(struct kvm *kvm)
>   (unsigned long long)available_ram >> MB_SHIFT);
>   }
>   }
> +
> + kvm__arch_validate_cfg(kvm);
>  }
>  
>  static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
> diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
> index 7b14b33b50ca..9f7b2fb26e95 100644
> --- a/include/kvm/kvm.h
> +++ b/include/kvm/kvm.h
> @@ -187,6 +187,7 @@ int kvm__get_sock_by_instance(const char *name);
>  int kvm__enumerate_instances(int (*callback)(const char *name, int pid));
>  void kvm__remove_socket(const char *name);
>  
> +void kvm__arch_validate_cfg(struct kvm *kvm);
>  void kvm__arch_set_cmdline(char *cmdline, bool video);
>  void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 
> ram_size);
>  void kvm__arch_delete_ram(struct kvm *kvm);
> diff --git a/mips/kvm.c b/mips/kvm.c
> index e668cbbefb25..cebec5ae0178 100644
> --- a/mips/kvm.c
> +++ b/mips/kvm.c
> @@ -13,6 +13,10 @@ struct kvm_ext kvm_req_ext[] = {
>   { 0, 0 }
>  };
>  
> +void kvm__arch_validate_cfg(struct kvm *kvm)
> +{
> +}
> +
>  void kvm__arch_read_term(struct kvm *kvm)
>  {
>   virtio_console__inject_interrupt(kvm);
> diff --git a/powerpc/kvm.c b/powerpc/kvm.c
> index 702d67dca614..3215b579f5dc 100644
> --- a/powerpc/kvm.c
> +++ b/powerpc/kvm.c
> @@ -48,6 +48,10 @@ struct kvm_ext kvm_req_ext[] = {
>   { 0, 0 }
>  };
>  
> +void kvm__arch_validate_cfg(struct kvm *kvm)
> +{
> +}
> +
>  static uint32_t mfpvr(void)
>  {
>   uint32_t r;
> diff --git a/riscv/kvm.c b/riscv/kvm.c
> index 84e02779a91c..7fb496282f4c 100644
> --- a/riscv/kvm.c
> +++ b/riscv/kvm.c
> @@ -13,6 +13,10 @@ struct kvm_ext kvm_req_ext[] = {
>   { 0, 0 },
>  };
>  
> +void kvm__arch_validate_cfg(struct kvm *kvm)
> +{
> +}
> +
>  bool kvm__arch_cpu_supports_vm(void)
>  {
>   /* The KVM capability check is enough. */
> diff --git a/x86/kvm.c b/x86/kvm.c
> index 3e0f0b743f8c..6683a5c81d49 100644
> --- a/x86/kvm.c
> +++ b/x86/kvm.c
> @@ -35,6 +35,10 @@ struct kvm_ext kvm_req_ext[] = {
>   { 0, 0 }
>  };
>  
> +void kvm__arch_validate_cfg(struct kvm *kvm)
> +{
> +}
> +
>  bool kvm__arch_cpu_supports_vm(void)
>  {
>   struct cpuid_regs regs;

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH] selftests: KVM: Handle compiler optimizations in ucall

2022-06-16 Thread Andrew Jones
On Thu, Jun 16, 2022 at 03:58:52PM +, David Laight wrote:
> From: Andrew Jones
> > Sent: 16 June 2022 13:03
> > 
> > On Wed, Jun 15, 2022 at 06:57:06PM +, Raghavendra Rao Ananta wrote:
> > > The selftests, when built with newer versions of clang, is found
> > > to have over optimized guests' ucall() function, and eliminating
> > > the stores for uc.cmd (perhaps due to no immediate readers). This
> > > resulted in the userspace side always reading a value of '0', and
> > > causing multiple test failures.
> > >
> > > As a result, prevent the compiler from optimizing the stores in
> > > ucall() with WRITE_ONCE().
> > >
> > > Suggested-by: Ricardo Koller 
> > > Suggested-by: Reiji Watanabe 
> > > Signed-off-by: Raghavendra Rao Ananta 
> > > ---
> > >  tools/testing/selftests/kvm/lib/aarch64/ucall.c | 9 -
> > >  1 file changed, 4 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> > b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> > > index e0b0164e9af8..be1d9728c4ce 100644
> > > --- a/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> > > +++ b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> > > @@ -73,20 +73,19 @@ void ucall_uninit(struct kvm_vm *vm)
> > >
> > >  void ucall(uint64_t cmd, int nargs, ...)
> > >  {
> > > - struct ucall uc = {
> > > - .cmd = cmd,
> > > - };
> > > + struct ucall uc = {};
> > >   va_list va;
> > >   int i;
> > >
> > > + WRITE_ONCE(uc.cmd, cmd);
> > >   nargs = nargs <= UCALL_MAX_ARGS ? nargs : UCALL_MAX_ARGS;
> > >
> > >   va_start(va, nargs);
> > >   for (i = 0; i < nargs; ++i)
> > > - uc.args[i] = va_arg(va, uint64_t);
> > > + WRITE_ONCE(uc.args[i], va_arg(va, uint64_t));
> > >   va_end(va);
> > >
> > > - *ucall_exit_mmio_addr = (vm_vaddr_t)&uc;
> > > + WRITE_ONCE(*ucall_exit_mmio_addr, (vm_vaddr_t)&uc);
> > >  }
> 
> Am I misreading things again?
> That function looks like it writes the address of an on-stack
> item into global data.

The write to the address that the global points at causes a switch
from guest to host context. The guest's stack remains intact while
executing host code and the host can access the uc stack variable
directly by its address. Take a look at lib/aarch64/ucall.c to see
all the details.

Thanks,
drew

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


[PATCH kvmtool] arm: gic: fdt: fix PPI CPU mask calculation

2022-06-16 Thread Andre Przywara
The GICv2 DT binding describes the third cell in each interrupt
descriptor as holding the trigger type, but also the CPU mask that this
IRQ applies to, in bits [15:8]. However this is not the case for GICv3,
where we don't use a CPU mask in the third cell: a simple mask wouldn't
fit for the many more supported cores anyway.

At the moment we fill this CPU mask field regardless of the GIC type,
for the PMU and arch timer DT nodes. This is not only the wrong thing to
do in case of a GICv3, but also triggers UBSAN splats when using more
than 30 cores, as we do shifting beyond what a u32 can hold:
$ lkvm run -k Image -c 31 --pmu
arm/timer.c:13:22: runtime error: left shift of 1 by 31 places cannot be 
represented in type 'int'
arm/timer.c:13:38: runtime error: signed integer overflow: -2147483648 - 1 
cannot be represented in type 'int'
arm/timer.c:13:43: runtime error: left shift of 2147483647 by 8 places cannot 
be represented in type 'int'
arm/aarch64/pmu.c:202:22: runtime error: left shift of 1 by 31 places cannot be 
represented in type 'int'
arm/aarch64/pmu.c:202:38: runtime error: signed integer overflow: -2147483648 - 
1 cannot be represented in type 'int'
arm/aarch64/pmu.c:202:43: runtime error: left shift of 2147483647 by 8 places 
cannot be represented in type 'int'

Fix that by adding a function that creates the mask by looking at the
GIC type first, and returning zero when a GICv3 is used. Also we
explicitly check for the CPU limit again, even though this would be
done before already, when we try to create a GICv2 VM with more than 8
cores.

Signed-off-by: Andre Przywara 
---
 arm/aarch64/pmu.c|  3 +--
 arm/gic.c| 13 +
 arm/include/arm-common/gic.h |  1 +
 arm/timer.c  |  4 +---
 4 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/arm/aarch64/pmu.c b/arm/aarch64/pmu.c
index 5f189b32..5ed4979a 100644
--- a/arm/aarch64/pmu.c
+++ b/arm/aarch64/pmu.c
@@ -199,8 +199,7 @@ void pmu__generate_fdt_nodes(void *fdt, struct kvm *kvm)
int pmu_id = -ENXIO;
int i;
 
-   u32 cpu_mask = (((1 << kvm->nrcpus) - 1) << GIC_FDT_IRQ_PPI_CPU_SHIFT) \
-  & GIC_FDT_IRQ_PPI_CPU_MASK;
+   u32 cpu_mask = gic__get_fdt_irq_cpumask(kvm);
u32 irq_prop[] = {
cpu_to_fdt32(GIC_FDT_IRQ_TYPE_PPI),
cpu_to_fdt32(irq - 16),
diff --git a/arm/gic.c b/arm/gic.c
index 26be4b4c..a223a72c 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -399,6 +399,19 @@ void gic__generate_fdt_nodes(void *fdt, enum irqchip_type 
type)
_FDT(fdt_end_node(fdt));
 }
 
+u32 gic__get_fdt_irq_cpumask(struct kvm *kvm)
+{
+   /* Only for GICv2 */
+   if (kvm->cfg.arch.irqchip == IRQCHIP_GICV3 ||
+   kvm->cfg.arch.irqchip == IRQCHIP_GICV3_ITS)
+   return 0;
+
+   if (kvm->nrcpus > 8)
+   return GIC_FDT_IRQ_PPI_CPU_MASK;
+
+   return ((1U << kvm->nrcpus) - 1) << GIC_FDT_IRQ_PPI_CPU_SHIFT;
+}
+
 #define KVM_IRQCHIP_IRQ(x) (KVM_ARM_IRQ_TYPE_SPI << KVM_ARM_IRQ_TYPE_SHIFT) |\
   ((x) & KVM_ARM_IRQ_NUM_MASK)
 
diff --git a/arm/include/arm-common/gic.h b/arm/include/arm-common/gic.h
index ec9cf31a..ad8bcbf2 100644
--- a/arm/include/arm-common/gic.h
+++ b/arm/include/arm-common/gic.h
@@ -37,6 +37,7 @@ int gic__alloc_irqnum(void);
 int gic__create(struct kvm *kvm, enum irqchip_type type);
 int gic__create_gicv2m_frame(struct kvm *kvm, u64 msi_frame_addr);
 void gic__generate_fdt_nodes(void *fdt, enum irqchip_type type);
+u32 gic__get_fdt_irq_cpumask(struct kvm *kvm);
 
 int gic__add_irqfd(struct kvm *kvm, unsigned int gsi, int trigger_fd,
   int resample_fd);
diff --git a/arm/timer.c b/arm/timer.c
index 71bfe8d4..6acc50ef 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -9,9 +9,7 @@
 void timer__generate_fdt_nodes(void *fdt, struct kvm *kvm, int *irqs)
 {
const char compatible[] = "arm,armv8-timer\0arm,armv7-timer";
-
-   u32 cpu_mask = (((1 << kvm->nrcpus) - 1) << GIC_FDT_IRQ_PPI_CPU_SHIFT) \
-  & GIC_FDT_IRQ_PPI_CPU_MASK;
+   u32 cpu_mask = gic__get_fdt_irq_cpumask(kvm);
u32 irq_prop[] = {
cpu_to_fdt32(GIC_FDT_IRQ_TYPE_PPI),
cpu_to_fdt32(irqs[0]),
-- 
2.25.1

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH 1/2] KVM: selftests: kvm_vm_elf_load() and elfhdr_get() should close fd

2022-06-16 Thread Andrew Jones
Hi Paolo,

Can you pick this old patch up?

Thanks,
drew

On Wed, Feb 16, 2022 at 07:49:46PM -0800, Reiji Watanabe wrote:
> kvm_vm_elf_load() and elfhdr_get() open one file each, but they
> never close the opened file descriptor.  If a test repeatedly
> creates and destroys a VM with vm_create_with_vcpus(), which
> (directly or indirectly) calls those two functions, the test
> might end up getting a open failure with EMFILE.
> Fix those two functions to close the file descriptor.
> 
> Signed-off-by: Reiji Watanabe 
> ---
>  tools/testing/selftests/kvm/lib/elf.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/lib/elf.c 
> b/tools/testing/selftests/kvm/lib/elf.c
> index 13e8e3dcf984..9b23537a3caa 100644
> --- a/tools/testing/selftests/kvm/lib/elf.c
> +++ b/tools/testing/selftests/kvm/lib/elf.c
> @@ -91,6 +91,7 @@ static void elfhdr_get(const char *filename, Elf64_Ehdr 
> *hdrp)
>   "  hdrp->e_shentsize: %x\n"
>   "  expected: %zx",
>   hdrp->e_shentsize, sizeof(Elf64_Shdr));
> + close(fd);
>  }
>  
>  /* VM ELF Load
> @@ -190,4 +191,5 @@ void kvm_vm_elf_load(struct kvm_vm *vm, const char 
> *filename)
>   phdr.p_filesz);
>   }
>   }
> + close(fd);
>  }
> -- 
> 2.35.1.473.g83b2b277ed-goog
>

Reviewed-by: Andrew Jones 

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


[PATCH v4 kvmtool 12/12] arm64: Allow the user to specify the RAM base address

2022-06-16 Thread Alexandru Elisei
Allow the user to specify the RAM base address by using -m/--mem size@addr
command line argument. The base address must be above 2GB, as to not
overlap with the MMIO I/O region.

Reviewed-by: Andre Przywara 
Signed-off-by: Alexandru Elisei 
---
 arm/aarch64/include/kvm/kvm-arch.h |  2 ++
 arm/aarch64/kvm.c  | 14 
 arm/kvm.c  |  7 --
 builtin-run.c  | 36 ++
 include/kvm/kvm-config.h   |  1 +
 include/kvm/kvm.h  | 12 ++
 include/linux/sizes.h  |  2 ++
 7 files changed, 64 insertions(+), 10 deletions(-)

diff --git a/arm/aarch64/include/kvm/kvm-arch.h 
b/arm/aarch64/include/kvm/kvm-arch.h
index ff857ca6e7b4..02d09a413831 100644
--- a/arm/aarch64/include/kvm/kvm-arch.h
+++ b/arm/aarch64/include/kvm/kvm-arch.h
@@ -10,6 +10,8 @@ void kvm__arch_enable_mte(struct kvm *kvm);
 
 #define MAX_PAGE_SIZE  SZ_64K
 
+#define ARCH_HAS_CFG_RAM_ADDRESS   1
+
 #include "arm-common/kvm-arch.h"
 
 #endif /* KVM__KVM_ARCH_H */
diff --git a/arm/aarch64/kvm.c b/arm/aarch64/kvm.c
index 357936844046..54200c9eec9d 100644
--- a/arm/aarch64/kvm.c
+++ b/arm/aarch64/kvm.c
@@ -4,6 +4,7 @@
 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -39,10 +40,15 @@ int vcpu_affinity_parser(const struct option *opt, const 
char *arg, int unset)
 
 void kvm__arch_validate_cfg(struct kvm *kvm)
 {
+
+   if (kvm->cfg.ram_addr < ARM_MEMORY_AREA) {
+   die("RAM address is below the I/O region ending at %luGB",
+   ARM_MEMORY_AREA >> 30);
+   }
+
if (kvm->cfg.arch.aarch32_guest &&
-   kvm->cfg.ram_size > ARM_LOMAP_MAX_MEMORY) {
-   die("RAM size 0x%llx exceeds maximum allowed 0x%llx",
-   kvm->cfg.ram_size, ARM_LOMAP_MAX_MEMORY);
+   kvm->cfg.ram_addr + kvm->cfg.ram_size > SZ_4G) {
+   die("RAM extends above 4GB");
}
 }
 
@@ -117,7 +123,7 @@ int kvm__get_vm_type(struct kvm *kvm)
return 0;
 
/* Otherwise, compute the minimal required IPA size */
-   max_ipa = ARM_MEMORY_AREA + kvm->cfg.ram_size - 1;
+   max_ipa = kvm->cfg.ram_addr + kvm->cfg.ram_size - 1;
ipa_bits = max(32, fls_long(max_ipa));
pr_debug("max_ipa %lx ipa_bits %d max_ipa_bits %d",
 max_ipa, ipa_bits, max_ipa_bits);
diff --git a/arm/kvm.c b/arm/kvm.c
index abcccfabf59e..d51cc15d8b1c 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -55,7 +55,7 @@ void kvm__init_ram(struct kvm *kvm)
madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
MADV_HUGEPAGE);
 
-   phys_start  = ARM_MEMORY_AREA;
+   phys_start  = kvm->cfg.ram_addr;
phys_size   = kvm->ram_size;
host_mem= kvm->ram_start;
 
@@ -65,6 +65,9 @@ void kvm__init_ram(struct kvm *kvm)
"address 0x%llx [err %d]", phys_size, phys_start, err);
 
kvm->arch.memory_guest_start = phys_start;
+
+   pr_debug("RAM created at 0x%llx - 0x%llx",
+phys_start, phys_start + phys_size - 1);
 }
 
 void kvm__arch_delete_ram(struct kvm *kvm)
@@ -201,7 +204,7 @@ bool kvm__load_firmware(struct kvm *kvm, const char 
*firmware_filename)
 
/* For default firmware address, lets load it at the begining of RAM */
if (fw_addr == 0)
-   fw_addr = ARM_MEMORY_AREA;
+   fw_addr = kvm->arch.memory_guest_start;
 
if (!validate_fw_addr(kvm, fw_addr))
die("Bad firmware destination: 0x%016llx", fw_addr);
diff --git a/builtin-run.c b/builtin-run.c
index 8b4e865f0a0e..87023e390e73 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -125,12 +125,21 @@ static u64 parse_mem_option(const char *nptr, char **next)
 static int mem_parser(const struct option *opt, const char *arg, int unset)
 {
struct kvm *kvm = opt->ptr;
-   char *next;
+   char *next, *nptr;
 
kvm->cfg.ram_size = parse_mem_option(arg, &next);
if (kvm->cfg.ram_size == 0)
die("Invalid RAM size: %s", arg);
 
+   if (kvm__arch_has_cfg_ram_address() && *next == '@') {
+   next++;
+   if (*next == '\0')
+   die("Missing memory address: %s", arg);
+
+   nptr = next;
+   kvm->cfg.ram_addr = parse_mem_option(nptr, &next);
+   }
+
if (*next != '\0')
die("Invalid memory specifier: %s", arg);
 
@@ -141,15 +150,26 @@ static int mem_parser(const struct option *opt, const 
char *arg, int unset)
 #define OPT_ARCH_RUN(...)
 #endif
 
+#ifdef ARCH_HAS_CFG_RAM_ADDRESS
+#define MEM_OPT_HELP_SHORT "size[BKMGTP][@addr[BKMGTP]]"
+#define MEM_OPT_HELP_LONG  \
+   "Virtual machine memory size and optional base address, both"   \
+   " measured by default in megabytes (M)"
+#else
+#define MEM_OPT_HELP_SHORT "size[BKMGTP]"
+#define MEM_OPT_HELP_LONG

[PATCH v4 kvmtool 11/12] Introduce kvm__arch_default_ram_address()

2022-06-16 Thread Alexandru Elisei
Add a new function, kvm__arch_default_ram_address(), which returns the
default address for guest RAM for each architecture.

Reviewed-by: Andre Przywara 
Signed-off-by: Alexandru Elisei 
---
 arm/aarch32/kvm.c | 5 +
 arm/aarch64/kvm.c | 5 +
 include/kvm/kvm.h | 1 +
 mips/kvm.c| 5 +
 powerpc/kvm.c | 5 +
 riscv/kvm.c   | 5 +
 x86/kvm.c | 5 +
 7 files changed, 31 insertions(+)

diff --git a/arm/aarch32/kvm.c b/arm/aarch32/kvm.c
index 9d68d7a15ee2..768a56bbb5b4 100644
--- a/arm/aarch32/kvm.c
+++ b/arm/aarch32/kvm.c
@@ -7,3 +7,8 @@ void kvm__arch_validate_cfg(struct kvm *kvm)
kvm->cfg.ram_size, ARM_LOMAP_MAX_MEMORY);
}
 }
+
+u64 kvm__arch_default_ram_address(void)
+{
+   return ARM_MEMORY_AREA;
+}
diff --git a/arm/aarch64/kvm.c b/arm/aarch64/kvm.c
index 2134528bf7f2..357936844046 100644
--- a/arm/aarch64/kvm.c
+++ b/arm/aarch64/kvm.c
@@ -46,6 +46,11 @@ void kvm__arch_validate_cfg(struct kvm *kvm)
}
 }
 
+u64 kvm__arch_default_ram_address(void)
+{
+   return ARM_MEMORY_AREA;
+}
+
 /*
  * Return the TEXT_OFFSET value that the guest kernel expects. Note
  * that pre-3.17 kernels expose this value using the native endianness
diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
index 640b76c095f9..360430b78b1e 100644
--- a/include/kvm/kvm.h
+++ b/include/kvm/kvm.h
@@ -190,6 +190,7 @@ void kvm__remove_socket(const char *name);
 void kvm__arch_validate_cfg(struct kvm *kvm);
 void kvm__arch_set_cmdline(char *cmdline, bool video);
 void kvm__arch_init(struct kvm *kvm);
+u64 kvm__arch_default_ram_address(void);
 void kvm__arch_delete_ram(struct kvm *kvm);
 int kvm__arch_setup_firmware(struct kvm *kvm);
 int kvm__arch_free_firmware(struct kvm *kvm);
diff --git a/mips/kvm.c b/mips/kvm.c
index fb60b210e7fc..0faa03a93518 100644
--- a/mips/kvm.c
+++ b/mips/kvm.c
@@ -13,6 +13,11 @@ struct kvm_ext kvm_req_ext[] = {
{ 0, 0 }
 };
 
+u64 kvm__arch_default_ram_address(void)
+{
+   return 0;
+}
+
 void kvm__arch_validate_cfg(struct kvm *kvm)
 {
 }
diff --git a/powerpc/kvm.c b/powerpc/kvm.c
index d281b070fd0e..7b0d0669aff4 100644
--- a/powerpc/kvm.c
+++ b/powerpc/kvm.c
@@ -48,6 +48,11 @@ struct kvm_ext kvm_req_ext[] = {
{ 0, 0 }
 };
 
+u64 kvm__arch_default_ram_address(void)
+{
+   return 0;
+}
+
 void kvm__arch_validate_cfg(struct kvm *kvm)
 {
 }
diff --git a/riscv/kvm.c b/riscv/kvm.c
index c46660772aa0..4d6f5cb57ac8 100644
--- a/riscv/kvm.c
+++ b/riscv/kvm.c
@@ -13,6 +13,11 @@ struct kvm_ext kvm_req_ext[] = {
{ 0, 0 },
 };
 
+u64 kvm__arch_default_ram_address(void)
+{
+   return RISCV_RAM;
+}
+
 void kvm__arch_validate_cfg(struct kvm *kvm)
 {
 }
diff --git a/x86/kvm.c b/x86/kvm.c
index 24b0305a1841..328fa7500596 100644
--- a/x86/kvm.c
+++ b/x86/kvm.c
@@ -35,6 +35,11 @@ struct kvm_ext kvm_req_ext[] = {
{ 0, 0 }
 };
 
+u64 kvm__arch_default_ram_address(void)
+{
+   return 0;
+}
+
 void kvm__arch_validate_cfg(struct kvm *kvm)
 {
 }
-- 
2.36.1

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


[PATCH v4 kvmtool 10/12] arm/arm64: Consolidate RAM initialization in kvm__init_ram()

2022-06-16 Thread Alexandru Elisei
From: Julien Grall 

RAM initialization is unnecessarily split between kvm__init_ram() and
kvm__arch_init(). Move all code related to RAM initialization to
kvm__init_ram(), making the code easier to follow and to modify.

One thing to note is that the initialization order is slightly altered:
kvm__arch_enable_mte() and gic__create() are now called before mmap'ing the
guest RAM. That is perfectly fine, as they don't use the host's mapping of
the guest memory.

Reviewed-by: Andre Przywara 
Signed-off-by: Julien Grall 
Signed-off-by: Alexandru Elisei 
---
 arm/kvm.c | 52 ++--
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/arm/kvm.c b/arm/kvm.c
index bd44aa350796..abcccfabf59e 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -26,9 +26,34 @@ bool kvm__arch_cpu_supports_vm(void)
 
 void kvm__init_ram(struct kvm *kvm)
 {
-   int err;
u64 phys_start, phys_size;
void *host_mem;
+   int err;
+
+   /*
+* Allocate guest memory. We must align our buffer to 64K to
+* correlate with the maximum guest page size for virtio-mmio.
+* If using THP, then our minimal alignment becomes 2M.
+* 2M trumps 64K, so let's go with that.
+*/
+   kvm->ram_size = kvm->cfg.ram_size;
+   kvm->arch.ram_alloc_size = kvm->ram_size + SZ_2M;
+   kvm->arch.ram_alloc_start = mmap_anon_or_hugetlbfs(kvm,
+   kvm->cfg.hugetlbfs_path,
+   kvm->arch.ram_alloc_size);
+
+   if (kvm->arch.ram_alloc_start == MAP_FAILED)
+   die("Failed to map %lld bytes for guest memory (%d)",
+   kvm->arch.ram_alloc_size, errno);
+
+   kvm->ram_start = (void *)ALIGN((unsigned long)kvm->arch.ram_alloc_start,
+   SZ_2M);
+
+   madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
+   MADV_MERGEABLE);
+
+   madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
+   MADV_HUGEPAGE);
 
phys_start  = ARM_MEMORY_AREA;
phys_size   = kvm->ram_size;
@@ -59,31 +84,6 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
 
 void kvm__arch_init(struct kvm *kvm)
 {
-   /*
-* Allocate guest memory. We must align our buffer to 64K to
-* correlate with the maximum guest page size for virtio-mmio.
-* If using THP, then our minimal alignment becomes 2M.
-* 2M trumps 64K, so let's go with that.
-*/
-   kvm->ram_size = kvm->cfg.ram_size;
-   kvm->arch.ram_alloc_size = kvm->ram_size + SZ_2M;
-   kvm->arch.ram_alloc_start = mmap_anon_or_hugetlbfs(kvm,
-   kvm->cfg.hugetlbfs_path,
-   kvm->arch.ram_alloc_size);
-
-   if (kvm->arch.ram_alloc_start == MAP_FAILED)
-   die("Failed to map %lld bytes for guest memory (%d)",
-   kvm->arch.ram_alloc_size, errno);
-
-   kvm->ram_start = (void *)ALIGN((unsigned long)kvm->arch.ram_alloc_start,
-   SZ_2M);
-
-   madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
-   MADV_MERGEABLE);
-
-   madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
-   MADV_HUGEPAGE);
-
/* Create the virtual GIC. */
if (gic__create(kvm, kvm->cfg.arch.irqchip))
die("Failed to create virtual GIC");
-- 
2.36.1

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


[PATCH v4 kvmtool 09/12] kvm__arch_init: Remove hugetlbfs_path and ram_size as parameters

2022-06-16 Thread Alexandru Elisei
From: Julien Grall 

The kvm struct already contains a pointer to the configuration, which
contains both hugetlbfs_path and ram_size, so is it not necessary to pass
them as arguments to kvm__arch_init().

Reviewed-by: Andre Przywara 
Signed-off-by: Julien Grall 
Signed-off-by: Alexandru Elisei 
---
 arm/kvm.c | 7 ---
 include/kvm/kvm.h | 2 +-
 kvm.c | 2 +-
 mips/kvm.c| 7 ---
 powerpc/kvm.c | 5 +++--
 riscv/kvm.c   | 7 ---
 x86/kvm.c | 4 +++-
 7 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/arm/kvm.c b/arm/kvm.c
index af0feae495d7..bd44aa350796 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -57,7 +57,7 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
 {
 }
 
-void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
+void kvm__arch_init(struct kvm *kvm)
 {
/*
 * Allocate guest memory. We must align our buffer to 64K to
@@ -65,9 +65,10 @@ void kvm__arch_init(struct kvm *kvm, const char 
*hugetlbfs_path, u64 ram_size)
 * If using THP, then our minimal alignment becomes 2M.
 * 2M trumps 64K, so let's go with that.
 */
-   kvm->ram_size = ram_size;
+   kvm->ram_size = kvm->cfg.ram_size;
kvm->arch.ram_alloc_size = kvm->ram_size + SZ_2M;
-   kvm->arch.ram_alloc_start = mmap_anon_or_hugetlbfs(kvm, hugetlbfs_path,
+   kvm->arch.ram_alloc_start = mmap_anon_or_hugetlbfs(kvm,
+   kvm->cfg.hugetlbfs_path,
kvm->arch.ram_alloc_size);
 
if (kvm->arch.ram_alloc_start == MAP_FAILED)
diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
index 9f7b2fb26e95..640b76c095f9 100644
--- a/include/kvm/kvm.h
+++ b/include/kvm/kvm.h
@@ -189,7 +189,7 @@ void kvm__remove_socket(const char *name);
 
 void kvm__arch_validate_cfg(struct kvm *kvm);
 void kvm__arch_set_cmdline(char *cmdline, bool video);
-void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size);
+void kvm__arch_init(struct kvm *kvm);
 void kvm__arch_delete_ram(struct kvm *kvm);
 int kvm__arch_setup_firmware(struct kvm *kvm);
 int kvm__arch_free_firmware(struct kvm *kvm);
diff --git a/kvm.c b/kvm.c
index 952ef1fbb41c..42b881217df6 100644
--- a/kvm.c
+++ b/kvm.c
@@ -479,7 +479,7 @@ int kvm__init(struct kvm *kvm)
goto err_vm_fd;
}
 
-   kvm__arch_init(kvm, kvm->cfg.hugetlbfs_path, kvm->cfg.ram_size);
+   kvm__arch_init(kvm);
 
INIT_LIST_HEAD(&kvm->mem_banks);
kvm__init_ram(kvm);
diff --git a/mips/kvm.c b/mips/kvm.c
index cebec5ae0178..fb60b210e7fc 100644
--- a/mips/kvm.c
+++ b/mips/kvm.c
@@ -62,12 +62,13 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
 }
 
 /* Architecture-specific KVM init */
-void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
+void kvm__arch_init(struct kvm *kvm)
 {
int ret;
 
-   kvm->ram_start = mmap_anon_or_hugetlbfs(kvm, hugetlbfs_path, ram_size);
-   kvm->ram_size = ram_size;
+   kvm->ram_size = kvm->cfg.ram_size;
+   kvm->ram_start = mmap_anon_or_hugetlbfs(kvm, kvm->cfg.hugetlbfs_path,
+   kvm->ram_size);
 
if (kvm->ram_start == MAP_FAILED)
die("out of memory");
diff --git a/powerpc/kvm.c b/powerpc/kvm.c
index 3215b579f5dc..d281b070fd0e 100644
--- a/powerpc/kvm.c
+++ b/powerpc/kvm.c
@@ -92,12 +92,13 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
 }
 
 /* Architecture-specific KVM init */
-void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
+void kvm__arch_init(struct kvm *kvm)
 {
+   const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
int cap_ppc_rma;
unsigned long hpt;
 
-   kvm->ram_size   = ram_size;
+   kvm->ram_size   = kvm->cfg.ram_size;
 
/* Map "default" hugetblfs path to the standard 16M mount point */
if (hugetlbfs_path && !strcmp(hugetlbfs_path, "default"))
diff --git a/riscv/kvm.c b/riscv/kvm.c
index 7fb496282f4c..c46660772aa0 100644
--- a/riscv/kvm.c
+++ b/riscv/kvm.c
@@ -56,7 +56,7 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
 {
 }
 
-void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
+void kvm__arch_init(struct kvm *kvm)
 {
/*
 * Allocate guest memory. We must align our buffer to 64K to
@@ -64,9 +64,10 @@ void kvm__arch_init(struct kvm *kvm, const char 
*hugetlbfs_path, u64 ram_size)
 * If using THP, then our minimal alignment becomes 2M.
 * 2M trumps 64K, so let's go with that.
 */
-   kvm->ram_size = min(ram_size, (u64)RISCV_MAX_MEMORY(kvm));
+   kvm->ram_size = min(kvm->cfg.ram_size, (u64)RISCV_MAX_MEMORY(kvm));
kvm->arch.ram_alloc_size = kvm->ram_size + SZ_2M;
-   kvm->arch.ram_alloc_start = mmap_anon_or_hugetlbfs(kvm, hugetlbfs_path,
+   kvm->arch.ram_alloc_start = mmap_anon_or_hugetlbf

[PATCH v4 kvmtool 08/12] builtin_run: Allow standard size specifiers for memory

2022-06-16 Thread Alexandru Elisei
From: Suzuki K Poulose 

Allow the user to use the standard B (bytes), K (kilobytes), M (megabytes),
G (gigabytes), T (terabytes) and P (petabytes) suffixes for memory size.
When none are specified, the default is megabytes.

Also raise an error if the guest specifies 0 as the memory size, instead
of treating it as uninitialized, as kvmtool has done so far.

Signed-off-by: Suzuki K Poulose 
Signed-off-by: Alexandru Elisei 
---
 builtin-run.c | 59 ++-
 1 file changed, 54 insertions(+), 5 deletions(-)

diff --git a/builtin-run.c b/builtin-run.c
index dcd08f739469..8b4e865f0a0e 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -49,9 +49,11 @@
 #include 
 #include 
 
-#define MB_SHIFT   (20)
 #define KB_SHIFT   (10)
+#define MB_SHIFT   (20)
 #define GB_SHIFT   (30)
+#define TB_SHIFT   (40)
+#define PB_SHIFT   (50)
 
 __thread struct kvm_cpu *current_kvm_cpu;
 
@@ -87,6 +89,54 @@ void kvm_run_set_wrapper_sandbox(void)
kvm_run_wrapper = KVM_RUN_SANDBOX;
 }
 
+static int parse_mem_unit(char **next)
+{
+   switch (**next) {
+   case 'B': case 'b': (*next)++; return 0;
+   case 'K': case 'k': (*next)++; return KB_SHIFT;
+   case 'M': case 'm': (*next)++; return MB_SHIFT;
+   case 'G': case 'g': (*next)++; return GB_SHIFT;
+   case 'T': case 't': (*next)++; return TB_SHIFT;
+   case 'P': case 'p': (*next)++; return PB_SHIFT;
+   }
+
+   return MB_SHIFT;
+}
+
+static u64 parse_mem_option(const char *nptr, char **next)
+{
+   u64 shift;
+   u64 val;
+
+   errno = 0;
+   val = strtoull(nptr, next, 10);
+   if (errno == ERANGE)
+   die("Memory too large: %s", nptr);
+   if (*next == nptr)
+   die("Invalid memory specifier: %s", nptr);
+
+   shift = parse_mem_unit(next);
+   if ((val << shift) < val)
+   die("Memory too large: %s", nptr);
+
+   return val << shift;
+}
+
+static int mem_parser(const struct option *opt, const char *arg, int unset)
+{
+   struct kvm *kvm = opt->ptr;
+   char *next;
+
+   kvm->cfg.ram_size = parse_mem_option(arg, &next);
+   if (kvm->cfg.ram_size == 0)
+   die("Invalid RAM size: %s", arg);
+
+   if (*next != '\0')
+   die("Invalid memory specifier: %s", arg);
+
+   return 0;
+}
+
 #ifndef OPT_ARCH_RUN
 #define OPT_ARCH_RUN(...)
 #endif
@@ -97,8 +147,9 @@ void kvm_run_set_wrapper_sandbox(void)
OPT_STRING('\0', "name", &(cfg)->guest_name, "guest name",  \
"A name for the guest"),\
OPT_INTEGER('c', "cpus", &(cfg)->nrcpus, "Number of CPUs"), \
-   OPT_U64('m', "mem", &(cfg)->ram_size, "Virtual machine memory"  \
-   " size in MB."),\
+   OPT_CALLBACK('m', "mem", NULL, "size[BKMGTP]",  \
+"Virtual machine memory size, by default measured" \
+" in megabytes (M)", mem_parser, kvm), \
OPT_CALLBACK('d', "disk", kvm, "image or rootfs_dir", "Disk "   \
" image or rootfs directory", img_name_parser,  \
kvm),   \
@@ -522,8 +573,6 @@ static void kvm_run_validate_cfg(struct kvm *kvm)
pr_warning("Ignoring initrd file when loading a firmware 
image");
 
if (kvm->cfg.ram_size) {
-   /* User specifies RAM size in megabytes. */
-   kvm->cfg.ram_size <<= MB_SHIFT;
available_ram = host_ram_size();
if (available_ram && kvm->cfg.ram_size > available_ram) {
pr_warning("Guest memory size %lluMB exceeds host 
physical RAM size %lluMB",
-- 
2.36.1

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


[PATCH v4 kvmtool 07/12] arm/arm64: Kill the ARM_HIMAP_MAX_MEMORY() macro

2022-06-16 Thread Alexandru Elisei
The ARM_HIMAP_MAX_MEMORY() is a remnant of a time when KVM only supported
40 bits if IPA. There are no users left for this macro, remove it.

Reviewed-by: Andre Przywara 
Signed-off-by: Alexandru Elisei 
---
 arm/include/arm-common/kvm-arch.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arm/include/arm-common/kvm-arch.h 
b/arm/include/arm-common/kvm-arch.h
index fc55360d4d15..6d80aac17125 100644
--- a/arm/include/arm-common/kvm-arch.h
+++ b/arm/include/arm-common/kvm-arch.h
@@ -65,7 +65,6 @@
 
 
 #define ARM_LOMAP_MAX_MEMORY   ((1ULL << 32) - ARM_MEMORY_AREA)
-#define ARM_HIMAP_MAX_MEMORY   ((1ULL << 40) - ARM_MEMORY_AREA)
 
 
 #define KVM_IOEVENTFD_HAS_PIO  0
-- 
2.36.1

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


[PATCH v4 kvmtool 06/12] arm/arm64: Kill the ARM_MAX_MEMORY() macro

2022-06-16 Thread Alexandru Elisei
For 32-bit guests, the maximum memory size is represented by the define
ARM_LOMAP_MAX_MEMORY, which ARM_MAX_MEMORY() returns.

For 64-bit guests, the RAM size is checked against the maximum allowed
by KVM in kvm__get_vm_type().

There are no users left for the ARM_MAX_MEMORY() macro, remove it.

Reviewed-by: Andre Przywara 
Signed-off-by: Alexandru Elisei 
---
 arm/aarch32/include/kvm/kvm-arch.h |  2 --
 arm/aarch64/include/kvm/kvm-arch.h | 16 
 2 files changed, 18 deletions(-)

diff --git a/arm/aarch32/include/kvm/kvm-arch.h 
b/arm/aarch32/include/kvm/kvm-arch.h
index 5616b27e257e..467fb09175b8 100644
--- a/arm/aarch32/include/kvm/kvm-arch.h
+++ b/arm/aarch32/include/kvm/kvm-arch.h
@@ -8,8 +8,6 @@
 struct kvm;
 static inline void kvm__arch_enable_mte(struct kvm *kvm) {}
 
-#define ARM_MAX_MEMORY(...)ARM_LOMAP_MAX_MEMORY
-
 #define MAX_PAGE_SIZE  SZ_4K
 
 #include "arm-common/kvm-arch.h"
diff --git a/arm/aarch64/include/kvm/kvm-arch.h 
b/arm/aarch64/include/kvm/kvm-arch.h
index 9124f6919d0f..ff857ca6e7b4 100644
--- a/arm/aarch64/include/kvm/kvm-arch.h
+++ b/arm/aarch64/include/kvm/kvm-arch.h
@@ -8,22 +8,6 @@ unsigned long long kvm__arch_get_kern_offset(struct kvm *kvm, 
int fd);
 int kvm__arch_get_ipa_limit(struct kvm *kvm);
 void kvm__arch_enable_mte(struct kvm *kvm);
 
-#define ARM_MAX_MEMORY(kvm)({  \
-   u64 max_ram;\
-   \
-   if ((kvm)->cfg.arch.aarch32_guest) {\
-   max_ram = ARM_LOMAP_MAX_MEMORY; \
-   } else {\
-   int ipabits = kvm__arch_get_ipa_limit(kvm); \
-   if (ipabits <= 0)   \
-   max_ram = ARM_HIMAP_MAX_MEMORY; \
-   else\
-   max_ram = (1ULL << ipabits) - ARM_MEMORY_AREA;  \
-   }   \
-   \
-   max_ram;\
-})
-
 #define MAX_PAGE_SIZE  SZ_64K
 
 #include "arm-common/kvm-arch.h"
-- 
2.36.1

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


[PATCH v4 kvmtool 05/12] arm/arm64: Fail if RAM size is too large for 32-bit guests

2022-06-16 Thread Alexandru Elisei
For 64-bit guests, kvmtool exists with an error in kvm__get_vm_type() if
the memory size is larger than what KVM supports. For 32-bit guests, the
RAM size is silently rounded down to ARM_LOMAP_MAX_MEMORY in
kvm__arch_init().

Be consistent and exit with an error when the user has configured the
wrong RAM size for 32-bit guests.

Reviewed-by: Andre Przywara 
Signed-off-by: Alexandru Elisei 
---
 arm/aarch32/kvm.c | 4 
 arm/aarch64/kvm.c | 5 +
 arm/kvm.c | 2 +-
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/arm/aarch32/kvm.c b/arm/aarch32/kvm.c
index ae33ac92479a..9d68d7a15ee2 100644
--- a/arm/aarch32/kvm.c
+++ b/arm/aarch32/kvm.c
@@ -2,4 +2,8 @@
 
 void kvm__arch_validate_cfg(struct kvm *kvm)
 {
+   if (kvm->cfg.ram_size > ARM_LOMAP_MAX_MEMORY) {
+   die("RAM size 0x%llx exceeds maximum allowed 0x%llx",
+   kvm->cfg.ram_size, ARM_LOMAP_MAX_MEMORY);
+   }
 }
diff --git a/arm/aarch64/kvm.c b/arm/aarch64/kvm.c
index ca348f118a56..2134528bf7f2 100644
--- a/arm/aarch64/kvm.c
+++ b/arm/aarch64/kvm.c
@@ -39,6 +39,11 @@ int vcpu_affinity_parser(const struct option *opt, const 
char *arg, int unset)
 
 void kvm__arch_validate_cfg(struct kvm *kvm)
 {
+   if (kvm->cfg.arch.aarch32_guest &&
+   kvm->cfg.ram_size > ARM_LOMAP_MAX_MEMORY) {
+   die("RAM size 0x%llx exceeds maximum allowed 0x%llx",
+   kvm->cfg.ram_size, ARM_LOMAP_MAX_MEMORY);
+   }
 }
 
 /*
diff --git a/arm/kvm.c b/arm/kvm.c
index c5913000e1ed..af0feae495d7 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -65,7 +65,7 @@ void kvm__arch_init(struct kvm *kvm, const char 
*hugetlbfs_path, u64 ram_size)
 * If using THP, then our minimal alignment becomes 2M.
 * 2M trumps 64K, so let's go with that.
 */
-   kvm->ram_size = min(ram_size, (u64)ARM_MAX_MEMORY(kvm));
+   kvm->ram_size = ram_size;
kvm->arch.ram_alloc_size = kvm->ram_size + SZ_2M;
kvm->arch.ram_alloc_start = mmap_anon_or_hugetlbfs(kvm, hugetlbfs_path,
kvm->arch.ram_alloc_size);
-- 
2.36.1

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


[PATCH v4 kvmtool 04/12] builtin-run: Add arch hook to validate VM configuration

2022-06-16 Thread Alexandru Elisei
Architectures are free to set their own command line options. Add an
architecture specific hook to validate these options.

For now, the hook does nothing, but it will be used in later patches.

Signed-off-by: Alexandru Elisei 
---
 Makefile  | 1 +
 arm/aarch32/kvm.c | 5 +
 arm/aarch64/kvm.c | 4 
 builtin-run.c | 2 ++
 include/kvm/kvm.h | 1 +
 mips/kvm.c| 4 
 powerpc/kvm.c | 4 
 riscv/kvm.c   | 4 
 x86/kvm.c | 4 
 9 files changed, 29 insertions(+)
 create mode 100644 arm/aarch32/kvm.c

diff --git a/Makefile b/Makefile
index 6464446a9f24..64bb9c95b6f6 100644
--- a/Makefile
+++ b/Makefile
@@ -170,6 +170,7 @@ ifeq ($(ARCH), arm)
OBJS+= $(OBJS_ARM_COMMON)
OBJS+= arm/aarch32/arm-cpu.o
OBJS+= arm/aarch32/kvm-cpu.o
+   OBJS+= arm/aarch32/kvm.o
ARCH_INCLUDE:= $(HDRS_ARM_COMMON)
ARCH_INCLUDE+= -Iarm/aarch32/include
CFLAGS  += -march=armv7-a
diff --git a/arm/aarch32/kvm.c b/arm/aarch32/kvm.c
new file mode 100644
index ..ae33ac92479a
--- /dev/null
+++ b/arm/aarch32/kvm.c
@@ -0,0 +1,5 @@
+#include "kvm/kvm.h"
+
+void kvm__arch_validate_cfg(struct kvm *kvm)
+{
+}
diff --git a/arm/aarch64/kvm.c b/arm/aarch64/kvm.c
index f3fe854e0b3f..ca348f118a56 100644
--- a/arm/aarch64/kvm.c
+++ b/arm/aarch64/kvm.c
@@ -37,6 +37,10 @@ int vcpu_affinity_parser(const struct option *opt, const 
char *arg, int unset)
return 0;
 }
 
+void kvm__arch_validate_cfg(struct kvm *kvm)
+{
+}
+
 /*
  * Return the TEXT_OFFSET value that the guest kernel expects. Note
  * that pre-3.17 kernels expose this value using the native endianness
diff --git a/builtin-run.c b/builtin-run.c
index e1770b3c9df2..dcd08f739469 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -531,6 +531,8 @@ static void kvm_run_validate_cfg(struct kvm *kvm)
(unsigned long long)available_ram >> MB_SHIFT);
}
}
+
+   kvm__arch_validate_cfg(kvm);
 }
 
 static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
index 7b14b33b50ca..9f7b2fb26e95 100644
--- a/include/kvm/kvm.h
+++ b/include/kvm/kvm.h
@@ -187,6 +187,7 @@ int kvm__get_sock_by_instance(const char *name);
 int kvm__enumerate_instances(int (*callback)(const char *name, int pid));
 void kvm__remove_socket(const char *name);
 
+void kvm__arch_validate_cfg(struct kvm *kvm);
 void kvm__arch_set_cmdline(char *cmdline, bool video);
 void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size);
 void kvm__arch_delete_ram(struct kvm *kvm);
diff --git a/mips/kvm.c b/mips/kvm.c
index e668cbbefb25..cebec5ae0178 100644
--- a/mips/kvm.c
+++ b/mips/kvm.c
@@ -13,6 +13,10 @@ struct kvm_ext kvm_req_ext[] = {
{ 0, 0 }
 };
 
+void kvm__arch_validate_cfg(struct kvm *kvm)
+{
+}
+
 void kvm__arch_read_term(struct kvm *kvm)
 {
virtio_console__inject_interrupt(kvm);
diff --git a/powerpc/kvm.c b/powerpc/kvm.c
index 702d67dca614..3215b579f5dc 100644
--- a/powerpc/kvm.c
+++ b/powerpc/kvm.c
@@ -48,6 +48,10 @@ struct kvm_ext kvm_req_ext[] = {
{ 0, 0 }
 };
 
+void kvm__arch_validate_cfg(struct kvm *kvm)
+{
+}
+
 static uint32_t mfpvr(void)
 {
uint32_t r;
diff --git a/riscv/kvm.c b/riscv/kvm.c
index 84e02779a91c..7fb496282f4c 100644
--- a/riscv/kvm.c
+++ b/riscv/kvm.c
@@ -13,6 +13,10 @@ struct kvm_ext kvm_req_ext[] = {
{ 0, 0 },
 };
 
+void kvm__arch_validate_cfg(struct kvm *kvm)
+{
+}
+
 bool kvm__arch_cpu_supports_vm(void)
 {
/* The KVM capability check is enough. */
diff --git a/x86/kvm.c b/x86/kvm.c
index 3e0f0b743f8c..6683a5c81d49 100644
--- a/x86/kvm.c
+++ b/x86/kvm.c
@@ -35,6 +35,10 @@ struct kvm_ext kvm_req_ext[] = {
{ 0, 0 }
 };
 
+void kvm__arch_validate_cfg(struct kvm *kvm)
+{
+}
+
 bool kvm__arch_cpu_supports_vm(void)
 {
struct cpuid_regs regs;
-- 
2.36.1

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


[PATCH v4 kvmtool 03/12] builtin-run: Rework RAM size validation

2022-06-16 Thread Alexandru Elisei
host_ram_size() uses sysconf() to calculate the available ram, and
sysconf() can fail. When that happens, host_ram_size() returns 0. kvmtool
warns the user when the configured VM ram size exceeds the size of the
host's memory, but doesn't take into account that host_ram_size() can
return 0. If the function returns zero, skip the warning.

Since this can only happen when the user sets the memory size (via the
-m/--mem command line argument), skip the check entirely if the user hasn't
set it. Move the check to kvm_run_validate_cfg(), as it checks for valid
user configuration.

Reviewed-by: Andre Przywara 
Signed-off-by: Alexandru Elisei 
---
 builtin-run.c | 20 +---
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/builtin-run.c b/builtin-run.c
index 2bf93fe13c92..e1770b3c9df2 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -509,6 +509,8 @@ static void kvm_run_set_real_cmdline(struct kvm *kvm)
 
 static void kvm_run_validate_cfg(struct kvm *kvm)
 {
+   u64 available_ram;
+
if (kvm->cfg.kernel_filename && kvm->cfg.firmware_filename)
die("Only one of --kernel or --firmware can be specified");
 
@@ -518,6 +520,17 @@ static void kvm_run_validate_cfg(struct kvm *kvm)
 
if (kvm->cfg.firmware_filename && kvm->cfg.initrd_filename)
pr_warning("Ignoring initrd file when loading a firmware 
image");
+
+   if (kvm->cfg.ram_size) {
+   /* User specifies RAM size in megabytes. */
+   kvm->cfg.ram_size <<= MB_SHIFT;
+   available_ram = host_ram_size();
+   if (available_ram && kvm->cfg.ram_size > available_ram) {
+   pr_warning("Guest memory size %lluMB exceeds host 
physical RAM size %lluMB",
+   (unsigned long long)kvm->cfg.ram_size >> 
MB_SHIFT,
+   (unsigned long long)available_ram >> MB_SHIFT);
+   }
+   }
 }
 
 static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
@@ -596,13 +609,6 @@ static struct kvm *kvm_cmd_run_init(int argc, const char 
**argv)
 
if (!kvm->cfg.ram_size)
kvm->cfg.ram_size = get_ram_size(kvm->cfg.nrcpus);
-   else
-   kvm->cfg.ram_size <<= MB_SHIFT;
-
-   if (kvm->cfg.ram_size > host_ram_size())
-   pr_warning("Guest memory size %lluMB exceeds host physical RAM 
size %lluMB",
-   (unsigned long long)kvm->cfg.ram_size >> MB_SHIFT,
-   (unsigned long long)host_ram_size() >> MB_SHIFT);
 
if (!kvm->cfg.dev)
kvm->cfg.dev = DEFAULT_KVM_DEV;
-- 
2.36.1

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


[PATCH v4 kvmtool 02/12] builtin-run: Always use RAM size in bytes

2022-06-16 Thread Alexandru Elisei
The user can specify the virtual machine memory size in MB, which is saved
in cfg->ram_size. kvmtool validates it against the host memory size,
converted from bytes to MB. ram_size is then converted to bytes, and this
is how it is used throughout the rest of kvmtool.

To avoid any confusion about the unit of measurement, especially once the
user is allowed to specify the unit of measurement, always use ram_size in
bytes.

Signed-off-by: Alexandru Elisei 
---
 builtin-run.c| 19 ++-
 include/kvm/kvm-config.h |  7 ---
 include/kvm/kvm.h|  2 +-
 3 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/builtin-run.c b/builtin-run.c
index 0126c9fbcba6..2bf93fe13c92 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -36,6 +36,7 @@
 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -264,7 +265,7 @@ static u64 host_ram_size(void)
return 0;
}
 
-   return (nr_pages * page_size) >> MB_SHIFT;
+   return (u64)nr_pages * page_size;
 }
 
 /*
@@ -278,11 +279,11 @@ static u64 get_ram_size(int nr_cpus)
u64 available;
u64 ram_size;
 
-   ram_size= 64 * (nr_cpus + 3);
+   ram_size= (u64)SZ_64M * (nr_cpus + 3);
 
available   = host_ram_size() * RAM_SIZE_RATIO;
if (!available)
-   available = MIN_RAM_SIZE_MB;
+   available = MIN_RAM_SIZE;
 
if (ram_size > available)
ram_size= available;
@@ -595,13 +596,13 @@ static struct kvm *kvm_cmd_run_init(int argc, const char 
**argv)
 
if (!kvm->cfg.ram_size)
kvm->cfg.ram_size = get_ram_size(kvm->cfg.nrcpus);
+   else
+   kvm->cfg.ram_size <<= MB_SHIFT;
 
if (kvm->cfg.ram_size > host_ram_size())
pr_warning("Guest memory size %lluMB exceeds host physical RAM 
size %lluMB",
-   (unsigned long long)kvm->cfg.ram_size,
-   (unsigned long long)host_ram_size());
-
-   kvm->cfg.ram_size <<= MB_SHIFT;
+   (unsigned long long)kvm->cfg.ram_size >> MB_SHIFT,
+   (unsigned long long)host_ram_size() >> MB_SHIFT);
 
if (!kvm->cfg.dev)
kvm->cfg.dev = DEFAULT_KVM_DEV;
@@ -676,12 +677,12 @@ static struct kvm *kvm_cmd_run_init(int argc, const char 
**argv)
if (kvm->cfg.kernel_filename) {
printf("  # %s run -k %s -m %Lu -c %d --name %s\n", 
KVM_BINARY_NAME,
   kvm->cfg.kernel_filename,
-  (unsigned long long)kvm->cfg.ram_size / 1024 / 1024,
+  (unsigned long long)kvm->cfg.ram_size >> MB_SHIFT,
   kvm->cfg.nrcpus, kvm->cfg.guest_name);
} else if (kvm->cfg.firmware_filename) {
printf("  # %s run --firmware %s -m %Lu -c %d --name %s\n", 
KVM_BINARY_NAME,
   kvm->cfg.firmware_filename,
-  (unsigned long long)kvm->cfg.ram_size / 1024 / 1024,
+  (unsigned long long)kvm->cfg.ram_size >> MB_SHIFT,
   kvm->cfg.nrcpus, kvm->cfg.guest_name);
}
 
diff --git a/include/kvm/kvm-config.h b/include/kvm/kvm-config.h
index 6a5720c4c7d4..31bc89520d52 100644
--- a/include/kvm/kvm-config.h
+++ b/include/kvm/kvm-config.h
@@ -5,6 +5,8 @@
 #include "kvm/vfio.h"
 #include "kvm/kvm-config-arch.h"
 
+#include 
+
 #define DEFAULT_KVM_DEV"/dev/kvm"
 #define DEFAULT_CONSOLE"serial"
 #define DEFAULT_NETWORK"user"
@@ -15,14 +17,13 @@
 #define DEFAULT_SCRIPT "none"
 #define DEFAULT_SANDBOX_FILENAME "guest/sandbox.sh"
 
-#define MIN_RAM_SIZE_MB(64ULL)
-#define MIN_RAM_SIZE_BYTE  (MIN_RAM_SIZE_MB << MB_SHIFT)
+#define MIN_RAM_SIZE   SZ_64M
 
 struct kvm_config {
struct kvm_config_arch arch;
struct disk_image_params disk_image[MAX_DISK_IMAGES];
struct vfio_device_params *vfio_devices;
-   u64 ram_size;
+   u64 ram_size;   /* Guest memory size, in bytes */
u8 num_net_devices;
u8 num_vfio_devices;
u64 vsock_cid;
diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
index ad732e56f5ed..7b14b33b50ca 100644
--- a/include/kvm/kvm.h
+++ b/include/kvm/kvm.h
@@ -87,7 +87,7 @@ struct kvm {
struct kvm_cpu  **cpus;
 
u32 mem_slots;  /* for 
KVM_SET_USER_MEMORY_REGION */
-   u64 ram_size;
+   u64 ram_size;   /* Guest memory size, in bytes 
*/
void*ram_start;
u64 ram_pagesize;
struct mutexmem_banks_lock;
-- 
2.36.1

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


[PATCH v4 kvmtool 01/12] Use MB for megabytes consistently

2022-06-16 Thread Alexandru Elisei
The help text for the -m/--mem argument states that the guest memory size
is in MiB (mebibyte). MiB is the same thing as MB (megabyte), and indeed
this is how MB is used throughout kvmtool.

Replace MiB with MB, so people don't get the wrong idea and start
believing that for kvmtool a MB is 10^6 bytes instead of 2^20.

Signed-off-by: Alexandru Elisei 
---
 Documentation/kvmtool.1 | 4 ++--
 builtin-run.c   | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/kvmtool.1 b/Documentation/kvmtool.1
index 2b8c274dc3ff..25d46f8f51f9 100644
--- a/Documentation/kvmtool.1
+++ b/Documentation/kvmtool.1
@@ -10,7 +10,7 @@ kvmtool is a userland tool for creating and controlling KVM 
guests.
 .SH "KVMTOOL COMMANDS"
 .sp
 .PP
-.B run -k  [\-c ] [\-m ] [\-p ]
+.B run -k  [\-c ] [\-m ] [\-p ]
 .br
 .B [\-i ] [\-d ] [\-\-console serial|virtio|hv]
 .br
@@ -30,7 +30,7 @@ The number of virtual CPUs to run.
 .sp
 .B \-m, \-\-mem 
 .RS 4
-Virtual machine memory size in MiB.
+Virtual machine memory size in MB.
 .RE
 .sp
 .B \-p, \-\-params 
diff --git a/builtin-run.c b/builtin-run.c
index 9a1a0c1fa6fb..0126c9fbcba6 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -97,7 +97,7 @@ void kvm_run_set_wrapper_sandbox(void)
"A name for the guest"),\
OPT_INTEGER('c', "cpus", &(cfg)->nrcpus, "Number of CPUs"), \
OPT_U64('m', "mem", &(cfg)->ram_size, "Virtual machine memory"  \
-   " size in MiB."),   \
+   " size in MB."),\
OPT_CALLBACK('d', "disk", kvm, "image or rootfs_dir", "Disk "   \
" image or rootfs directory", img_name_parser,  \
kvm),   \
-- 
2.36.1

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


[PATCH v4 kvmtool 00/12] arm64: Allow the user to set RAM base address

2022-06-16 Thread Alexandru Elisei
The series can be found at [1]. It is loosely based on the patches that
allow the user to define the VM memory layout (RAM + MMIO) [2]. I've
cherry-picked a handful of patches from that series, the rest I wrote from
scratch since there have been several changes to the way guest memory is
handled. I've chosen to focus on specifying the RAM layout with only one
RAM bank and leave the rest for a later series because this was relatively
easy to accomplish, while still being very useful.

What this series does: for arm64, the user can now specify the base address
for RAM:

$ ./lkvm run -m1G@2G .. # Equivalent to ./lkvm run -m1024

The memory units are B (bytes), K (kilobytes), M (megabytes), G
(gigabytes), T (terrabytes), P (petabytes). Lowercase is also valid.

Want to put RAM at the top of the physical address range? Easy:

$ ./lkvm run -m2G@1022G .. # Assumes the maximum is 40 bits of IPA

There one limitation on the RAM base address: it must not overlap with the
MMIO range that kvmtool uses for arm/arm64, which lives below 2GB.

Why this is useful, in my opinion:

1. Testing how a payload handles different memory layouts without the need
to hack kvmtool or find the hardware that implements the desired layout.

2. It can serve as a development tool for adding support for larger PA
ranges for Linux and KVM (currently capped at 48 bits for 4k/16k pages), or
other payloads.

Summary of the series
==

* The series starts with refactoring how kvm->cfg.ram_size is validated
  and used, followed by several cleanups in the arm and arm64 code.

* Then patch #8 ("builtin_run: Allow standard size specifiers for memory")
  introduced the ability to specify the measurement unit for memory. I
  believe that typing the equivalent of 2TB in megabytes isn't appealing
  for anyone.

* More cleanups in the arm/arm64 code follow, which are needed for patch
  #12 ("arm64: Allow the user to specify the RAM base address"). This is
  where the ability to specify the RAM base address is introduced.

Testing
===

Same testing as before:

- Build tested each patch for all architectures.

- Ran an x86 kernel with and without setting the amount of RAM using the
  memory specifiers; tested that setting the RAM address results in an
  error.

- Ran an arm64 kernel without setting the size, with setting the size and
  with setting the size and address; tried different addresses (2G, 3G,
  256G); also tested that going below 2G or above the maximum IPA correctly
  results in an error.

- Ran all arm64 kvm-unit-test tests with similar combinations of memory
  size and address (instead of 256G I used 128G, as that's where I/O lives
  for qemu and kvm-unit-tests maps that unconditionally as I/O).

- Ran all 32bit arm tests on an arm64 host with various combinations of
  memory size and address (base address at 2G and 2.5G only due to a
  limitation in the way the tests are set up).

Changelog
=

Since v3:

* Gathered Reviewed-by tags, thank you!

* Rebased on top of current HEAD, f44af23e3a62 ("virtio/pci:
  Factor MSI route creation").

* Dropped what was patch #2 ("sizes.h: Make all sizes 64bit") and opted
  instead to cast SZ_64M to u64 in get_ram_size() in what is now patch #2
  ("builtin-run: Always use RAM size in bytes") (Andre).

* Reworked the way the unit of measurement for memory is parsed (Andre).

* Reworked the way the memory size is validated to specifically check for an
  invalid number (Andre).

Since v2:

* Patch #2 ("sizes.h: Make all sizes 64bit") is new (reported by Andre).

* Casted nr_pages to u64 in host_ram_size() to avoid overflows when multiplied
  by page_size on 32-bit systems with more than 2GB of RAM (Andre).

* Initialize ram_addr before parsing the command line options because the
  default was at address 0, which is invalid for arm64 (Andre).

* Fix check for RAM top above 4GB for aarch32 guests.

Since v1:

* Rebased on top of current HEAD (commit 4639b72f61a3 ("arm64: Add
  --vcpu-affinity command line argument")).

* Removed the last 3 patches that touched the --firmware-address command line
  argument. They weren't necessary for this series, I'll resend them after this
  series gets merged.

* Moved patch #8 ("builtin_run: Allow standard size specifiers for memory")
  later in the series (was #6).

[1] 
https://gitlab.arm.com/linux-arm/kvmtool-ae/-/tree/arm-allow-the-user-to-define-ram-address-v4
[2] 
https://lkml.kernel.org/kvm/1569245722-23375-1-git-send-email-alexandru.eli...@arm.com/

Alexandru Elisei (9):
  Use MB for megabytes consistently
  builtin-run: Always use RAM size in bytes
  builtin-run: Rework RAM size validation
  builtin-run: Add arch hook to validate VM configuration
  arm/arm64: Fail if RAM size is too large for 32-bit guests
  arm/arm64: Kill the ARM_MAX_MEMORY() macro
  arm/arm64: Kill the ARM_HIMAP_MAX_MEMORY() macro
  Introduce kvm__arch_default_ram_address()
  arm64: Allow the user to specify the RAM base address

Julien Grall (2):
  kvm__arch_init:

Re: [PATCH 1/4] KVM: selftests: enumerate GUEST_ASSERT arguments

2022-06-16 Thread Andrew Jones
On Wed, Jun 15, 2022 at 07:31:13PM +, Colton Lewis wrote:
> Enumerate GUEST_ASSERT arguments to avoid magic indices to ucall.args.
> 
> Signed-off-by: Colton Lewis 
> ---
>  tools/testing/selftests/kvm/include/ucall_common.h | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/ucall_common.h 
> b/tools/testing/selftests/kvm/include/ucall_common.h
> index 98562f685151..dbe872870b83 100644
> --- a/tools/testing/selftests/kvm/include/ucall_common.h
> +++ b/tools/testing/selftests/kvm/include/ucall_common.h
> @@ -32,6 +32,14 @@ uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall 
> *uc);
>   ucall(UCALL_SYNC, 6, "hello", stage, arg1, 
> arg2, arg3, arg4)
>  #define GUEST_SYNC(stage)ucall(UCALL_SYNC, 2, "hello", stage)
>  #define GUEST_DONE() ucall(UCALL_DONE, 0)
> +
> +enum guest_assert_builtin_args {
> + GUEST_ERROR_STRING,
> + GUEST_FILE,
> + GUEST_LINE,
> + GUEST_ASSERT_BUILTIN_NARGS
> +};
> +
>  #define __GUEST_ASSERT(_condition, _condstr, _nargs, _args...) do {\
>   if (!(_condition))  \
>   ucall(UCALL_ABORT, 2 + _nargs,  \
> -- 
> 2.36.1.476.g0c4daa206d-goog
>

Reviewed-by: Andrew Jones 

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH 3/4] KVM: selftests: Write REPORT_GUEST_ASSERT macros to pair with GUEST_ASSERT

2022-06-16 Thread Andrew Jones
On Wed, Jun 15, 2022 at 07:31:15PM +, Colton Lewis wrote:
> Write REPORT_GUEST_ASSERT macros to pair with GUEST_ASSERT to abstract
> and make consistent all guest assertion reporting. Every report
> includes an explanatory string, a filename, and a line number.
> 
> Signed-off-by: Colton Lewis 
> ---
>  .../selftests/kvm/include/ucall_common.h  | 42 +++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/ucall_common.h 
> b/tools/testing/selftests/kvm/include/ucall_common.h
> index 568c562f14cd..e8af3b4fef6d 100644
> --- a/tools/testing/selftests/kvm/include/ucall_common.h
> +++ b/tools/testing/selftests/kvm/include/ucall_common.h
> @@ -6,6 +6,7 @@
>   */
>  #ifndef SELFTEST_KVM_UCALL_COMMON_H
>  #define SELFTEST_KVM_UCALL_COMMON_H
> +#include "test_util.h"
>  
>  /* Common ucalls */
>  enum {
> @@ -64,4 +65,45 @@ enum guest_assert_builtin_args {
>  
>  #define GUEST_ASSERT_EQ(a, b) __GUEST_ASSERT((a) == (b), #a " == " #b, 2, a, 
> b)
>  
> +#define __REPORT_GUEST_ASSERT(_ucall, fmt, _args...) \
> + TEST_FAIL("%s at %s:%ld\n" fmt, \
> +   (const char *)(_ucall).args[GUEST_ERROR_STRING],  \
> +   (const char *)(_ucall).args[GUEST_FILE],  \
> +   (_ucall).args[GUEST_LINE],\
> +   ##_args)
> +
> +#define GUEST_ASSERT_ARG(ucall, i) ((ucall).args[GUEST_ASSERT_BUILTIN_NARGS 
> + i])
> +
> +#define REPORT_GUEST_ASSERT(ucall)   \
> + __REPORT_GUEST_ASSERT((ucall), "")
> +
> +#define REPORT_GUEST_ASSERT_1(ucall, fmt)\
> + __REPORT_GUEST_ASSERT((ucall),  \
> +   fmt,  \
> +   GUEST_ASSERT_ARG((ucall), 0))
> +
> +#define REPORT_GUEST_ASSERT_2(ucall, fmt)\
> + __REPORT_GUEST_ASSERT((ucall),  \
> +   fmt,  \
> +   GUEST_ASSERT_ARG((ucall), 0), \
> +   GUEST_ASSERT_ARG((ucall), 1))
> +
> +#define REPORT_GUEST_ASSERT_3(ucall, fmt)\
> + __REPORT_GUEST_ASSERT((ucall),  \
> +   fmt,  \
> +   GUEST_ASSERT_ARG((ucall), 0), \
> +   GUEST_ASSERT_ARG((ucall), 1), \
> +   GUEST_ASSERT_ARG((ucall), 2))
> +
> +#define REPORT_GUEST_ASSERT_4(ucall, fmt)\
> + __REPORT_GUEST_ASSERT((ucall),  \
> +   fmt,  \
> +   GUEST_ASSERT_ARG((ucall), 0), \
> +   GUEST_ASSERT_ARG((ucall), 1), \
> +   GUEST_ASSERT_ARG((ucall), 2), \
> +   GUEST_ASSERT_ARG((ucall), 3))
> +
> +#define REPORT_GUEST_ASSERT_N(ucall, fmt, args...)   \
> + __REPORT_GUEST_ASSERT((ucall), fmt, ##args)
> +
>  #endif /* SELFTEST_KVM_UCALL_COMMON_H */
> -- 
> 2.36.1.476.g0c4daa206d-goog
>

nit: All the ()'s around ucall when it's between ( and , are unnecessary.

Otherwise,

Reviewed-by: Andrew Jones 

Thanks,
drew

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH 4/4] KVM: selftests: Fix filename reporting in guest asserts

2022-06-16 Thread Andrew Jones
On Wed, Jun 15, 2022 at 07:31:16PM +, Colton Lewis wrote:
> Fix filename reporting in guest asserts by ensuring the GUEST_ASSERT
> macro records __FILE__ and substituting REPORT_GUEST_ASSERT for many
> repetitive calls to TEST_FAIL.
> 
> Previously filename was reported by using __FILE__ directly in the
> selftest, wrongly assuming it would always be the same as where the
> assertion failed.y
> 
> Signed-off-by: Colton Lewis 
> Reported-by: Ricardo Koller 
> Fixes: 4e18bccc2e5544f0be28fc1c4e6be47a469d6c60
> ---
>  tools/testing/selftests/kvm/aarch64/arch_timer.c   | 12 
>  .../selftests/kvm/aarch64/debug-exceptions.c   |  4 +---
>  tools/testing/selftests/kvm/aarch64/vgic_irq.c |  4 +---
>  tools/testing/selftests/kvm/include/ucall_common.h | 14 --
>  tools/testing/selftests/kvm/memslot_perf_test.c|  4 +---
>  tools/testing/selftests/kvm/steal_time.c   |  3 +--
>  .../selftests/kvm/system_counter_offset_test.c |  3 +--
>  tools/testing/selftests/kvm/x86_64/amx_test.c  |  3 +--
>  tools/testing/selftests/kvm/x86_64/cpuid_test.c|  3 +--
>  .../selftests/kvm/x86_64/cr4_cpuid_sync_test.c |  2 +-
>  .../selftests/kvm/x86_64/emulator_error_test.c |  3 +--
>  tools/testing/selftests/kvm/x86_64/evmcs_test.c|  3 +--
>  tools/testing/selftests/kvm/x86_64/hyperv_clock.c  |  3 +--
>  .../testing/selftests/kvm/x86_64/hyperv_features.c |  6 ++
>  .../testing/selftests/kvm/x86_64/hyperv_svm_test.c |  3 +--
>  .../testing/selftests/kvm/x86_64/kvm_clock_test.c  |  3 +--
>  tools/testing/selftests/kvm/x86_64/kvm_pv_test.c   |  3 +--
>  .../testing/selftests/kvm/x86_64/set_boot_cpu_id.c |  4 +---
>  tools/testing/selftests/kvm/x86_64/state_test.c|  3 +--
>  .../selftests/kvm/x86_64/svm_int_ctl_test.c|  2 +-
>  .../testing/selftests/kvm/x86_64/svm_vmcall_test.c |  2 +-
>  tools/testing/selftests/kvm/x86_64/tsc_msrs_test.c |  4 +---
>  .../selftests/kvm/x86_64/userspace_io_test.c   |  4 +---
>  .../selftests/kvm/x86_64/userspace_msr_exit_test.c |  5 ++---
>  .../selftests/kvm/x86_64/vmx_apic_access_test.c|  3 +--
>  .../kvm/x86_64/vmx_close_while_nested_test.c   |  2 +-
>  .../selftests/kvm/x86_64/vmx_dirty_log_test.c  |  3 +--
>  .../kvm/x86_64/vmx_invalid_nested_guest_state.c|  2 +-
>  .../kvm/x86_64/vmx_nested_tsc_scaling_test.c   |  2 +-
>  .../kvm/x86_64/vmx_preemption_timer_test.c |  3 +--
>  .../selftests/kvm/x86_64/vmx_tsc_adjust_test.c |  2 +-
>  .../testing/selftests/kvm/x86_64/xen_shinfo_test.c |  2 +-
>  .../testing/selftests/kvm/x86_64/xen_vmcall_test.c |  2 +-
>  33 files changed, 49 insertions(+), 72 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c 
> b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> index f68019be67c0..9d7bda70c20a 100644
> --- a/tools/testing/selftests/kvm/aarch64/arch_timer.c
> +++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
> @@ -231,10 +231,14 @@ static void *test_vcpu_run(void *arg)
>   break;
>   case UCALL_ABORT:
>   sync_global_from_guest(vm, *shared_data);
> - TEST_FAIL("%s at %s:%ld\n\tvalues: %lu, %lu; %lu, vcpu: %u; 
> stage: %u; iter: %u",
> - (const char *)uc.args[0], __FILE__, uc.args[1],
> - uc.args[2], uc.args[3], uc.args[4], vcpu_idx,
> - shared_data->guest_stage, shared_data->nr_iter);
> + REPORT_GUEST_ASSERT_N(uc,
> +   "values: %lu, %lu; %lu, vcpu %u; stage; 
> %u; iter: %u",
> +   GUEST_ASSERT_ARG(uc, 0),
> +   GUEST_ASSERT_ARG(uc, 1),
> +   GUEST_ASSERT_ARG(uc, 2),
> +   vcpu_idx,
> +   shared_data->guest_stage,
> +   shared_data->nr_iter);
>   break;
>   default:
>   TEST_FAIL("Unexpected guest exit\n");
> diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c 
> b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> index b8072b40ccc8..2ee35cf9801e 100644
> --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> @@ -283,9 +283,7 @@ int main(int argc, char *argv[])
>   stage, (ulong)uc.args[1]);
>   break;
>   case UCALL_ABORT:
> - TEST_FAIL("%s at %s:%ld\n\tvalues: %#lx, %#lx",
> - (const char *)uc.args[0],
> - __FILE__, uc.args[1], uc.args[2], uc.args[3]);
> + REPORT_GUEST_ASSERT_2(uc, "values: %#lx, %#lx");
>   break;
>   case UCALL_DONE:
>   goto done;
> diff --git a/tools/testing/selftests/kvm/aarch64/vgic_irq.c 
> b/tools/testing/selftests/kvm/aarch64/vgic_

Re: [PATCH] selftests: KVM: Handle compiler optimizations in ucall

2022-06-16 Thread Paolo Bonzini
Queued, thanks.

Paolo


___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH 2/4] KVM: selftests: Increase UCALL_MAX_ARGS to 7

2022-06-16 Thread Andrew Jones
On Wed, Jun 15, 2022 at 07:31:14PM +, Colton Lewis wrote:
> Increase UCALL_MAX_ARGS to 7 to allow GUEST_ASSERT_4 to pass 3 builtin
> ucall arguments specified in guest_assert_builtin_args plus 4
> user-specified arguments.
> 
> Signed-off-by: Colton Lewis 
> ---
>  tools/testing/selftests/kvm/include/ucall_common.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/ucall_common.h 
> b/tools/testing/selftests/kvm/include/ucall_common.h
> index dbe872870b83..568c562f14cd 100644
> --- a/tools/testing/selftests/kvm/include/ucall_common.h
> +++ b/tools/testing/selftests/kvm/include/ucall_common.h
> @@ -16,7 +16,7 @@ enum {
>   UCALL_UNHANDLED,
>  };
>  
> -#define UCALL_MAX_ARGS 6
> +#define UCALL_MAX_ARGS 7
>  
>  struct ucall {
>   uint64_t cmd;
> -- 
> 2.36.1.476.g0c4daa206d-goog
>

We probably want to ensure all architectures are good with this. afaict,
riscv only expects 6 args and uses UCALL_MAX_ARGS to cap the ucall inputs,
for example.

Thanks,
drew

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH] selftests: KVM: Handle compiler optimizations in ucall

2022-06-16 Thread Andrew Jones
On Wed, Jun 15, 2022 at 06:57:06PM +, Raghavendra Rao Ananta wrote:
> The selftests, when built with newer versions of clang, is found
> to have over optimized guests' ucall() function, and eliminating
> the stores for uc.cmd (perhaps due to no immediate readers). This
> resulted in the userspace side always reading a value of '0', and
> causing multiple test failures.
> 
> As a result, prevent the compiler from optimizing the stores in
> ucall() with WRITE_ONCE().
> 
> Suggested-by: Ricardo Koller 
> Suggested-by: Reiji Watanabe 
> Signed-off-by: Raghavendra Rao Ananta 
> ---
>  tools/testing/selftests/kvm/lib/aarch64/ucall.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/ucall.c 
> b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> index e0b0164e9af8..be1d9728c4ce 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
> @@ -73,20 +73,19 @@ void ucall_uninit(struct kvm_vm *vm)
>  
>  void ucall(uint64_t cmd, int nargs, ...)
>  {
> - struct ucall uc = {
> - .cmd = cmd,
> - };
> + struct ucall uc = {};
>   va_list va;
>   int i;
>  
> + WRITE_ONCE(uc.cmd, cmd);
>   nargs = nargs <= UCALL_MAX_ARGS ? nargs : UCALL_MAX_ARGS;
>  
>   va_start(va, nargs);
>   for (i = 0; i < nargs; ++i)
> - uc.args[i] = va_arg(va, uint64_t);
> + WRITE_ONCE(uc.args[i], va_arg(va, uint64_t));
>   va_end(va);
>  
> - *ucall_exit_mmio_addr = (vm_vaddr_t)&uc;
> + WRITE_ONCE(*ucall_exit_mmio_addr, (vm_vaddr_t)&uc);
>  }
>  
>  uint64_t get_ucall(struct kvm_vm *vm, uint32_t vcpu_id, struct ucall *uc)
> -- 
> 2.36.1.476.g0c4daa206d-goog
>

Reviewed-by: Andrew Jones 

Thanks,
drew

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


[PATCH] KVM: arm64: Add Oliver as a reviewer

2022-06-16 Thread Marc Zyngier
Oliver Upton has agreed to help with reviewing the KVM/arm64
patches, and has been doing so for a while now, so adding him
as to the reviewer list.

Note that Oliver is using a different email address for this
purpose, rather than the one his been using for his other
contributions.

Signed-off-by: Marc Zyngier 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index a6d3bd9d2a8d..7192d1277558 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10821,6 +10821,7 @@ M:  Marc Zyngier 
 R: James Morse 
 R: Alexandru Elisei 
 R: Suzuki K Poulose 
+R: Oliver Upton 
 L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers)
 L: kvmarm@lists.cs.columbia.edu (moderated for non-subscribers)
 S: Maintained
-- 
2.34.1

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm