Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <03c79e51-7a0b-f406-d4d2-b10f43b6a7a1@linux.ibm.com>
Date:   Tue, 28 Jun 2022 18:41:59 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.10.0
Subject: Re: [PATCH v10 3/3] KVM: s390: resetting the Topology-Change-Report
Content-Language: en-US
To:     Pierre Morel <pmorel@linux.ibm.com>, kvm@vger.kernel.org
Cc:     linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
        borntraeger@de.ibm.com, frankja@linux.ibm.com, cohuck@redhat.com,
        david@redhat.com, thuth@redhat.com, imbrenda@linux.ibm.com,
        hca@linux.ibm.com, gor@linux.ibm.com, wintera@linux.ibm.com,
        seiden@linux.ibm.com, nrb@linux.ibm.com
References: <20220620125437.37122-1-pmorel@linux.ibm.com>
 <20220620125437.37122-4-pmorel@linux.ibm.com>
From:   Janis Schoetterl-Glausch <scgl@linux.ibm.com>
In-Reply-To: <20220620125437.37122-4-pmorel@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 6/20/22 14:54, 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, we give userland the possibility to
> query the MTCR state.
> 
> We indicate KVM support for the CPU topology facility with a new
> KVM capability: KVM_CAP_S390_CPU_TOPOLOGY.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  Documentation/virt/kvm/api.rst   | 31 +++++++++++
>  arch/s390/include/uapi/asm/kvm.h | 10 ++++
>  arch/s390/kvm/kvm-s390.c         | 96 ++++++++++++++++++++++++++++++++
>  include/uapi/linux/kvm.h         |  1 +
>  4 files changed, 138 insertions(+)
> 
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 11e00a46c610..326f8b7e7671 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -7956,6 +7956,37 @@ should adjust CPUID leaf 0xA to reflect that the PMU is disabled.
>  When enabled, KVM will exit to userspace with KVM_EXIT_SYSTEM_EVENT of
>  type KVM_SYSTEM_EVENT_SUSPEND to process the guest suspend request.
> 
> +8.37 KVM_CAP_S390_CPU_TOPOLOGY
> +------------------------------
> +
> +:Capability: KVM_CAP_S390_CPU_TOPOLOGY
> +:Architectures: s390
> +:Type: vm
> +
> +This capability indicates that KVM will provide the S390 CPU Topology
> +facility which consist of the interpretation of the PTF instruction for
> +the Function Code 2 along with interception and forwarding of both the
> +PTF instruction with Function Codes 0 or 1 and the STSI(15,1,x)
> +instruction to the userland hypervisor.

The way the code is written, STSI 15.x.x is forwarded to user space,
might actually make sense to future proof the code by restricting that
to 15.1.2-6 in priv.c.
> +
> +The stfle facility 11, CPU Topology facility, should not be provided
> +to the guest without this capability.
> +
> +When this capability is present, KVM provides a new attribute group
> +on vm fd, KVM_S390_VM_CPU_TOPOLOGY.
> +This new attribute allows to get, set or clear the Modified Change
> +Topology Report (MTCR) bit of the SCA through the kvm_device_attr
> +structure.
> +
> +Getting the MTCR bit is realized by using a kvm_device_attr attr
> +entry value of KVM_GET_DEVICE_ATTR and with kvm_device_attr addr
> +entry pointing to the address of a struct kvm_cpu_topology.
> +The value of the MTCR is return by the bit mtcr of the structure.
> +
> +When using KVM_SET_DEVICE_ATTR the MTCR is set by using the
> +attr->attr value KVM_S390_VM_CPU_TOPO_MTCR_SET and cleared by
> +using KVM_S390_VM_CPU_TOPO_MTCR_CLEAR.
> +
>  9. Known KVM API problems
>  =========================
> 
> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
> index 7a6b14874d65..df5e8279ffd0 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,15 @@ 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_MTCR_CLEAR	0
> +#define KVM_S390_VM_CPU_TOPO_MTCR_SET	1

Are you going to transition to a set-value-provided-by-user API with the next series?
I don't particularly like that MTCR is user visible, it's kind of an implementation detail.

> +
> +struct kvm_cpu_topology {
> +	__u16 mtcr : 1;

So I'd give this a more descriptive name, report_topology_change/topo_change_report_pending ?

> +	__u16 reserved : 15;

Are these bits for future proofing? If so a few more would do no harm IMO.
> +};

The use of a bit field in uapi surprised my, but I guess it's fine and kvm_sync_regs has them too.
> +
>  /* 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 95b96019ca8e..ae39041bb149 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -606,6 +606,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_S390_PROTECTED:
>  		r = is_prot_virt_host();
>  		break;
> +	case KVM_CAP_S390_CPU_TOPOLOGY:
> +		r = test_facility(11);
> +		break;
>  	default:
>  		r = 0;
>  	}
> @@ -817,6 +820,20 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap)
>  		icpt_operexc_on_all_vcpus(kvm);
>  		r = 0;
>  		break;
> +	case KVM_CAP_S390_CPU_TOPOLOGY:
> +		r = -EINVAL;
> +		mutex_lock(&kvm->lock);
> +		if (kvm->created_vcpus) {
> +			r = -EBUSY;
> +		} else if (test_facility(11)) {
> +			set_kvm_facility(kvm->arch.model.fac_mask, 11);
> +			set_kvm_facility(kvm->arch.model.fac_list, 11);
> +			r = 0;
> +		}
> +		mutex_unlock(&kvm->lock);
> +		VM_EVENT(kvm, 3, "ENABLE: CPU TOPOLOGY %s",

Most of the other cases spell out the cap, so it'd be "ENABLE: CAP_S390_CPU_TOPOLOGY %s".

> +			 r ? "(not available)" : "(success)");
> +		break;
>  	default:
>  		r = -EINVAL;
>  		break;
> @@ -1710,6 +1727,76 @@ static void kvm_s390_sca_set_mtcr(struct kvm *kvm)
>  	ipte_unlock(kvm);
>  }
> 

Some brainstorming function names:

kvm_s390_get_topo_change_report
kvm_s390_(un|re)set_topo_change_report
kvm_s390_(publish|revoke|unpublish)_topo_change_report
kvm_s390_(report|signal|revoke)_topology_change

[...]