diff mbox series

[v9,3/3] s390x: KVM: resetting the Topology-Change-Report

Message ID 20220506092403.47406-4-pmorel@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series s390x: KVM: CPU Topology | expand

Commit Message

Pierre Morel May 6, 2022, 9:24 a.m. UTC
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@linux.ibm.com>
---
 arch/s390/include/uapi/asm/kvm.h |  5 ++
 arch/s390/kvm/kvm-s390.c         | 79 ++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

Comments

David Hildenbrand May 12, 2022, 9:31 a.m. UTC | #1
On 06.05.22 11:24, 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@linux.ibm.com>
> ---
>  arch/s390/include/uapi/asm/kvm.h |  5 ++
>  arch/s390/kvm/kvm-s390.c         | 79 ++++++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
> index 7a6b14874d65..abdcf4069343 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,10 @@ 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
> +
>  /* 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 c8bdce31464f..80a1244f0ead 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1731,6 +1731,76 @@ static void kvm_s390_sca_set_mtcr(struct kvm *kvm)
>  	ipte_unlock(kvm);
>  }
>  
> +/**
> + * 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 void kvm_s390_sca_clear_mtcr(struct kvm *kvm)
> +{
> +	struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
> +
> +	ipte_lock(kvm);
> +	sca->utility  &= ~SCA_UTILITY_MTCR;


One space too much.

sca->utility &= ~SCA_UTILITY_MTCR;

> +	ipte_unlock(kvm);
> +}
> +
> +static int kvm_s390_set_topology(struct kvm *kvm, struct kvm_device_attr *attr)
> +{
> +	if (!test_kvm_facility(kvm, 11))
> +		return -ENXIO;
> +
> +	switch (attr->attr) {
> +	case KVM_S390_VM_CPU_TOPO_MTR_SET:
> +		kvm_s390_sca_set_mtcr(kvm);
> +		break;
> +	case KVM_S390_VM_CPU_TOPO_MTR_CLEAR:
> +		kvm_s390_sca_clear_mtcr(kvm);
> +		break;
> +	}
> +	return 0;
> +}
> +
> +/**
> + * kvm_s390_sca_get_mtcr
> + * @kvm: guest KVM description
> + *
> + * Is only relevant if the topology facility is present,
> + * the caller should check KVM facility 11
> + *
> + * reports to QEMU the Multiprocessor Topology-Change-Report.
> + */
> +static int kvm_s390_sca_get_mtcr(struct kvm *kvm)
> +{
> +	struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
> +	int val;
> +
> +	ipte_lock(kvm);
> +	val = !!(sca->utility & SCA_UTILITY_MTCR);
> +	ipte_unlock(kvm);
> +
> +	return val;
> +}
> +
> +static int kvm_s390_get_topology(struct kvm *kvm, struct kvm_device_attr *attr)
> +{
> +	int mtcr;

I think we prefer something like u16 when copying to user space.

> +
> +	if (!test_kvm_facility(kvm, 11))
> +		return -ENXIO;
> +
> +	mtcr = kvm_s390_sca_get_mtcr(kvm);
> +	if (copy_to_user((void __user *)attr->addr, &mtcr, sizeof(mtcr)))
> +		return -EFAULT;
> +
> +	return 0;
> +}

You should probably add documentation, and document that only the last
bit (0x1) has a meaning.

