On 20.04.22 13:34, Pierre Morel wrote:
> During a subsystem reset the Topology-Change-Report is cleared.
> Let's give userland the possibility to clear the MTCR in the case
> of a subsystem reset.
>
> To migrate the MTCR, let's give userland the possibility to
> query the MTCR state.
>
> Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
> ---
> arch/s390/include/uapi/asm/kvm.h | 9 +++
> arch/s390/kvm/kvm-s390.c | 103 +++++++++++++++++++++++++++++++
> 2 files changed, 112 insertions(+)
>
> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
> index 7a6b14874d65..bb3df6d49f27 100644
> --- a/arch/s390/include/uapi/asm/kvm.h
> +++ b/arch/s390/include/uapi/asm/kvm.h
> @@ -74,6 +74,7 @@ struct kvm_s390_io_adapter_req {
> #define KVM_S390_VM_CRYPTO 2
> #define KVM_S390_VM_CPU_MODEL 3
> #define KVM_S390_VM_MIGRATION 4
> +#define KVM_S390_VM_CPU_TOPOLOGY 5
>
> /* kvm attributes for mem_ctrl */
> #define KVM_S390_VM_MEM_ENABLE_CMMA 0
> @@ -171,6 +172,14 @@ struct kvm_s390_vm_cpu_subfunc {
> #define KVM_S390_VM_MIGRATION_START 1
> #define KVM_S390_VM_MIGRATION_STATUS 2
>
> +/* kvm attributes for cpu topology */
> +#define KVM_S390_VM_CPU_TOPO_MTR_CLEAR 0
> +#define KVM_S390_VM_CPU_TOPO_MTR_SET 1
> +
> +struct kvm_s390_cpu_topology {
> + __u16 mtcr;
> +};
Just wondering:
1) Do we really need a struct for that
2) Do we want to leave some room for later expansion?
> +
> /* for KVM_GET_REGS and KVM_SET_REGS */
> struct kvm_regs {
> /* general purpose regs for s390 */
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 925ccc59f283..755f325c9e70 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1756,6 +1756,100 @@ static int kvm_s390_sca_set_mtcr(struct kvm *kvm)
> return 0;
> }
>
> +/**
> + * kvm_s390_sca_clear_mtcr
> + * @kvm: guest KVM description
> + *
> + * Is only relevant if the topology facility is present,
> + * the caller should check KVM facility 11
> + *
> + * Updates the Multiprocessor Topology-Change-Report to signal
> + * the guest with a topology change.
> + */
> +static int kvm_s390_sca_clear_mtcr(struct kvm *kvm)
> +{
> + struct bsca_block *sca = kvm->arch.sca;
> + struct kvm_vcpu *vcpu;
> + int val;
> +
> + vcpu = kvm_s390_get_first_vcpu(kvm);
> + if (!vcpu)
> + return -ENODEV;
It would be cleaner to have ipte_lock/ipte_unlock variants that are
independent of a vcpu.
Instead of checking for "vcpu->arch.sie_block->eca & ECA_SII" we might
just check for sclp.has_siif. Everything else that performs the
lock/unlock should be contained in "struct kvm" directly, unless I am
missing something.
[...]
> +
> +static int kvm_s390_get_topology(struct kvm *kvm, struct kvm_device_attr *attr)
> +{
> + struct kvm_s390_cpu_topology *topology;
> + int ret = 0;
> +
> + if (!test_kvm_facility(kvm, 11))
> + return -ENXIO;
> +
> + topology = kzalloc(sizeof(*topology), GFP_KERNEL);
> + if (!topology)
> + return -ENOMEM;
I'm confused. We're allocating a __u16 to then free it again below? Why
not simply use a value on the stack like in kvm_s390_vm_get_migration()?
u16 mtcr;
...
mtcr = kvm_s390_sca_get_mtcr(kvm);
if (copy_to_user((void __user *)attr->addr, &mtcr, sizeof(mtcr)))
return -EFAULT;
return 0;
> +
> + topology->mtcr = kvm_s390_sca_get_mtcr(kvm);
s/ / /
> + if (copy_to_user((void __user *)attr->addr, topology,
> + sizeof(struct kvm_s390_cpu_topology)))
> + ret = -EFAULT;
> +
> + kfree(topology);
> + return ret;
> +}
> +
--
Thanks,
David / dhildenb
