| Message ID | 20180219125249.16457-1-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19.02.2018 13:52, Christian Borntraeger wrote: > Some facilities should only provided to the guest, if these are be provided > enabled by a CPU model. This allows to avoid capabilities and > simply fallback to the cpumodel for deciding about a facility > without enabling it for older QEMUs or QEMUs without a CPU > model. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > arch/s390/kvm/kvm-s390.c | 54 ++++++++++++++++++++++++++-------------- > arch/s390/kvm/kvm-s390.h | 2 -- > arch/s390/tools/gen_facilities.c | 20 +++++++++++++++ > 3 files changed, 55 insertions(+), 21 deletions(-) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index ba4c7092335a..9c05767c95be 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -151,13 +151,34 @@ static int nested; > module_param(nested, int, S_IRUGO); > MODULE_PARM_DESC(nested, "Nested virtualization support"); > > -/* upper facilities limit for kvm */ > -unsigned long kvm_s390_fac_list_mask[16] = { FACILITIES_KVM }; > > -unsigned long kvm_s390_fac_list_mask_size(void) > +/* > + * For now we handle not more than 16 double words as this is what > + * the s390 base kernel handles and stores in 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 > + > +/* > + * 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 }; > +/* > + * 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(ARRAY_SIZE(kvm_s390_fac_list_mask) > S390_ARCH_FAC_MASK_SIZE_U64); > - return ARRAY_SIZE(kvm_s390_fac_list_mask); > + 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(S390_lowcore.stfle_fac_list)); > + > + return SIZE_INTERNAL; > } > > /* available cpu features supported by kvm */ > @@ -1942,20 +1963,15 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > if (!kvm->arch.sie_page2) > goto out_err; > > - /* Populate the facility mask initially. */ > - memcpy(kvm->arch.model.fac_mask, S390_lowcore.stfle_fac_list, > - sizeof(S390_lowcore.stfle_fac_list)); > - for (i = 0; i < S390_ARCH_FAC_LIST_SIZE_U64; i++) { > - if (i < kvm_s390_fac_list_mask_size()) > - kvm->arch.model.fac_mask[i] &= kvm_s390_fac_list_mask[i]; > - else > - kvm->arch.model.fac_mask[i] = 0UL; > - } > - > - /* Populate the facility list initially. */ > kvm->arch.model.fac_list = kvm->arch.sie_page2->fac_list; > - memcpy(kvm->arch.model.fac_list, kvm->arch.model.fac_mask, > - S390_ARCH_FAC_LIST_SIZE_BYTE); > + > + for (i = 0; i < kvm_s390_fac_size(); i++) { > + kvm->arch.model.fac_mask[i] = S390_lowcore.stfle_fac_list[i] & > + (kvm_s390_fac_base[i] | > + kvm_s390_fac_ext[i]); > + kvm->arch.model.fac_list[i] = S390_lowcore.stfle_fac_list[i] & > + kvm_s390_fac_base[i]; > + } > > /* we are always in czam mode - even on pre z14 machines */ > set_kvm_facility(kvm->arch.model.fac_mask, 138); > @@ -4031,7 +4047,7 @@ static int __init kvm_s390_init(void) > } > > for (i = 0; i < 16; i++) > - kvm_s390_fac_list_mask[i] |= > + kvm_s390_fac_base[i] |= > S390_lowcore.stfle_fac_list[i] & nonhyp_mask(i); > > return kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE); > diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h > index bd31b37b0e6f..77b141e6fa4e 100644 > --- a/arch/s390/kvm/kvm-s390.h > +++ b/arch/s390/kvm/kvm-s390.h > @@ -297,8 +297,6 @@ void exit_sie(struct kvm_vcpu *vcpu); > void kvm_s390_sync_request(int req, struct kvm_vcpu *vcpu); > int kvm_s390_vcpu_setup_cmma(struct kvm_vcpu *vcpu); > void kvm_s390_vcpu_unsetup_cmma(struct kvm_vcpu *vcpu); > -unsigned long kvm_s390_fac_list_mask_size(void); > -extern unsigned long kvm_s390_fac_list_mask[]; > void kvm_s390_set_cpu_timer(struct kvm_vcpu *vcpu, __u64 cputm); > __u64 kvm_s390_get_cpu_timer(struct kvm_vcpu *vcpu); > > diff --git a/arch/s390/tools/gen_facilities.c b/arch/s390/tools/gen_facilities.c > index 424a1ba4f874..37a77a3f280c 100644 > --- a/arch/s390/tools/gen_facilities.c > +++ b/arch/s390/tools/gen_facilities.c > @@ -62,6 +62,13 @@ static struct facility_def facility_defs[] = { > } > }, > { > + /* > + * FACILITIES_KVM contains the list of facilities that are > + * part of the default facility mask and list passed to the > + * initial CPU model. If no CPU model is used this, together > + * with the non-hypervisor managed bits, is the maximum list > + * of guest facilities supported by KVM. > + */ > .name = "FACILITIES_KVM", > .bits = (int[]){ > 0, /* N3 instructions */ > @@ -89,6 +96,19 @@ static struct facility_def facility_defs[] = { > -1 /* END */ > } > }, > + { > + /* > + * FACILITIES_KVM_CPUMODEL contains the list of facilities > + * that can be enabled by CPU model code if the host supports > + * it. These facilities are not passed to the guest without > + * CPU model support. > + */ > + > + .name = "FACILITIES_KVM_CPUMODEL", > + .bits = (int[]){ > + -1 /* END */ > + } > + }, > }; > > static void print_facility_list(struct facility_def *def) > Looks good to me. Reviewed-by: David Hildenbrand <david@redhat.com>
On Mon, 19 Feb 2018 12:52:49 +0000 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > Some facilities should only provided to the guest, if these are s/provided/be provided/ s/these/they/ > enabled by a CPU model. This allows to avoid capabilities and s/allows/allows us/ > simply fallback to the cpumodel for deciding about a facility s/simply fallback/to simply fall back/ > without enabling it for older QEMUs or QEMUs without a CPU > model. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > arch/s390/kvm/kvm-s390.c | 54 ++++++++++++++++++++++++++-------------- > arch/s390/kvm/kvm-s390.h | 2 -- > arch/s390/tools/gen_facilities.c | 20 +++++++++++++++ > 3 files changed, 55 insertions(+), 21 deletions(-) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index ba4c7092335a..9c05767c95be 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -151,13 +151,34 @@ static int nested; > module_param(nested, int, S_IRUGO); > MODULE_PARM_DESC(nested, "Nested virtualization support"); > > -/* upper facilities limit for kvm */ > -unsigned long kvm_s390_fac_list_mask[16] = { FACILITIES_KVM }; > > -unsigned long kvm_s390_fac_list_mask_size(void) > +/* > + * For now we handle not more than 16 double words as this is what "For now, we handle at most..." > + * the s390 base kernel handles and stores in prefix page. If we s/in prefix page/in the prefix page/ > + * ever need to go beyond this, this requires changes to code, but > + * the external uapi can stay. > + */ > +#define SIZE_INTERNAL 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 }; > +/* > + * 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 }; > + (...) > diff --git a/arch/s390/tools/gen_facilities.c b/arch/s390/tools/gen_facilities.c > index 424a1ba4f874..37a77a3f280c 100644 > --- a/arch/s390/tools/gen_facilities.c > +++ b/arch/s390/tools/gen_facilities.c > @@ -62,6 +62,13 @@ static struct facility_def facility_defs[] = { > } > }, > { > + /* > + * FACILITIES_KVM contains the list of facilities that are > + * part of the default facility mask and list passed to the "mask and list that are passed..." (I stumbled over this, even if the sentence is fine...) > + * initial CPU model. If no CPU model is used this, together s/is used this,/is used, this,/ > + * with the non-hypervisor managed bits, is the maximum list > + * of guest facilities supported by KVM. > + */ > .name = "FACILITIES_KVM", > .bits = (int[]){ > 0, /* N3 instructions */ > @@ -89,6 +96,19 @@ static struct facility_def facility_defs[] = { > -1 /* END */ > } > }, > + { > + /* > + * FACILITIES_KVM_CPUMODEL contains the list of facilities > + * that can be enabled by CPU model code if the host supports > + * it. These facilities are not passed to the guest without > + * CPU model support. > + */ > + > + .name = "FACILITIES_KVM_CPUMODEL", > + .bits = (int[]){ > + -1 /* END */ > + } > + }, > }; > > static void print_facility_list(struct facility_def *def) Only nitpickery, else Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On 02/19/2018 05:08 PM, Cornelia Huck wrote: > Only nitpickery, else Thanks a lot. Still not as perfect as Erik Blake, but your spell checking skills are much better than mine ;-) > > Reviewed-by: Cornelia Huck <cohuck@redhat.com> >
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index ba4c7092335a..9c05767c95be 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -151,13 +151,34 @@ static int nested; module_param(nested, int, S_IRUGO); MODULE_PARM_DESC(nested, "Nested virtualization support"); -/* upper facilities limit for kvm */ -unsigned long kvm_s390_fac_list_mask[16] = { FACILITIES_KVM }; -unsigned long kvm_s390_fac_list_mask_size(void) +/* + * For now we handle not more than 16 double words as this is what + * the s390 base kernel handles and stores in 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 + +/* + * 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 }; +/* + * 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(ARRAY_SIZE(kvm_s390_fac_list_mask) > S390_ARCH_FAC_MASK_SIZE_U64); - return ARRAY_SIZE(kvm_s390_fac_list_mask); + 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(S390_lowcore.stfle_fac_list)); + + return SIZE_INTERNAL; } /* available cpu features supported by kvm */ @@ -1942,20 +1963,15 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) if (!kvm->arch.sie_page2) goto out_err; - /* Populate the facility mask initially. */ - memcpy(kvm->arch.model.fac_mask, S390_lowcore.stfle_fac_list, - sizeof(S390_lowcore.stfle_fac_list)); - for (i = 0; i < S390_ARCH_FAC_LIST_SIZE_U64; i++) { - if (i < kvm_s390_fac_list_mask_size()) - kvm->arch.model.fac_mask[i] &= kvm_s390_fac_list_mask[i]; - else - kvm->arch.model.fac_mask[i] = 0UL; - } - - /* Populate the facility list initially. */ kvm->arch.model.fac_list = kvm->arch.sie_page2->fac_list; - memcpy(kvm->arch.model.fac_list, kvm->arch.model.fac_mask, - S390_ARCH_FAC_LIST_SIZE_BYTE); + + for (i = 0; i < kvm_s390_fac_size(); i++) { + kvm->arch.model.fac_mask[i] = S390_lowcore.stfle_fac_list[i] & + (kvm_s390_fac_base[i] | + kvm_s390_fac_ext[i]); + kvm->arch.model.fac_list[i] = S390_lowcore.stfle_fac_list[i] & + kvm_s390_fac_base[i]; + } /* we are always in czam mode - even on pre z14 machines */ set_kvm_facility(kvm->arch.model.fac_mask, 138); @@ -4031,7 +4047,7 @@ static int __init kvm_s390_init(void) } for (i = 0; i < 16; i++) - kvm_s390_fac_list_mask[i] |= + kvm_s390_fac_base[i] |= S390_lowcore.stfle_fac_list[i] & nonhyp_mask(i); return kvm_init(NULL, sizeof(struct kvm_vcpu), 0, THIS_MODULE); diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h index bd31b37b0e6f..77b141e6fa4e 100644 --- a/arch/s390/kvm/kvm-s390.h +++ b/arch/s390/kvm/kvm-s390.h @@ -297,8 +297,6 @@ void exit_sie(struct kvm_vcpu *vcpu); void kvm_s390_sync_request(int req, struct kvm_vcpu *vcpu); int kvm_s390_vcpu_setup_cmma(struct kvm_vcpu *vcpu); void kvm_s390_vcpu_unsetup_cmma(struct kvm_vcpu *vcpu); -unsigned long kvm_s390_fac_list_mask_size(void); -extern unsigned long kvm_s390_fac_list_mask[]; void kvm_s390_set_cpu_timer(struct kvm_vcpu *vcpu, __u64 cputm); __u64 kvm_s390_get_cpu_timer(struct kvm_vcpu *vcpu); diff --git a/arch/s390/tools/gen_facilities.c b/arch/s390/tools/gen_facilities.c index 424a1ba4f874..37a77a3f280c 100644 --- a/arch/s390/tools/gen_facilities.c +++ b/arch/s390/tools/gen_facilities.c @@ -62,6 +62,13 @@ static struct facility_def facility_defs[] = { } }, { + /* + * FACILITIES_KVM contains the list of facilities that are + * part of the default facility mask and list passed to the + * initial CPU model. If no CPU model is used this, together + * with the non-hypervisor managed bits, is the maximum list + * of guest facilities supported by KVM. + */ .name = "FACILITIES_KVM", .bits = (int[]){ 0, /* N3 instructions */ @@ -89,6 +96,19 @@ static struct facility_def facility_defs[] = { -1 /* END */ } }, + { + /* + * FACILITIES_KVM_CPUMODEL contains the list of facilities + * that can be enabled by CPU model code if the host supports + * it. These facilities are not passed to the guest without + * CPU model support. + */ + + .name = "FACILITIES_KVM_CPUMODEL", + .bits = (int[]){ + -1 /* END */ + } + }, }; static void print_facility_list(struct facility_def *def)
Some facilities should only provided to the guest, if these are enabled by a CPU model. This allows to avoid capabilities and simply fallback to the cpumodel for deciding about a facility without enabling it for older QEMUs or QEMUs without a CPU model. Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> --- arch/s390/kvm/kvm-s390.c | 54 ++++++++++++++++++++++++++-------------- arch/s390/kvm/kvm-s390.h | 2 -- arch/s390/tools/gen_facilities.c | 20 +++++++++++++++ 3 files changed, 55 insertions(+), 21 deletions(-)