Apart from that LGTM.
Claudio Imbrenda May 12, 2022, 9:52 a.m. UTC | #2
On Thu, 12 May 2022 11:31:18 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 06.05.22 11:24, 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@linux.ibm.com>
> > ---
> >  arch/s390/include/uapi/asm/kvm.h |  5 ++
> >  arch/s390/kvm/kvm-s390.c         | 79 ++++++++++++++++++++++++++++++++
> >  2 files changed, 84 insertions(+)
> > 
> > diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
> > index 7a6b14874d65..abdcf4069343 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,10 @@ 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
> > +
> >  /* 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 c8bdce31464f..80a1244f0ead 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -1731,6 +1731,76 @@ static void kvm_s390_sca_set_mtcr(struct kvm *kvm)
> >  	ipte_unlock(kvm);
> >  }
> >  
> > +/**
> > + * 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 void kvm_s390_sca_clear_mtcr(struct kvm *kvm)
> > +{
> > +	struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
> > +
> > +	ipte_lock(kvm);
> > +	sca->utility  &= ~SCA_UTILITY_MTCR;  
> 
> 
> One space too much.
> 
> sca->utility &= ~SCA_UTILITY_MTCR;
> 
> > +	ipte_unlock(kvm);
> > +}
> > +
> > +static int kvm_s390_set_topology(struct kvm *kvm, struct kvm_device_attr *attr)
> > +{
> > +	if (!test_kvm_facility(kvm, 11))
> > +		return -ENXIO;
> > +
> > +	switch (attr->attr) {
> > +	case KVM_S390_VM_CPU_TOPO_MTR_SET:
> > +		kvm_s390_sca_set_mtcr(kvm);
> > +		break;
> > +	case KVM_S390_VM_CPU_TOPO_MTR_CLEAR:
> > +		kvm_s390_sca_clear_mtcr(kvm);
> > +		break;
> > +	}
> > +	return 0;
> > +}
> > +
> > +/**
> > + * kvm_s390_sca_get_mtcr
> > + * @kvm: guest KVM description
> > + *
> > + * Is only relevant if the topology facility is present,
> > + * the caller should check KVM facility 11
> > + *
> > + * reports to QEMU the Multiprocessor Topology-Change-Report.
> > + */
> > +static int kvm_s390_sca_get_mtcr(struct kvm *kvm)
> > +{
> > +	struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
> > +	int val;
> > +
> > +	ipte_lock(kvm);
> > +	val = !!(sca->utility & SCA_UTILITY_MTCR);
> > +	ipte_unlock(kvm);
> > +
> > +	return val;
> > +}
> > +
> > +static int kvm_s390_get_topology(struct kvm *kvm, struct kvm_device_attr *attr)
> > +{
> > +	int mtcr;  
> 
> I think we prefer something like u16 when copying to user space.

but then userspace also has to expect a u16, right?

> 
> > +
> > +	if (!test_kvm_facility(kvm, 11))
> > +		return -ENXIO;
> > +
> > +	mtcr = kvm_s390_sca_get_mtcr(kvm);
> > +	if (copy_to_user((void __user *)attr->addr, &mtcr, sizeof(mtcr)))
> > +		return -EFAULT;
> > +
> > +	return 0;
> > +}  
> 
> You should probably add documentation, and document that only the last
> bit (0x1) has a meaning.
> 
> Apart from that LGTM.
>
David Hildenbrand May 12, 2022, 10:01 a.m. UTC | #3
>>
>> I think we prefer something like u16 when copying to user space.
> 
> but then userspace also has to expect a u16, right?

Yep.
Pierre Morel May 16, 2022, 10:36 a.m. UTC | #4
On 5/12/22 11:31, David Hildenbrand wrote:
> On 06.05.22 11:24, 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@linux.ibm.com>
>> ---
>>   arch/s390/include/uapi/asm/kvm.h |  5 ++
>>   arch/s390/kvm/kvm-s390.c         | 79 ++++++++++++++++++++++++++++++++
>>   2 files changed, 84 insertions(+)
>>
>> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
>> index 7a6b14874d65..abdcf4069343 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,10 @@ 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
>> +
>>   /* 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 c8bdce31464f..80a1244f0ead 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -1731,6 +1731,76 @@ static void kvm_s390_sca_set_mtcr(struct kvm *kvm)
>>   	ipte_unlock(kvm);
>>   }
>>   
>> +/**
>> + * 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 void kvm_s390_sca_clear_mtcr(struct kvm *kvm)
>> +{
>> +	struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
>> +
>> +	ipte_lock(kvm);
>> +	sca->utility  &= ~SCA_UTILITY_MTCR;
> 
> 
> One space too much.
> 
> sca->utility &= ~SCA_UTILITY_MTCR;
> 
>> +	ipte_unlock(kvm);
>> +}
>> +
>> +static int kvm_s390_set_topology(struct kvm *kvm, struct kvm_device_attr *attr)
>> +{
>> +	if (!test_kvm_facility(kvm, 11))
>> +		return -ENXIO;
>> +
>> +	switch (attr->attr) {
>> +	case KVM_S390_VM_CPU_TOPO_MTR_SET:
>> +		kvm_s390_sca_set_mtcr(kvm);
>> +		break;
>> +	case KVM_S390_VM_CPU_TOPO_MTR_CLEAR:
>> +		kvm_s390_sca_clear_mtcr(kvm);
>> +		break;
>> +	}
>> +	return 0;
>> +}
>> +
>> +/**
>> + * kvm_s390_sca_get_mtcr
>> + * @kvm: guest KVM description
>> + *
>> + * Is only relevant if the topology facility is present,
>> + * the caller should check KVM facility 11
>> + *
>> + * reports to QEMU the Multiprocessor Topology-Change-Report.
>> + */
>> +static int kvm_s390_sca_get_mtcr(struct kvm *kvm)
>> +{
>> +	struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
>> +	int val;
>> +
>> +	ipte_lock(kvm);
>> +	val = !!(sca->utility & SCA_UTILITY_MTCR);
>> +	ipte_unlock(kvm);
>> +
>> +	return val;
>> +}
>> +
>> +static int kvm_s390_get_topology(struct kvm *kvm, struct kvm_device_attr *attr)
>> +{
>> +	int mtcr;
> 
> I think we prefer something like u16 when copying to user space.

If you prefer.
The original idea was to have something like a bool but then I should 
have change get_mtcr to has_mtcr.


> 
>> +
>> +	if (!test_kvm_facility(kvm, 11))
>> +		return -ENXIO;
>> +
>> +	mtcr = kvm_s390_sca_get_mtcr(kvm);
>> +	if (copy_to_user((void __user *)attr->addr, &mtcr, sizeof(mtcr)))
>> +		return -EFAULT;
>> +
>> +	return 0;
>> +}
> 
> You should probably add documentation, and document that only the last
> bit (0x1) has a meaning.
> 
> Apart from that LGTM.
>
Pierre Morel May 16, 2022, 2:21 p.m. UTC | #5
On 5/12/22 12:01, David Hildenbrand wrote:
>>>
>>> I think we prefer something like u16 when copying to user space.
>>
>> but then userspace also has to expect a u16, right?
> 
> Yep.
> 

Yes but in fact, inspired by previous discussion I had on the VFIO 
interface, that is the reason why I did prefer an int.
It is much simpler than a u16 and the definition of a bit.

Despite a bit in a u16 is what the s3990 achitecture proposes I thought 
we could make it easier on the KVM/QEMU interface.

But if the discussion stops here, I will do as you both propose change 
to u16 in KVM and userland and add the documentation for the interface.

Regards,
Pierre
Pierre Morel May 18, 2022, 10:51 a.m. UTC | #6
On 5/12/22 11:31, David Hildenbrand wrote:
> On 06.05.22 11:24, 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@linux.ibm.com>
>> ---
>>   arch/s390/include/uapi/asm/kvm.h |  5 ++
>>   arch/s390/kvm/kvm-s390.c         | 79 ++++++++++++++++++++++++++++++++
>>   2 files changed, 84 insertions(+)
>>
>> diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
>> index 7a6b14874d65..abdcf4069343 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,10 @@ 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
>> +
>>   /* 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 c8bdce31464f..80a1244f0ead 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -1731,6 +1731,76 @@ static void kvm_s390_sca_set_mtcr(struct kvm *kvm)
>>   	ipte_unlock(kvm);
>>   }
>>   
>> +/**
>> + * 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 void kvm_s390_sca_clear_mtcr(struct kvm *kvm)
>> +{
>> +	struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
>> +
>> +	ipte_lock(kvm);
>> +	sca->utility  &= ~SCA_UTILITY_MTCR;
> 
> 
> One space too much.
> 
> sca->utility &= ~SCA_UTILITY_MTCR;
> 
>> +	ipte_unlock(kvm);
>> +}
>> +
>> +static int kvm_s390_set_topology(struct kvm *kvm, struct kvm_device_attr *attr)
>> +{
>> +	if (!test_kvm_facility(kvm, 11))
>> +		return -ENXIO;
>> +
>> +	switch (attr->attr) {
>> +	case KVM_S390_VM_CPU_TOPO_MTR_SET:
>> +		kvm_s390_sca_set_mtcr(kvm);
>> +		break;
>> +	case KVM_S390_VM_CPU_TOPO_MTR_CLEAR:
>> +		kvm_s390_sca_clear_mtcr(kvm);
>> +		break;
>> +	}
>> +	return 0;
>> +}
>> +
>> +/**
>> + * kvm_s390_sca_get_mtcr
>> + * @kvm: guest KVM description
>> + *
>> + * Is only relevant if the topology facility is present,
>> + * the caller should check KVM facility 11
>> + *
>> + * reports to QEMU the Multiprocessor Topology-Change-Report.
>> + */
>> +static int kvm_s390_sca_get_mtcr(struct kvm *kvm)
>> +{
>> +	struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
>> +	int val;
>> +
>> +	ipte_lock(kvm);
>> +	val = !!(sca->utility & SCA_UTILITY_MTCR);
>> +	ipte_unlock(kvm);
>> +
>> +	return val;
>> +}
>> +
>> +static int kvm_s390_get_topology(struct kvm *kvm, struct kvm_device_attr *attr)
>> +{
>> +	int mtcr;
> 
> I think we prefer something like u16 when copying to user space.

I come back here.
I think I prefer to keep the int.

the u16 is more than the MTCR but the entire utility field, so what 
should I do:

rename the function to kvm_s390_get_sca_utility() ?
and then should I modify the KVM_S390_VM_CPU_TOPOLOGY
to KVM_S390_VM_SCA_UTILITY ?

I do not like that, I do not think we should report/handle more 
information than expected/needed.

I can mask the MTCR bit and return a u16 with bit 0 (0x8000) set
but I find this a little weird

I admit an int is may be not optimal.
logically I should report a bool but I do not like to report a bool 
through the UAPI.

The more I think about it the more I think an int is OK.
Or in the case we want to spare memory space I can create a flag in a 
u16 but it should theoretically be different than the firmware MTCR bit. 
Could be 0x0001.
But still, it is only to leave during the copy_to_user where the copy of 
an int may be as good or better than the copy of a u16.

So any more opinion on this?

Regards,
Pierre

> 
>> +
>> +	if (!test_kvm_facility(kvm, 11))
>> +		return -ENXIO;
>> +
>> +	mtcr = kvm_s390_sca_get_mtcr(kvm);
>> +	if (copy_to_user((void __user *)attr->addr, &mtcr, sizeof(mtcr)))
>> +		return -EFAULT;
>> +
>> +	return 0;
>> +}
> 
> You should probably add documentation, and document that only the last
> bit (0x1) has a meaning.
> 
> Apart from that LGTM.
>
David Hildenbrand May 18, 2022, 2:33 p.m. UTC | #7
On 16.05.22 16:21, Pierre Morel wrote:
> 
> 
> On 5/12/22 12:01, David Hildenbrand wrote:
>>>>
>>>> I think we prefer something like u16 when copying to user space.
>>>
>>> but then userspace also has to expect a u16, right?
>>
>> Yep.
>>
> 
> Yes but in fact, inspired by previous discussion I had on the VFIO 
> interface, that is the reason why I did prefer an int.
> It is much simpler than a u16 and the definition of a bit.
> 
> Despite a bit in a u16 is what the s3990 achitecture proposes I thought 
> we could make it easier on the KVM/QEMU interface.
> 
> But if the discussion stops here, I will do as you both propose change 
> to u16 in KVM and userland and add the documentation for the interface.

In general, we pass via the ABI fixed-sized values -- u8, u16, u32, u64
... instead of int. Simply because sizeof(int) is in theory variable
(e.g., 32bit vs 64bit).

Take a look at arch/s390/include/uapi/asm/kvm.h and you won't find any
usage of int or bool.

Having that said, I'll let the maintainers decide. Using e.g., u8 is
just the natural thing to do on a Linux ABI, but we don't really support
32 bit ... maybe we'll support 128bit at one point? ;)
Pierre Morel May 18, 2022, 4:55 p.m. UTC | #8
On 5/18/22 16:33, David Hildenbrand wrote:
> On 16.05.22 16:21, Pierre Morel wrote:
>>
>>
>> On 5/12/22 12:01, David Hildenbrand wrote:
>>>>>
>>>>> I think we prefer something like u16 when copying to user space.
>>>>
>>>> but then userspace also has to expect a u16, right?
>>>
>>> Yep.
>>>
>>
>> Yes but in fact, inspired by previous discussion I had on the VFIO
>> interface, that is the reason why I did prefer an int.
>> It is much simpler than a u16 and the definition of a bit.
>>
>> Despite a bit in a u16 is what the s3990 achitecture proposes I thought
>> we could make it easier on the KVM/QEMU interface.
>>
>> But if the discussion stops here, I will do as you both propose change
>> to u16 in KVM and userland and add the documentation for the interface.
> 
> In general, we pass via the ABI fixed-sized values -- u8, u16, u32, u64
> ... instead of int. Simply because sizeof(int) is in theory variable
> (e.g., 32bit vs 64bit).
> 
> Take a look at arch/s390/include/uapi/asm/kvm.h and you won't find any
> usage of int or bool.
> 
> Having that said, I'll let the maintainers decide. Using e.g., u8 is
> just the natural thing to do on a Linux ABI, but we don't really support
> 32 bit ... maybe we'll support 128bit at one point? ;)
> 

