Re: [kvm-unit-tests PATCH v2 6/8] s390x: Add cmm tests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 20.03.2018 12:19, Janosch Frank wrote:
> Collaborative Memory Management is the extended vm memory usage
> hinting for the hypervisor and handling the ESSA instruction is quite
> complicated. Let's add at least a bare-bones test, maybe someone will
> extend it later.
> 
> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxxxxxxx>
> ---
>  lib/s390x/asm/interrupt.h |  1 +
>  lib/s390x/interrupt.c     |  8 ++++++
>  s390x/Makefile            |  1 +
>  s390x/cmm.c               | 71 +++++++++++++++++++++++++++++++++++++++++++++++
>  s390x/unittests.cfg       |  3 ++
>  5 files changed, 84 insertions(+)
>  create mode 100644 s390x/cmm.c
> 
> diff --git a/lib/s390x/asm/interrupt.h b/lib/s390x/asm/interrupt.h
> index 3ccc8e3..3f8f2f7 100644
> --- a/lib/s390x/asm/interrupt.h
> +++ b/lib/s390x/asm/interrupt.h
> @@ -15,6 +15,7 @@ void handle_pgm_int(void);
>  void expect_pgm_int(void);
>  uint16_t clear_pgm_int(void);
>  void check_pgm_int_code(uint16_t code);
> +uint16_t get_pgm_int_code(void);
>  
>  /* Activate low-address protection */
>  static inline void low_prot_enable(void)
> diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c
> index 56c7603..bfa21e8 100644
> --- a/lib/s390x/interrupt.c
> +++ b/lib/s390x/interrupt.c
> @@ -41,6 +41,14 @@ void check_pgm_int_code(uint16_t code)
>  	       code == lc->pgm_int_code, code, lc->pgm_int_code);
>  }
>  
> +uint16_t get_pgm_int_code(void)
> +{
> +	mb();
> +	if (!lc->pgm_int_code)
> +		pgm_int_expected = false;
> +	return lc->pgm_int_code;
> +}
> +
>  static void fixup_pgm_int(void)
>  {
>  	switch (lc->pgm_int_code) {
> diff --git a/s390x/Makefile b/s390x/Makefile
> index 438c6b2..e04cad0 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -6,6 +6,7 @@ tests += $(TEST_DIR)/sthyi.elf
>  tests += $(TEST_DIR)/skey.elf
>  tests += $(TEST_DIR)/diag10.elf
>  tests += $(TEST_DIR)/pfmf.elf
> +tests += $(TEST_DIR)/cmm.elf
>  
>  all: directories test_cases
>  
> diff --git a/s390x/cmm.c b/s390x/cmm.c
> new file mode 100644
> index 0000000..817d84c
> --- /dev/null
> +++ b/s390x/cmm.c
> @@ -0,0 +1,71 @@
> +/*
> + * CMM tests (ESSA)
> + *
> + * Copyright (c) 2018 IBM Corp
> + *
> + * Authors:
> + *  Janosch Frank <frankja@xxxxxxxxxxxxxxxxxx>
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Library General Public License version 2.
> + */
> +
> +#include <libcflat.h>
> +#include <asm/asm-offsets.h>
> +#include <asm/interrupt.h>
> +#include <asm/page.h>
> +
> +static uint8_t pagebuf[PAGE_SIZE * 2] __attribute__((aligned(PAGE_SIZE * 2)));
> +const unsigned long page0 = (unsigned long)pagebuf;
> +
> +static unsigned long essa(uint8_t state, unsigned long paddr)
> +{
> +	register uint64_t addr asm("0") = paddr;
> +	register uint64_t extr_state asm("1");
> +
> +	asm volatile(".insn rrf,0xb9ab0000,%[extr_state],%[addr],%[new_state],0"
> +			: [extr_state] "=&d" (extr_state)

Why do you need the "&" modifier here?

> +			: [addr] "a" (addr), [new_state] "i" (state));

According to my version of the gcc documentation, the "a" constraint
means: "Address register (general purpose register except r0)". So it's
a little bit weird that you use it together with asm("0"). I guess it
does not really matter in this case ... but while we're at it: I think
you could simply drop the local "addr" variable here and use paddr
directly instead.

You could also drop the asm("1") from the extr_state variable as far as
I can see.

> +	return (unsigned long)extr_state;
> +}

(remaining parts of the patch look fine to me)

 Thomas



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux