| Message ID | 20190221184304.42805-2-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: small prep patches for cpu model | expand |
On 21.02.19 19:43, Christian Borntraeger wrote: > While we will not implement interception for query functions yet, we can > and should disable functions that have a control bit based on the given > CPU model. > > Let us start with enabling the subfunction interface. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > arch/s390/include/asm/kvm_host.h | 1 + > arch/s390/kvm/kvm-s390.c | 34 +++++++++++++++++++++----------- > 2 files changed, 23 insertions(+), 12 deletions(-) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index d5d24889c3bcf..a042861554507 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -712,6 +712,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]; > + struct kvm_s390_vm_cpu_subfunc subfuncs; > /* facility list requested by guest (in dma page) */ > __u64 *fac_list; > u64 cpuid; > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 7f4bc58a53b97..5058fe58ece77 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -1258,11 +1258,20 @@ static int kvm_s390_set_processor_feat(struct kvm *kvm, > static int kvm_s390_set_processor_subfunc(struct kvm *kvm, > struct kvm_device_attr *attr) > { > - /* > - * Once supported by kernel + hw, we have to store the subfunctions > - * in kvm->arch and remember that user space configured them. > - */ > - return -ENXIO; > + mutex_lock(&kvm->lock); > + if (kvm->created_vcpus) { > + mutex_unlock(&kvm->lock); > + return -EBUSY; > + } > + > + if (copy_from_user(&kvm->arch.model.subfuncs, (void __user *)attr->addr, > + sizeof(struct kvm_s390_vm_cpu_subfunc))) { > + mutex_unlock(&kvm->lock); > + return -EFAULT; > + } Wonder if we want to allow setting features the host does not have. I guess we want to (similar to stfle). > + mutex_unlock(&kvm->lock); > + > + return 0; > } > > static int kvm_s390_set_cpu_model(struct kvm *kvm, struct kvm_device_attr *attr) > @@ -1381,12 +1390,11 @@ static int kvm_s390_get_machine_feat(struct kvm *kvm, > static int kvm_s390_get_processor_subfunc(struct kvm *kvm, > struct kvm_device_attr *attr) > { > - /* > - * Once we can actually configure subfunctions (kernel + hw support), > - * we have to check if they were already set by user space, if so copy > - * them from kvm->arch. > - */ > - return -ENXIO; > + if (copy_to_user((void __user *)attr->addr, &kvm->arch.model.subfuncs, > + sizeof(struct kvm_s390_vm_cpu_subfunc))) > + return -EFAULT; > + > + return 0; > } > > static int kvm_s390_get_machine_subfunc(struct kvm *kvm, > @@ -1397,6 +1405,7 @@ static int kvm_s390_get_machine_subfunc(struct kvm *kvm, > return -EFAULT; > return 0; > } > + > static int kvm_s390_get_cpu_model(struct kvm *kvm, struct kvm_device_attr *attr) > { > int ret = -ENXIO; > @@ -1514,10 +1523,10 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr) > case KVM_S390_VM_CPU_PROCESSOR_FEAT: > case KVM_S390_VM_CPU_MACHINE_FEAT: > case KVM_S390_VM_CPU_MACHINE_SUBFUNC: > + case KVM_S390_VM_CPU_PROCESSOR_SUBFUNC: > ret = 0; > break; > /* configuring subfunctions is not supported yet */ You want to drop that comment as well I think. > - case KVM_S390_VM_CPU_PROCESSOR_SUBFUNC: > default: > ret = -ENXIO; > break; > @@ -2218,6 +2227,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > kvm->arch.model.fac_list[i] = S390_lowcore.stfle_fac_list[i] & > kvm_s390_fac_base[i]; > } > + kvm->arch.model.subfuncs = kvm_s390_available_subfunc; > > /* we are always in czam mode - even on pre z14 machines */ > set_kvm_facility(kvm->arch.model.fac_mask, 138); > Reviewed-by: David Hildenbrand <david@redhat.com>
On 21.02.2019 20:09, David Hildenbrand wrote: > On 21.02.19 19:43, Christian Borntraeger wrote: >> While we will not implement interception for query functions yet, we can >> and should disable functions that have a control bit based on the given >> CPU model. >> >> Let us start with enabling the subfunction interface. >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> arch/s390/include/asm/kvm_host.h | 1 + >> arch/s390/kvm/kvm-s390.c | 34 +++++++++++++++++++++----------- >> 2 files changed, 23 insertions(+), 12 deletions(-) >> >> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >> index d5d24889c3bcf..a042861554507 100644 >> --- a/arch/s390/include/asm/kvm_host.h >> +++ b/arch/s390/include/asm/kvm_host.h >> @@ -712,6 +712,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]; >> + struct kvm_s390_vm_cpu_subfunc subfuncs; >> /* facility list requested by guest (in dma page) */ >> __u64 *fac_list; >> u64 cpuid; >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index 7f4bc58a53b97..5058fe58ece77 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -1258,11 +1258,20 @@ static int kvm_s390_set_processor_feat(struct kvm *kvm, >> static int kvm_s390_set_processor_subfunc(struct kvm *kvm, >> struct kvm_device_attr *attr) >> { >> - /* >> - * Once supported by kernel + hw, we have to store the subfunctions >> - * in kvm->arch and remember that user space configured them. >> - */ >> - return -ENXIO; >> + mutex_lock(&kvm->lock); >> + if (kvm->created_vcpus) { >> + mutex_unlock(&kvm->lock); >> + return -EBUSY; >> + } >> + >> + if (copy_from_user(&kvm->arch.model.subfuncs, (void __user *)attr->addr, >> + sizeof(struct kvm_s390_vm_cpu_subfunc))) { >> + mutex_unlock(&kvm->lock); >> + return -EFAULT; >> + } > > Wonder if we want to allow setting features the host does not have. I > guess we want to (similar to stfle). Right now we allow this and I think this is ok. > >> + mutex_unlock(&kvm->lock); >> + >> + return 0; >> } >> >> static int kvm_s390_set_cpu_model(struct kvm *kvm, struct kvm_device_attr *attr) >> @@ -1381,12 +1390,11 @@ static int kvm_s390_get_machine_feat(struct kvm *kvm, >> static int kvm_s390_get_processor_subfunc(struct kvm *kvm, >> struct kvm_device_attr *attr) >> { >> - /* >> - * Once we can actually configure subfunctions (kernel + hw support), >> - * we have to check if they were already set by user space, if so copy >> - * them from kvm->arch. >> - */ >> - return -ENXIO; >> + if (copy_to_user((void __user *)attr->addr, &kvm->arch.model.subfuncs, >> + sizeof(struct kvm_s390_vm_cpu_subfunc))) >> + return -EFAULT; >> + >> + return 0; >> } >> >> static int kvm_s390_get_machine_subfunc(struct kvm *kvm, >> @@ -1397,6 +1405,7 @@ static int kvm_s390_get_machine_subfunc(struct kvm *kvm, >> return -EFAULT; >> return 0; >> } >> + >> static int kvm_s390_get_cpu_model(struct kvm *kvm, struct kvm_device_attr *attr) >> { >> int ret = -ENXIO; >> @@ -1514,10 +1523,10 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr) >> case KVM_S390_VM_CPU_PROCESSOR_FEAT: >> case KVM_S390_VM_CPU_MACHINE_FEAT: >> case KVM_S390_VM_CPU_MACHINE_SUBFUNC: >> + case KVM_S390_VM_CPU_PROCESSOR_SUBFUNC: >> ret = 0; >> break; >> /* configuring subfunctions is not supported yet */ > > You want to drop that comment as well I think. yes, will drop. > > >> - case KVM_S390_VM_CPU_PROCESSOR_SUBFUNC: >> default: >> ret = -ENXIO; >> break; >> @@ -2218,6 +2227,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) >> kvm->arch.model.fac_list[i] = S390_lowcore.stfle_fac_list[i] & >> kvm_s390_fac_base[i]; >> } >> + kvm->arch.model.subfuncs = kvm_s390_available_subfunc; >> >> /* we are always in czam mode - even on pre z14 machines */ >> set_kvm_facility(kvm->arch.model.fac_mask, 138); >> > > Reviewed-by: David Hildenbrand <david@redhat.com> >
On Thu, 21 Feb 2019 19:43:03 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > While we will not implement interception for query functions yet, we can > and should disable functions that have a control bit based on the given > CPU model. > > Let us start with enabling the subfunction interface. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > arch/s390/include/asm/kvm_host.h | 1 + > arch/s390/kvm/kvm-s390.c | 34 +++++++++++++++++++++----------- > 2 files changed, 23 insertions(+), 12 deletions(-) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 7f4bc58a53b97..5058fe58ece77 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -1258,11 +1258,20 @@ static int kvm_s390_set_processor_feat(struct kvm *kvm, > static int kvm_s390_set_processor_subfunc(struct kvm *kvm, > struct kvm_device_attr *attr) > { > - /* > - * Once supported by kernel + hw, we have to store the subfunctions > - * in kvm->arch and remember that user space configured them. > - */ > - return -ENXIO; > + mutex_lock(&kvm->lock); > + if (kvm->created_vcpus) { > + mutex_unlock(&kvm->lock); > + return -EBUSY; > + } > + > + if (copy_from_user(&kvm->arch.model.subfuncs, (void __user *)attr->addr, > + sizeof(struct kvm_s390_vm_cpu_subfunc))) { Is there any need for sanitizing/checking the data provided by userspace before sticking it into our internal structures? Not that we do anything useful with it yet :) > + mutex_unlock(&kvm->lock); > + return -EFAULT; > + } > + mutex_unlock(&kvm->lock); > + > + return 0; > }
On 22.02.2019 09:48, Cornelia Huck wrote: > On Thu, 21 Feb 2019 19:43:03 +0100 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> While we will not implement interception for query functions yet, we can >> and should disable functions that have a control bit based on the given >> CPU model. >> >> Let us start with enabling the subfunction interface. >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> arch/s390/include/asm/kvm_host.h | 1 + >> arch/s390/kvm/kvm-s390.c | 34 +++++++++++++++++++++----------- >> 2 files changed, 23 insertions(+), 12 deletions(-) >> > >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index 7f4bc58a53b97..5058fe58ece77 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -1258,11 +1258,20 @@ static int kvm_s390_set_processor_feat(struct kvm *kvm, >> static int kvm_s390_set_processor_subfunc(struct kvm *kvm, >> struct kvm_device_attr *attr) >> { >> - /* >> - * Once supported by kernel + hw, we have to store the subfunctions >> - * in kvm->arch and remember that user space configured them. >> - */ >> - return -ENXIO; >> + mutex_lock(&kvm->lock); >> + if (kvm->created_vcpus) { >> + mutex_unlock(&kvm->lock); >> + return -EBUSY; >> + } >> + >> + if (copy_from_user(&kvm->arch.model.subfuncs, (void __user *)attr->addr, >> + sizeof(struct kvm_s390_vm_cpu_subfunc))) { > > Is there any need for sanitizing/checking the data provided by > userspace before sticking it into our internal structures? Not that we > do anything useful with it yet :) No, its more like facility bits. Its the value that is returned by the query function of several instructions. If QEMU wants to lie then the guest might misbehave but the host kernel is fine. > >> + mutex_unlock(&kvm->lock); >> + return -EFAULT; >> + } >> + mutex_unlock(&kvm->lock); >> + >> + return 0; >> } >
On Fri, 22 Feb 2019 10:07:53 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 22.02.2019 09:48, Cornelia Huck wrote: > > On Thu, 21 Feb 2019 19:43:03 +0100 > > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > >> While we will not implement interception for query functions yet, we can > >> and should disable functions that have a control bit based on the given > >> CPU model. > >> > >> Let us start with enabling the subfunction interface. > >> > >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > >> --- > >> arch/s390/include/asm/kvm_host.h | 1 + > >> arch/s390/kvm/kvm-s390.c | 34 +++++++++++++++++++++----------- > >> 2 files changed, 23 insertions(+), 12 deletions(-) > >> > > > >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > >> index 7f4bc58a53b97..5058fe58ece77 100644 > >> --- a/arch/s390/kvm/kvm-s390.c > >> +++ b/arch/s390/kvm/kvm-s390.c > >> @@ -1258,11 +1258,20 @@ static int kvm_s390_set_processor_feat(struct kvm *kvm, > >> static int kvm_s390_set_processor_subfunc(struct kvm *kvm, > >> struct kvm_device_attr *attr) > >> { > >> - /* > >> - * Once supported by kernel + hw, we have to store the subfunctions > >> - * in kvm->arch and remember that user space configured them. > >> - */ > >> - return -ENXIO; > >> + mutex_lock(&kvm->lock); > >> + if (kvm->created_vcpus) { > >> + mutex_unlock(&kvm->lock); > >> + return -EBUSY; > >> + } > >> + > >> + if (copy_from_user(&kvm->arch.model.subfuncs, (void __user *)attr->addr, > >> + sizeof(struct kvm_s390_vm_cpu_subfunc))) { > > > > Is there any need for sanitizing/checking the data provided by > > userspace before sticking it into our internal structures? Not that we > > do anything useful with it yet :) > > No, its more like facility bits. Its the value that is returned by the query > function of several instructions. If QEMU wants to lie then the guest might > misbehave but the host kernel is fine. Fair enough, if wonky data cannot affect the host negatively. > > > > >> + mutex_unlock(&kvm->lock); > >> + return -EFAULT; > >> + } > >> + mutex_unlock(&kvm->lock); > >> + > >> + return 0; > >> } > > >
On Thu, 21 Feb 2019 19:43:03 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > While we will not implement interception for query functions yet, we can > and should disable functions that have a control bit based on the given > CPU model. > > Let us start with enabling the subfunction interface. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > arch/s390/include/asm/kvm_host.h | 1 + > arch/s390/kvm/kvm-s390.c | 34 +++++++++++++++++++++----------- > 2 files changed, 23 insertions(+), 12 deletions(-) Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On 21.02.19 19:43, Christian Borntraeger wrote: > While we will not implement interception for query functions yet, we can > and should disable functions that have a control bit based on the given > CPU model. > > Let us start with enabling the subfunction interface. Always wondered when we'll use these interfaces :-) Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h index d5d24889c3bcf..a042861554507 100644 --- a/arch/s390/include/asm/kvm_host.h +++ b/arch/s390/include/asm/kvm_host.h @@ -712,6 +712,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]; + struct kvm_s390_vm_cpu_subfunc subfuncs; /* facility list requested by guest (in dma page) */ __u64 *fac_list; u64 cpuid; diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index 7f4bc58a53b97..5058fe58ece77 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -1258,11 +1258,20 @@ static int kvm_s390_set_processor_feat(struct kvm *kvm, static int kvm_s390_set_processor_subfunc(struct kvm *kvm, struct kvm_device_attr *attr) { - /* - * Once supported by kernel + hw, we have to store the subfunctions - * in kvm->arch and remember that user space configured them. - */ - return -ENXIO; + mutex_lock(&kvm->lock); + if (kvm->created_vcpus) { + mutex_unlock(&kvm->lock); + return -EBUSY; + } + + if (copy_from_user(&kvm->arch.model.subfuncs, (void __user *)attr->addr, + sizeof(struct kvm_s390_vm_cpu_subfunc))) { + mutex_unlock(&kvm->lock); + return -EFAULT; + } + mutex_unlock(&kvm->lock); + + return 0; } static int kvm_s390_set_cpu_model(struct kvm *kvm, struct kvm_device_attr *attr) @@ -1381,12 +1390,11 @@ static int kvm_s390_get_machine_feat(struct kvm *kvm, static int kvm_s390_get_processor_subfunc(struct kvm *kvm, struct kvm_device_attr *attr) { - /* - * Once we can actually configure subfunctions (kernel + hw support), - * we have to check if they were already set by user space, if so copy - * them from kvm->arch. - */ - return -ENXIO; + if (copy_to_user((void __user *)attr->addr, &kvm->arch.model.subfuncs, + sizeof(struct kvm_s390_vm_cpu_subfunc))) + return -EFAULT; + + return 0; } static int kvm_s390_get_machine_subfunc(struct kvm *kvm, @@ -1397,6 +1405,7 @@ static int kvm_s390_get_machine_subfunc(struct kvm *kvm, return -EFAULT; return 0; } + static int kvm_s390_get_cpu_model(struct kvm *kvm, struct kvm_device_attr *attr) { int ret = -ENXIO; @@ -1514,10 +1523,10 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr) case KVM_S390_VM_CPU_PROCESSOR_FEAT: case KVM_S390_VM_CPU_MACHINE_FEAT: case KVM_S390_VM_CPU_MACHINE_SUBFUNC: + case KVM_S390_VM_CPU_PROCESSOR_SUBFUNC: ret = 0; break; /* configuring subfunctions is not supported yet */ - case KVM_S390_VM_CPU_PROCESSOR_SUBFUNC: default: ret = -ENXIO; break; @@ -2218,6 +2227,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) kvm->arch.model.fac_list[i] = S390_lowcore.stfle_fac_list[i] & kvm_s390_fac_base[i]; } + kvm->arch.model.subfuncs = kvm_s390_available_subfunc; /* we are always in czam mode - even on pre z14 machines */ set_kvm_facility(kvm->arch.model.fac_mask, 138);
While we will not implement interception for query functions yet, we can and should disable functions that have a control bit based on the given CPU model. Let us start with enabling the subfunction interface. Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> --- arch/s390/include/asm/kvm_host.h | 1 + arch/s390/kvm/kvm-s390.c | 34 +++++++++++++++++++++----------- 2 files changed, 23 insertions(+), 12 deletions(-)