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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx>
>
