[PATCH v2 0/4] KVM: s390: Fix minor bugs in STFLE shadowing

[PATCH v2 0/4] KVM: s390: Fix minor bugs in STFLE shadowing

[Nina Schoetterl-Glausch]

v1 -> v2 (range-diff below):
 * pick up tags (thanks {Claudio, David})
 * drop Fixes tag on cleanup patch, change message (thanks David)
 * drop Fixes tag on second patch since the length of the facility list
   copied wasn't initially specified and only clarified in later
   revisions
 * use READ/WRITE_ONCE (thanks {David, Heiko})

Improve the STFLE vsie implementation.
Firstly, fix a bug concerning the identification if the guest is
intending to use interpretive execution for STFLE for its guest.
Secondly, decrease the amount of guest memory accessed to the
minimum.
Also do some (optional) cleanups.

Nina Schoetterl-Glausch (4):
  KVM: s390: vsie: Fix STFLE interpretive execution identification
  KVM: s390: vsie: Fix length of facility list shadowed
  KVM: s390: cpu model: Use proper define for facility mask size
  KVM: s390: Minor refactor of base/ext facility lists

 arch/s390/include/asm/facility.h |  6 +++++
 arch/s390/include/asm/kvm_host.h |  2 +-
 arch/s390/kernel/Makefile        |  2 +-
 arch/s390/kernel/facility.c      | 21 +++++++++++++++
 arch/s390/kvm/kvm-s390.c         | 44 ++++++++++++++------------------
 arch/s390/kvm/vsie.c             | 15 +++++++++--
 6 files changed, 61 insertions(+), 29 deletions(-)
 create mode 100644 arch/s390/kernel/facility.c

Range-diff against v1:
1:  cffe5f1c29d1 ! 1:  de77a2c36786 KVM: s390: vsie: Fix STFLE interpretive execution identification
    @@ Commit message
         Perform the check before applying the address mask instead of after.
     
         Fixes: 66b630d5b7f2 ("KVM: s390: vsie: support STFLE interpretation")
    +    Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
    +    Acked-by: David Hildenbrand <david@redhat.com>
         Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
     
      ## arch/s390/kvm/vsie.c ##