OK then I use u16 with a flag in case we get something in the utilities 
which is related to the topology in the future.

Thanks,
Pierre
diff mbox series

Patch

diff --git a/arch/s390/include/uapi/asm/kvm.h b/arch/s390/include/uapi/asm/kvm.h
index 7a6b14874d65..abdcf4069343 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,10 @@  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
+
 /* 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 c8bdce31464f..80a1244f0ead 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1731,6 +1731,76 @@  static void kvm_s390_sca_set_mtcr(struct kvm *kvm)
 	ipte_unlock(kvm);
 }
 
+/**
+ * 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 void kvm_s390_sca_clear_mtcr(struct kvm *kvm)
+{
+	struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
+
+	ipte_lock(kvm);
+	sca->utility  &= ~SCA_UTILITY_MTCR;
+	ipte_unlock(kvm);
+}
+
+static int kvm_s390_set_topology(struct kvm *kvm, struct kvm_device_attr *attr)
+{
+	if (!test_kvm_facility(kvm, 11))
+		return -ENXIO;
+
+	switch (attr->attr) {
+	case KVM_S390_VM_CPU_TOPO_MTR_SET:
+		kvm_s390_sca_set_mtcr(kvm);
+		break;
+	case KVM_S390_VM_CPU_TOPO_MTR_CLEAR:
+		kvm_s390_sca_clear_mtcr(kvm);
+		break;
+	}
+	return 0;
+}
+
+/**
+ * kvm_s390_sca_get_mtcr
+ * @kvm: guest KVM description
+ *
+ * Is only relevant if the topology facility is present,
+ * the caller should check KVM facility 11
+ *
+ * reports to QEMU the Multiprocessor Topology-Change-Report.
+ */
+static int kvm_s390_sca_get_mtcr(struct kvm *kvm)
+{
+	struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
+	int val;
+
+	ipte_lock(kvm);
+	val = !!(sca->utility & SCA_UTILITY_MTCR);
+	ipte_unlock(kvm);
+
+	return val;
+}
+
+static int kvm_s390_get_topology(struct kvm *kvm, struct kvm_device_attr *attr)
+{
+	int mtcr;
+
+	if (!test_kvm_facility(kvm, 11))
+		return -ENXIO;
+
+	mtcr = kvm_s390_sca_get_mtcr(kvm);
+	if (copy_to_user((void __user *)attr->addr, &mtcr, sizeof(mtcr)))
+		return -EFAULT;
+
+	return 0;
+}
+
 static int kvm_s390_vm_set_attr(struct kvm *kvm, struct kvm_device_attr *attr)
 {
 	int ret;
@@ -1751,6 +1821,9 @@  static int kvm_s390_vm_set_attr(struct kvm *kvm, struct kvm_device_attr *attr)
 	case KVM_S390_VM_MIGRATION:
 		ret = kvm_s390_vm_set_migration(kvm, attr);
 		break;
+	case KVM_S390_VM_CPU_TOPOLOGY:
+		ret = kvm_s390_set_topology(kvm, attr);
+		break;
 	default:
 		ret = -ENXIO;
 		break;
@@ -1776,6 +1849,9 @@  static int kvm_s390_vm_get_attr(struct kvm *kvm, struct kvm_device_attr *attr)
 	case KVM_S390_VM_MIGRATION:
 		ret = kvm_s390_vm_get_migration(kvm, attr);
 		break;
+	case KVM_S390_VM_CPU_TOPOLOGY:
+		ret = kvm_s390_get_topology(kvm, attr);
+		break;
 	default:
 		ret = -ENXIO;
 		break;
@@ -1849,6 +1925,9 @@  static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
 	case KVM_S390_VM_MIGRATION:
 		ret = 0;
 		break;
+	case KVM_S390_VM_CPU_TOPOLOGY:
+		ret = test_kvm_facility(kvm, 11) ? 0 : -ENXIO;
+		break;
 	default:
 		ret = -ENXIO;
 		break;