Fix two bugs in the STFLE vsie implementation.
The first concerns the identification if the guest is intending
to use interpretive execution for STFLE for its guest.
The second fixes too much of the guests memory being accessed when
shadowing.
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 previously unused constant
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 | 18 +++++++++++++
arch/s390/kvm/kvm-s390.c | 44 ++++++++++++++------------------
arch/s390/kvm/vsie.c | 15 +++++++++--
6 files changed, 58 insertions(+), 29 deletions(-)
create mode 100644 arch/s390/kernel/facility.c
base-commit: 05d3ef8bba77c1b5f98d941d8b2d4aeab8118ef1
--
2.39.2
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")
Signed-off-by: Nina Schoetterl-Glausch <[email protected]>
---
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
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.
Fixes: 66b630d5b7f2 ("KVM: s390: vsie: support STFLE interpretation")
Signed-off-by: Nina Schoetterl-Glausch <[email protected]>
---
arch/s390/include/asm/facility.h | 6 ++++++
arch/s390/kernel/Makefile | 2 +-
arch/s390/kernel/facility.c | 18 ++++++++++++++++++
arch/s390/kvm/vsie.c | 12 +++++++++++-
4 files changed, 36 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..c33a95a562da
--- /dev/null
+++ b/arch/s390/kernel/facility.c
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright IBM Corp. 2023
+ */
+
+#include <asm/facility.h>
+
+unsigned int stfle_size(void)
+{
+ static unsigned int size = 0;
+ u64 dummy;
+
+ if (!size) {
+ size = __stfle_asm(&dummy, 1) + 1;
+ }
+ return size;
+}
+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
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 <[email protected]>
---
I found it confusing before and 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
No point in defining a size for the mask if we're not going to use it.
Fixes: 9d8d578605b4 ("KVM: s390: use facilities and cpu_id per KVM")
Signed-off-by: Nina Schoetterl-Glausch <[email protected]>
---
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
On 03.11.23 18:30, Nina Schoetterl-Glausch wrote:
> 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")
> Signed-off-by: Nina Schoetterl-Glausch <[email protected]>
No access to documentation, but sounds plausible.
Acked-by: David Hildenbrand <[email protected]>
--
Cheers,
David / dhildenb
On 03.11.23 18:30, Nina Schoetterl-Glausch wrote:
> 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.
>
> Fixes: 66b630d5b7f2 ("KVM: s390: vsie: support STFLE interpretation")
> Signed-off-by: Nina Schoetterl-Glausch <[email protected]>
> ---
> arch/s390/include/asm/facility.h | 6 ++++++
> arch/s390/kernel/Makefile | 2 +-
> arch/s390/kernel/facility.c | 18 ++++++++++++++++++
> arch/s390/kvm/vsie.c | 12 +++++++++++-
> 4 files changed, 36 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..c33a95a562da
> --- /dev/null
> +++ b/arch/s390/kernel/facility.c
> @@ -0,0 +1,18 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright IBM Corp. 2023
> + */
> +
> +#include <asm/facility.h>
> +
> +unsigned int stfle_size(void)
> +{
> + static unsigned int size = 0;
> + u64 dummy;
> +
> + if (!size) {
> + size = __stfle_asm(&dummy, 1) + 1;
> + }
> + return size;
> +}
> +EXPORT_SYMBOL(stfle_size);
Possible races? Should have to use an atomic?
No access to documentation, but sounds plausible.
Acked-by: David Hildenbrand <[email protected]>
--
Cheers,
David / dhildenb
On 03.11.23 18:30, Nina Schoetterl-Glausch wrote:
> No point in defining a size for the mask if we're not going to use it.
I neither understand the patch description nor what the bug is that is
being fixed (and how that description relates to the patch
subject+description).
Please improve the patch description.
--
Cheers,
David / dhildenb
On 03.11.23 19:36, David Hildenbrand wrote:
> On 03.11.23 18:30, Nina Schoetterl-Glausch wrote:
>> No point in defining a size for the mask if we're not going to use it.
>
> I neither understand the patch description nor what the bug is that is
> being fixed (and how that description relates to the patch
> subject+description).
>
> Please improve the patch description.
>
Should this be
"
KVM: s390: cpu model: use proper define for facility mask size
We're using S390_ARCH_FAC_LIST_SIZE_U64 instead of
S390_ARCH_FAC_MASK_SIZE_U64 to define the array size of the facility
mask. Let's properly use S390_ARCH_FAC_MASK_SIZE_U64. Note that both
values are the same and, therefore, this is a pure cleanup.
"
I'm not convinced there is a bug and that this deserves a "Fixes:".
--
Cheers,
David / dhildenb
On Fri, 3 Nov 2023 18:30:05 +0100
Nina Schoetterl-Glausch <[email protected]> wrote:
> 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")
> Signed-off-by: Nina Schoetterl-Glausch <[email protected]>
Reviewed-by: Claudio Imbrenda <[email protected]>
> ---
> 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)))
On Fri, 3 Nov 2023 18:30:08 +0100
Nina Schoetterl-Glausch <[email protected]> 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 <[email protected]>
> ---
>
>
> I found it confusing before and think it's nicer this way but
> it might be needless churn.
some things are probably overkill
>
>
> 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 };
this was sized to [SIZE_INTERNAL], now it doesn't have a fixed size. is
this intentional?
> +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];
> + }
I like it better when it's all in one place, instead of having two loops
> 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);
where did the stfle_fac_list[i] go?
>
> r = __kvm_s390_init();
> if (r)
On Fri, 2023-11-03 at 19:41 +0100, David Hildenbrand wrote:
> On 03.11.23 19:36, David Hildenbrand wrote:
> > On 03.11.23 18:30, Nina Schoetterl-Glausch wrote:
> > > No point in defining a size for the mask if we're not going to use it.
> >
> > I neither understand the patch description nor what the bug is that is
> > being fixed (and how that description relates to the patch
> > subject+description).
> >
> > Please improve the patch description.
> >
>
> Should this be
>
> "
> KVM: s390: cpu model: use proper define for facility mask size
>
> We're using S390_ARCH_FAC_LIST_SIZE_U64 instead of
> S390_ARCH_FAC_MASK_SIZE_U64 to define the array size of the facility
> mask. Let's properly use S390_ARCH_FAC_MASK_SIZE_U64. Note that both
> values are the same and, therefore, this is a pure cleanup.
> "
>
> I'm not convinced there is a bug and that this deserves a "Fixes:".
Oh yeah, sorry, purely a cleanup. S390_ARCH_FAC_MASK_SIZE_U64 wasn't
used anywhere. I also considered just getting rid of it and using one
constant for both list and mask.
On Fri, 2023-11-03 at 19:32 +0100, Claudio Imbrenda wrote:
> On Fri, 3 Nov 2023 18:30:08 +0100
> Nina Schoetterl-Glausch <[email protected]> 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 <[email protected]>
> > ---
> >
> >
> > I found it confusing before and think it's nicer this way but
> > it might be needless churn.
>
> some things are probably overkill
I mostly wanted to get rid of kvm_s390_fac_size() since it's meaning
wasn't all that clear IMO. It's not the size of the host's facility list,
it's not the size of the guest's facility list (same as host right now,
but could be different in the future) it's the size of the arrays.
But like I said, none of it is necessary and while I'm reasonably sure
I didn't break anything, you never know :).
[...]
> > 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
[...]
> > /*
> > * 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 };
>
> this was sized to [SIZE_INTERNAL], now it doesn't have a fixed size. is
> this intentional?
Yes, it's as big as it needs to be, that way it cannot be too small, so one
less thing to consider.
[...]
> > /* 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];
> > + }
>
> I like it better when it's all in one place, instead of having two loops
Hmm, it's the result of the arrays being different lengths now.
[...]
> > - 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);
>
> where did the stfle_fac_list[i] go?
I deleted it. That's what I meant by "Get rid of implicit double
anding of stfle_fac_list".
Besides it being redundant I didn't like it conceptually.
kvm_s390_fac_base specifies the facilities we support, regardless
if they're installed in the configuration. The hypervisor managed
ones are unconditionally declared via FACILITIES_KVM and we can add
the non hypervisor managed ones unconditionally, too.
> > r = __kvm_s390_init();
> > if (r)
>
On Mon, 06 Nov 2023 12:38:55 +0100
Nina Schoetterl-Glausch <[email protected]> wrote:
[...]
> > this was sized to [SIZE_INTERNAL], now it doesn't have a fixed size. is
> > this intentional?
>
> Yes, it's as big as it needs to be, that way it cannot be too small, so one
> less thing to consider.
fair enough
> [...]
> > > /* 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];
> > > + }
> >
> > I like it better when it's all in one place, instead of having two loops
>
> Hmm, it's the result of the arrays being different lengths now.
ah, I had missed that, the names are very similar.
>
> [...]
>
> > > - 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);
> >
> > where did the stfle_fac_list[i] go?
>
> I deleted it. That's what I meant by "Get rid of implicit double
> anding of stfle_fac_list".
> Besides it being redundant I didn't like it conceptually.
> kvm_s390_fac_base specifies the facilities we support, regardless
> if they're installed in the configuration. The hypervisor managed
> ones are unconditionally declared via FACILITIES_KVM and we can add
> the non hypervisor managed ones unconditionally, too.
makes sense
>
> > > r = __kvm_s390_init();
> > > if (r)
> >
>
On Fri, 2023-11-03 at 19:34 +0100, David Hildenbrand wrote:
> On 03.11.23 18:30, Nina Schoetterl-Glausch wrote:
> > 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.
> >
> > Fixes: 66b630d5b7f2 ("KVM: s390: vsie: support STFLE interpretation")
> > Signed-off-by: Nina Schoetterl-Glausch <[email protected]>
> > ---
> > arch/s390/include/asm/facility.h | 6 ++++++
> > arch/s390/kernel/Makefile | 2 +-
> > arch/s390/kernel/facility.c | 18 ++++++++++++++++++
> > arch/s390/kvm/vsie.c | 12 +++++++++++-
> > 4 files changed, 36 insertions(+), 2 deletions(-)
> > create mode 100644 arch/s390/kernel/facility.c
[...]
> > diff --git a/arch/s390/kernel/facility.c b/arch/s390/kernel/facility.c
[...]
> > +#include <asm/facility.h>
> > +
> > +unsigned int stfle_size(void)
> > +{
> > + static unsigned int size = 0;
> > + u64 dummy;
> > +
> > + if (!size) {
> > + size = __stfle_asm(&dummy, 1) + 1;
> > + }
> > + return size;
> > +}
> > +EXPORT_SYMBOL(stfle_size);
>
> Possible races? Should have to use an atomic?
Good point. Calling __stfle_asm multiple times is fine
and AFAIK torn reads/writes aren't possible. I don't see a way
for the compiler to break things either.
But it might indeed be nicer to use an atomic, without
any downsides.
>
> No access to documentation, but sounds plausible.
>
> Acked-by: David Hildenbrand <[email protected]>
Thanks!
On Mon, Nov 06, 2023 at 02:06:22PM +0100, Nina Schoetterl-Glausch wrote:
> > > +unsigned int stfle_size(void)
> > > +{
> > > + static unsigned int size = 0;
> > > + u64 dummy;
> > > +
> > > + if (!size) {
> > > + size = __stfle_asm(&dummy, 1) + 1;
> > > + }
Please get rid of the braces here. checkpatch.pl with "--strict" should
complain too, I guess.
> > Possible races? Should have to use an atomic?
>
> Good point. Calling __stfle_asm multiple times is fine
> and AFAIK torn reads/writes aren't possible. I don't see a way
> for the compiler to break things either.
> But it might indeed be nicer to use an atomic, without
> any downsides.
Please use WRITE_ONCE() and READ_ONCE(); that's more than sufficient here.