2:  8ef9965c4671 ! 2:  f3b189627e96 KVM: s390: vsie: Fix length of facility list shadowed
    @@ Commit message
         The memory following the facility list need not be accessible, in which
         case we'd wrongly inject a validity intercept.
     
    -    Fixes: 66b630d5b7f2 ("KVM: s390: vsie: support STFLE interpretation")
    +    Acked-by: David Hildenbrand <david@redhat.com>
         Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
     
      ## arch/s390/include/asm/facility.h ##
    @@ arch/s390/kernel/facility.c (new)
     +
     +unsigned int stfle_size(void)
     +{
    -+	static unsigned int size = 0;
    ++	static unsigned int size;
     +	u64 dummy;
    ++	unsigned int r;
     +
    -+	if (!size) {
    -+		size = __stfle_asm(&dummy, 1) + 1;
    ++	r = READ_ONCE(size);
    ++	if (!r) {
    ++		r = __stfle_asm(&dummy, 1) + 1;
    ++		WRITE_ONCE(size, r);
     +	}
    -+	return size;
    ++	return r;
     +}
     +EXPORT_SYMBOL(stfle_size);
     
3:  4104a7c218f1 ! 3:  4907bb8fb2bc KVM: s390: cpu model: Use previously unused constant
    @@ Metadata
     Author: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
     
      ## Commit message ##
    -    KVM: s390: cpu model: Use previously unused constant
    +    KVM: s390: cpu model: Use proper define for facility mask size
     
    -    No point in defining a size for the mask if we're not going to use it.
    +    Use the previously unused S390_ARCH_FAC_MASK_SIZE_U64 instead of
    +    S390_ARCH_FAC_LIST_SIZE_U64 for defining the fac_mask array.
    +    Note that both values are the same, there is no functional change.
     
    -    Fixes: 9d8d578605b4 ("KVM: s390: use facilities and cpu_id per KVM")
         Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
     
      ## arch/s390/include/asm/kvm_host.h ##
4:  b6a18de5a089 = 4:  2745898a22c3 KVM: s390: Minor refactor of base/ext facility lists

base-commit: 05d3ef8bba77c1b5f98d941d8b2d4aeab8118ef1
-- 
2.39.2

[PATCH v2 1/4] KVM: s390: vsie: Fix STFLE interpretive execution identification

[Nina Schoetterl-Glausch]

STFLE can be interpretively executed.
This occurs when the facility list designation is unequal to zero.
Perform the check before applying the address mask instead of after.

Fixes: 66b630d5b7f2 ("KVM: s390: vsie: support STFLE interpretation")
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Acked-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 arch/s390/kvm/vsie.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 61499293c2ac..d989772fe211 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -988,9 +988,10 @@ static void retry_vsie_icpt(struct vsie_page *vsie_page)
 static int handle_stfle(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 {
 	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
-	__u32 fac = READ_ONCE(vsie_page->scb_o->fac) & 0x7ffffff8U;
+	__u32 fac = READ_ONCE(vsie_page->scb_o->fac);
 
 	if (fac && test_kvm_facility(vcpu->kvm, 7)) {
+		fac = fac & 0x7ffffff8U;
 		retry_vsie_icpt(vsie_page);
 		if (read_guest_real(vcpu, fac, &vsie_page->fac,
 				    sizeof(vsie_page->fac)))
-- 
2.39.2

[PATCH v2 2/4] KVM: s390: vsie: Fix length of facility list shadowed

[Nina Schoetterl-Glausch]

The length of the facility list accessed when interpretively executing
STFLE is the same as the hosts facility list (in case of format-0)
When shadowing, copy only those bytes.
The memory following the facility list need not be accessible, in which
case we'd wrongly inject a validity intercept.

Acked-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 arch/s390/include/asm/facility.h |  6 ++++++
 arch/s390/kernel/Makefile        |  2 +-
 arch/s390/kernel/facility.c      | 21 +++++++++++++++++++++
 arch/s390/kvm/vsie.c             | 12 +++++++++++-
 4 files changed, 39 insertions(+), 2 deletions(-)
 create mode 100644 arch/s390/kernel/facility.c

diff --git a/arch/s390/include/asm/facility.h b/arch/s390/include/asm/facility.h
index 94b6919026df..796007125dff 100644
--- a/arch/s390/include/asm/facility.h
+++ b/arch/s390/include/asm/facility.h
@@ -111,4 +111,10 @@ static inline void stfle(u64 *stfle_fac_list, int size)
 	preempt_enable();
 }
 
+/**
+ * stfle_size - Actual size of the facility list as specified by stfle
+ * (number of double words)
+ */
+unsigned int stfle_size(void);
+
 #endif /* __ASM_FACILITY_H */
diff --git a/arch/s390/kernel/Makefile b/arch/s390/kernel/Makefile
index 0df2b88cc0da..0daa81439478 100644
--- a/arch/s390/kernel/Makefile
+++ b/arch/s390/kernel/Makefile
@@ -41,7 +41,7 @@ obj-y	+= sysinfo.o lgr.o os_info.o
 obj-y	+= runtime_instr.o cache.o fpu.o dumpstack.o guarded_storage.o sthyi.o
 obj-y	+= entry.o reipl.o kdebugfs.o alternative.o
 obj-y	+= nospec-branch.o ipl_vmparm.o machine_kexec_reloc.o unwind_bc.o
-obj-y	+= smp.o text_amode31.o stacktrace.o abs_lowcore.o
+obj-y	+= smp.o text_amode31.o stacktrace.o abs_lowcore.o facility.o
 
 extra-y				+= vmlinux.lds
 
diff --git a/arch/s390/kernel/facility.c b/arch/s390/kernel/facility.c
new file mode 100644
index 000000000000..5e80a4f65363
--- /dev/null
+++ b/arch/s390/kernel/facility.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright IBM Corp. 2023
+ */
+
+#include <asm/facility.h>
+
+unsigned int stfle_size(void)
+{
+	static unsigned int size;
+	u64 dummy;
+	unsigned int r;
+
+	r = READ_ONCE(size);
+	if (!r) {
+		r = __stfle_asm(&dummy, 1) + 1;
+		WRITE_ONCE(size, r);
+	}
+	return r;
+}
+EXPORT_SYMBOL(stfle_size);
diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index d989772fe211..c168e88c64da 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -19,6 +19,7 @@
 #include <asm/nmi.h>
 #include <asm/dis.h>
 #include <asm/fpu/api.h>
+#include <asm/facility.h>
 #include "kvm-s390.h"
 #include "gaccess.h"
 
@@ -990,11 +991,20 @@ static int handle_stfle(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
 	__u32 fac = READ_ONCE(vsie_page->scb_o->fac);
 
+	/*
+	 * Alternate-STFLE-Interpretive-Execution facilities are not supported
+	 * -> format-0 flcb
+	 */
 	if (fac && test_kvm_facility(vcpu->kvm, 7)) {
 		fac = fac & 0x7ffffff8U;
 		retry_vsie_icpt(vsie_page);
+		/*
+		 * format-0 -> size of nested guest's facility list == guest's size
+		 * guest's size == host's size, since STFLE is interpretatively executed
+		 * using a format-0 for the guest, too.
+		 */
 		if (read_guest_real(vcpu, fac, &vsie_page->fac,
-				    sizeof(vsie_page->fac)))
+				    stfle_size() * sizeof(u64)))
 			return set_validity_icpt(scb_s, 0x1090U);
 		scb_s->fac = (__u32)(__u64) &vsie_page->fac;
 	}
-- 
2.39.2

Re: [PATCH v2 2/4] KVM: s390: vsie: Fix length of facility list shadowed

[Claudio Imbrenda]

On Tue,  7 Nov 2023 13:31:16 +0100
Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:

[...]

> -obj-y	+= smp.o text_amode31.o stacktrace.o abs_lowcore.o
> +obj-y	+= smp.o text_amode31.o stacktrace.o abs_lowcore.o facility.o
>  
>  extra-y				+= vmlinux.lds
>  
> diff --git a/arch/s390/kernel/facility.c b/arch/s390/kernel/facility.c
> new file mode 100644
> index 000000000000..5e80a4f65363
> --- /dev/null
> +++ b/arch/s390/kernel/facility.c

I wonder if this is the right place for this?

This function seems to be used only for vsie, maybe you can just move
it to vsie.c? or do you think it will be used elsewhere too?

[...]

Re: [PATCH v2 2/4] KVM: s390: vsie: Fix length of facility list shadowed

[Nina Schoetterl-Glausch]

On Tue, 2023-11-07 at 18:11 +0100, Claudio Imbrenda wrote:
> On Tue,  7 Nov 2023 13:31:16 +0100
> Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:
> 
> [...]
> 
> > -obj-y	+= smp.o text_amode31.o stacktrace.o abs_lowcore.o
> > +obj-y	+= smp.o text_amode31.o stacktrace.o abs_lowcore.o facility.o
> >  
> >  extra-y				+= vmlinux.lds
> >  
> > diff --git a/arch/s390/kernel/facility.c b/arch/s390/kernel/facility.c
> > new file mode 100644
> > index 000000000000..5e80a4f65363
> > --- /dev/null
> > +++ b/arch/s390/kernel/facility.c
> 
> I wonder if this is the right place for this?

I've wondered the same :D
> 
> This function seems to be used only for vsie, maybe you can just move
> it to vsie.c? or do you think it will be used elsewhere too?

It's a general STFLE function and if I put it into vsie.c I'm not sure
that, if the same functionality was required somewhere else, it would be
found and moved to a common location.

I was also somewhat resistant to calling a double underscore function from
vsie.c. Of course I could implement it with my own inline asm...

The way I did it seemed nicest, but if someone else has a strong opinion
I'll defer to that.
> 
> [...]

Re: [PATCH v2 2/4] KVM: s390: vsie: Fix length of facility list shadowed

[Heiko Carstens]

On Wed, Nov 08, 2023 at 11:30:09AM +0100, Nina Schoetterl-Glausch wrote:
> On Tue, 2023-11-07 at 18:11 +0100, Claudio Imbrenda wrote:
> > On Tue,  7 Nov 2023 13:31:16 +0100
> > Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:
> > 
> > [...]
> > 
> > > -obj-y	+= smp.o text_amode31.o stacktrace.o abs_lowcore.o
> > > +obj-y	+= smp.o text_amode31.o stacktrace.o abs_lowcore.o facility.o
> > >  
> > >  extra-y				+= vmlinux.lds
> > >  
> > > diff --git a/arch/s390/kernel/facility.c b/arch/s390/kernel/facility.c
> > > new file mode 100644
> > > index 000000000000..5e80a4f65363
> > > --- /dev/null
> > > +++ b/arch/s390/kernel/facility.c
> > 
> > I wonder if this is the right place for this?
> 
> I've wondered the same :D
> > 
> > This function seems to be used only for vsie, maybe you can just move
> > it to vsie.c? or do you think it will be used elsewhere too?
> 
> It's a general STFLE function and if I put it into vsie.c I'm not sure
> that, if the same functionality was required somewhere else, it would be
> found and moved to a common location.
> 
> I was also somewhat resistant to calling a double underscore function from
> vsie.c. Of course I could implement it with my own inline asm...
> 
> The way I did it seemed nicest, but if someone else has a strong opinion
> I'll defer to that.

I think it is ok to have new file for just this. It is better than what
we've done too often in the past: dump new functionality to some more or
less random file instead. The usual victim would have been setup.c.

So I prefer a new file, even if we end up with only one function there.

Re: [PATCH v2 2/4] KVM: s390: vsie: Fix length of facility list shadowed

[Claudio Imbrenda]

On Tue,  7 Nov 2023 13:31:16 +0100
Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:

[...]

> diff --git a/arch/s390/kernel/facility.c b/arch/s390/kernel/facility.c
> new file mode 100644
> index 000000000000..5e80a4f65363
> --- /dev/null
> +++ b/arch/s390/kernel/facility.c
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright IBM Corp. 2023
> + */
> +
> +#include <asm/facility.h>
> +
> +unsigned int stfle_size(void)
> +{
> +	static unsigned int size;
> +	u64 dummy;
> +	unsigned int r;

reverse Christmas tree please :)


with that fixed:

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

[...]

Re: [PATCH v2 2/4] KVM: s390: vsie: Fix length of facility list shadowed

[Nina Schoetterl-Glausch]

On Wed, 2023-11-08 at 12:23 +0100, Claudio Imbrenda wrote:
> On Tue,  7 Nov 2023 13:31:16 +0100
> Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:
> 
> [...]
> 
> > diff --git a/arch/s390/kernel/facility.c b/arch/s390/kernel/facility.c
> > new file mode 100644
> > index 000000000000..5e80a4f65363
> > --- /dev/null
> > +++ b/arch/s390/kernel/facility.c
> > @@ -0,0 +1,21 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright IBM Corp. 2023
> > + */
> > +
> > +#include <asm/facility.h>
> > +
> > +unsigned int stfle_size(void)
> > +{
> > +	static unsigned int size;
> > +	u64 dummy;
> > +	unsigned int r;
> 
> reverse Christmas tree please :)

Might be an opportunity to clear that up for me.
AFAIK reverse christmas tree isn't universally enforced in the kernel.
Do we do it in generic s390 code? I know we do for s390 kvm.
Personally I don't quite get the rational, but I don't care much either :)
Heiko?

> with that fixed:
> 
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> 
> [...]

Re: [PATCH v2 2/4] KVM: s390: vsie: Fix length of facility list shadowed

[Heiko Carstens]

On Wed, Nov 08, 2023 at 12:49:21PM +0100, Nina Schoetterl-Glausch wrote:
> On Wed, 2023-11-08 at 12:23 +0100, Claudio Imbrenda wrote:
> > On Tue,  7 Nov 2023 13:31:16 +0100
> > Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:
> > 
> > [...]
> > > +unsigned int stfle_size(void)
> > > +{
> > > +	static unsigned int size;
> > > +	u64 dummy;
> > > +	unsigned int r;
> > 
> > reverse Christmas tree please :)
> 
> Might be an opportunity to clear that up for me.
> AFAIK reverse christmas tree isn't universally enforced in the kernel.
> Do we do it in generic s390 code? I know we do for s390 kvm.
> Personally I don't quite get the rational, but I don't care much either :)
> Heiko?

