| Message ID | 20231103173008.630217-5-nsg@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: Fix minor bugs in STFLE shadowing | expand |
On Fri, 3 Nov 2023 18:30:08 +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> > --- > > > 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:32 +0100, Claudio Imbrenda wrote: > On Fri, 3 Nov 2023 18:30:08 +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> > > --- > > > > > > 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 <nsg@linux.ibm.com> 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) > > >
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)
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> --- 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(-)