We do that for _new_ code in s390 code, yes.

[PATCH v2 3/4] KVM: s390: cpu model: Use proper define for facility mask size

[Nina Schoetterl-Glausch]

Use the previously unused S390_ARCH_FAC_MASK_SIZE_U64 instead of
S390_ARCH_FAC_LIST_SIZE_U64 for defining the fac_mask array.
Note that both values are the same, there is no functional change.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 arch/s390/include/asm/kvm_host.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 427f9528a7b6..46fcd2f9dff8 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -811,7 +811,7 @@ struct s390_io_adapter {
 
 struct kvm_s390_cpu_model {
 	/* facility mask supported by kvm & hosting machine */
-	__u64 fac_mask[S390_ARCH_FAC_LIST_SIZE_U64];
+	__u64 fac_mask[S390_ARCH_FAC_MASK_SIZE_U64];
 	struct kvm_s390_vm_cpu_subfunc subfuncs;
 	/* facility list requested by guest (in dma page) */
 	__u64 *fac_list;
-- 
2.39.2

Re: [PATCH v2 3/4] KVM: s390: cpu model: Use proper define for facility mask size

[Claudio Imbrenda]

On Tue,  7 Nov 2023 13:31:17 +0100
Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:

> Use the previously unused S390_ARCH_FAC_MASK_SIZE_U64 instead of
> S390_ARCH_FAC_LIST_SIZE_U64 for defining the fac_mask array.
> Note that both values are the same, there is no functional change.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  arch/s390/include/asm/kvm_host.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 427f9528a7b6..46fcd2f9dff8 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -811,7 +811,7 @@ struct s390_io_adapter {
>  
>  struct kvm_s390_cpu_model {
>  	/* facility mask supported by kvm & hosting machine */
> -	__u64 fac_mask[S390_ARCH_FAC_LIST_SIZE_U64];
> +	__u64 fac_mask[S390_ARCH_FAC_MASK_SIZE_U64];
>  	struct kvm_s390_vm_cpu_subfunc subfuncs;
>  	/* facility list requested by guest (in dma page) */
>  	__u64 *fac_list;

[PATCH v2 4/4] KVM: s390: Minor refactor of base/ext facility lists

[Nina Schoetterl-Glausch]

Directly use the size of the arrays instead of going through the
indirection of kvm_s390_fac_size().
Don't use magic number for the number of entries in the non hypervisor
managed facility bit mask list.
Make the constraint of that number on kvm_s390_fac_base obvious.
Get rid of implicit double anding of stfle_fac_list.

Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---

Notes:
    I think it's nicer this way but it might be needless churn.

 arch/s390/kvm/kvm-s390.c | 44 +++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 25 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index b3f17e014cab..e00ab2f38c89 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -217,33 +217,25 @@ static int async_destroy = 1;
 module_param(async_destroy, int, 0444);
 MODULE_PARM_DESC(async_destroy, "Asynchronous destroy for protected guests");
 
-/*
- * For now we handle at most 16 double words as this is what the s390 base
- * kernel handles and stores in the prefix page. If we ever need to go beyond
- * this, this requires changes to code, but the external uapi can stay.
- */
-#define SIZE_INTERNAL 16
-
+#define HMFAI_DWORDS 16
 /*
  * Base feature mask that defines default mask for facilities. Consists of the
  * defines in FACILITIES_KVM and the non-hypervisor managed bits.
  */
-static unsigned long kvm_s390_fac_base[SIZE_INTERNAL] = { FACILITIES_KVM };
+static unsigned long kvm_s390_fac_base[HMFAI_DWORDS] = { FACILITIES_KVM };
+static_assert(ARRAY_SIZE(((long[]){ FACILITIES_KVM })) <= HMFAI_DWORDS);
+static_assert(ARRAY_SIZE(kvm_s390_fac_base) <= S390_ARCH_FAC_MASK_SIZE_U64);
+static_assert(ARRAY_SIZE(kvm_s390_fac_base) <= S390_ARCH_FAC_LIST_SIZE_U64);
+static_assert(ARRAY_SIZE(kvm_s390_fac_base) <= ARRAY_SIZE(stfle_fac_list));
+
 /*
  * Extended feature mask. Consists of the defines in FACILITIES_KVM_CPUMODEL
  * and defines the facilities that can be enabled via a cpu model.
  */
-static unsigned long kvm_s390_fac_ext[SIZE_INTERNAL] = { FACILITIES_KVM_CPUMODEL };
-
-static unsigned long kvm_s390_fac_size(void)
-{
-	BUILD_BUG_ON(SIZE_INTERNAL > S390_ARCH_FAC_MASK_SIZE_U64);
-	BUILD_BUG_ON(SIZE_INTERNAL > S390_ARCH_FAC_LIST_SIZE_U64);
-	BUILD_BUG_ON(SIZE_INTERNAL * sizeof(unsigned long) >
-		sizeof(stfle_fac_list));
-
-	return SIZE_INTERNAL;
-}
+static const unsigned long kvm_s390_fac_ext[] = { FACILITIES_KVM_CPUMODEL };
+static_assert(ARRAY_SIZE(kvm_s390_fac_ext) <= S390_ARCH_FAC_MASK_SIZE_U64);
+static_assert(ARRAY_SIZE(kvm_s390_fac_ext) <= S390_ARCH_FAC_LIST_SIZE_U64);
+static_assert(ARRAY_SIZE(kvm_s390_fac_ext) <= ARRAY_SIZE(stfle_fac_list));
 
 /* available cpu features supported by kvm */
 static DECLARE_BITMAP(kvm_s390_available_cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
@@ -3341,13 +3333,16 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	kvm->arch.sie_page2->kvm = kvm;
 	kvm->arch.model.fac_list = kvm->arch.sie_page2->fac_list;
 
-	for (i = 0; i < kvm_s390_fac_size(); i++) {
+	for (i = 0; i < ARRAY_SIZE(kvm_s390_fac_base); i++) {
 		kvm->arch.model.fac_mask[i] = stfle_fac_list[i] &
-					      (kvm_s390_fac_base[i] |
-					       kvm_s390_fac_ext[i]);
+					      kvm_s390_fac_base[i];
 		kvm->arch.model.fac_list[i] = stfle_fac_list[i] &
 					      kvm_s390_fac_base[i];
 	}
+	for (i = 0; i < ARRAY_SIZE(kvm_s390_fac_ext); i++) {
+		kvm->arch.model.fac_mask[i] |= stfle_fac_list[i] &
+					       kvm_s390_fac_ext[i];
+	}
 	kvm->arch.model.subfuncs = kvm_s390_available_subfunc;
 
 	/* we are always in czam mode - even on pre z14 machines */
@@ -5859,9 +5854,8 @@ static int __init kvm_s390_init(void)
 		return -EINVAL;
 	}
 
-	for (i = 0; i < 16; i++)
-		kvm_s390_fac_base[i] |=
-			stfle_fac_list[i] & nonhyp_mask(i);
+	for (i = 0; i < HMFAI_DWORDS; i++)
+		kvm_s390_fac_base[i] |= nonhyp_mask(i);
 
 	r = __kvm_s390_init();
 	if (r)
-- 
2.39.2

Re: [PATCH v2 4/4] KVM: s390: Minor refactor of base/ext facility lists

[Claudio Imbrenda]

On Tue,  7 Nov 2023 13:31:18 +0100
Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:

> Directly use the size of the arrays instead of going through the
> indirection of kvm_s390_fac_size().
> Don't use magic number for the number of entries in the non hypervisor
> managed facility bit mask list.
> Make the constraint of that number on kvm_s390_fac_base obvious.
> Get rid of implicit double anding of stfle_fac_list.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
> 
> Notes:
>     I think it's nicer this way but it might be needless churn.
> 
>  arch/s390/kvm/kvm-s390.c | 44 +++++++++++++++++-----------------------
>  1 file changed, 19 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index b3f17e014cab..e00ab2f38c89 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -217,33 +217,25 @@ static int async_destroy = 1;
>  module_param(async_destroy, int, 0444);
>  MODULE_PARM_DESC(async_destroy, "Asynchronous destroy for protected guests");
>  
> -/*
> - * For now we handle at most 16 double words as this is what the s390 base
> - * kernel handles and stores in the prefix page. If we ever need to go beyond
> - * this, this requires changes to code, but the external uapi can stay.
> - */
> -#define SIZE_INTERNAL 16
> -
> +#define HMFAI_DWORDS 16
>  /*
>   * Base feature mask that defines default mask for facilities. Consists of the
>   * defines in FACILITIES_KVM and the non-hypervisor managed bits.
>   */
> -static unsigned long kvm_s390_fac_base[SIZE_INTERNAL] = { FACILITIES_KVM };
> +static unsigned long kvm_s390_fac_base[HMFAI_DWORDS] = { FACILITIES_KVM };
> +static_assert(ARRAY_SIZE(((long[]){ FACILITIES_KVM })) <= HMFAI_DWORDS);
> +static_assert(ARRAY_SIZE(kvm_s390_fac_base) <= S390_ARCH_FAC_MASK_SIZE_U64);
> +static_assert(ARRAY_SIZE(kvm_s390_fac_base) <= S390_ARCH_FAC_LIST_SIZE_U64);
> +static_assert(ARRAY_SIZE(kvm_s390_fac_base) <= ARRAY_SIZE(stfle_fac_list));
> +
>  /*
>   * Extended feature mask. Consists of the defines in FACILITIES_KVM_CPUMODEL
>   * and defines the facilities that can be enabled via a cpu model.
>   */
> -static unsigned long kvm_s390_fac_ext[SIZE_INTERNAL] = { FACILITIES_KVM_CPUMODEL };
> -
> -static unsigned long kvm_s390_fac_size(void)
> -{
> -	BUILD_BUG_ON(SIZE_INTERNAL > S390_ARCH_FAC_MASK_SIZE_U64);
> -	BUILD_BUG_ON(SIZE_INTERNAL > S390_ARCH_FAC_LIST_SIZE_U64);
> -	BUILD_BUG_ON(SIZE_INTERNAL * sizeof(unsigned long) >
> -		sizeof(stfle_fac_list));
> -
> -	return SIZE_INTERNAL;
> -}
> +static const unsigned long kvm_s390_fac_ext[] = { FACILITIES_KVM_CPUMODEL };
> +static_assert(ARRAY_SIZE(kvm_s390_fac_ext) <= S390_ARCH_FAC_MASK_SIZE_U64);
> +static_assert(ARRAY_SIZE(kvm_s390_fac_ext) <= S390_ARCH_FAC_LIST_SIZE_U64);
> +static_assert(ARRAY_SIZE(kvm_s390_fac_ext) <= ARRAY_SIZE(stfle_fac_list));
>  
>  /* available cpu features supported by kvm */
>  static DECLARE_BITMAP(kvm_s390_available_cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
> @@ -3341,13 +3333,16 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>  	kvm->arch.sie_page2->kvm = kvm;
>  	kvm->arch.model.fac_list = kvm->arch.sie_page2->fac_list;
>  
> -	for (i = 0; i < kvm_s390_fac_size(); i++) {
> +	for (i = 0; i < ARRAY_SIZE(kvm_s390_fac_base); i++) {
>  		kvm->arch.model.fac_mask[i] = stfle_fac_list[i] &
> -					      (kvm_s390_fac_base[i] |
> -					       kvm_s390_fac_ext[i]);
> +					      kvm_s390_fac_base[i];
>  		kvm->arch.model.fac_list[i] = stfle_fac_list[i] &
>  					      kvm_s390_fac_base[i];
>  	}
> +	for (i = 0; i < ARRAY_SIZE(kvm_s390_fac_ext); i++) {
> +		kvm->arch.model.fac_mask[i] |= stfle_fac_list[i] &
> +					       kvm_s390_fac_ext[i];
> +	}
>  	kvm->arch.model.subfuncs = kvm_s390_available_subfunc;
>  
>  	/* we are always in czam mode - even on pre z14 machines */
> @@ -5859,9 +5854,8 @@ static int __init kvm_s390_init(void)
>  		return -EINVAL;
>  	}
>  
> -	for (i = 0; i < 16; i++)
> -		kvm_s390_fac_base[i] |=
> -			stfle_fac_list[i] & nonhyp_mask(i);
> +	for (i = 0; i < HMFAI_DWORDS; i++)
> +		kvm_s390_fac_base[i] |= nonhyp_mask(i);
>  
>  	r = __kvm_s390_init();
>  	if (r)