Re: [PATCH v3 2/3] KVM: s390: Always allocate esca_block

["Christoph Schlameuss" <schlameuss@linux.ibm.com>]

On Mon May 26, 2025 at 12:36 PM CEST, Janosch Frank wrote:
> On 5/22/25 11:31 AM, Christoph Schlameuss wrote:
>> Instead of allocating a BSCA and upgrading it for PV or when adding the
>> 65th cpu we can always use the ESCA.
>> 
>> The only downside of the change is that we will always allocate 4 pages
>> for a 248 cpu ESCA instead of a single page for the BSCA per VM.
>> In return we can delete a bunch of checks and special handling depending
>> on the SCA type as well as the whole BSCA to ESCA conversion.
>> 
>> As a fallback we can still run without SCA entries when the SIGP
>> interpretation facility or ESCA are not available.
>> 
>> Signed-off-by: Christoph Schlameuss <schlameuss@linux.ibm.com>
>> ---
>>   arch/s390/include/asm/kvm_host.h |   1 -
>>   arch/s390/kvm/interrupt.c        |  71 +++++------------
>>   arch/s390/kvm/kvm-s390.c         | 161 ++++++---------------------------------
>>   arch/s390/kvm/kvm-s390.h         |   4 +-
>>   4 files changed, 45 insertions(+), 192 deletions(-)
>> 
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index f51bac835260f562eaf4bbfd373a24bfdbc43834..d03e354a63d9c931522c1a1607eba8685c24527f 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -631,7 +631,6 @@ struct kvm_s390_pv {
>>   
>
> [...]
>
>>   int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>>   {
>> -	gfp_t alloc_flags = GFP_KERNEL_ACCOUNT;
>> -	int i, rc;
>> +	gfp_t alloc_flags = GFP_KERNEL_ACCOUNT | __GFP_ZERO;
>>   	char debug_name[16];
>> -	static unsigned long sca_offset;
>> +	int i, rc;
>>   
>>   	rc = -EINVAL;
>>   #ifdef CONFIG_KVM_S390_UCONTROL
>> @@ -3358,17 +3341,12 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>>   	if (!sclp.has_64bscao)
>>   		alloc_flags |= GFP_DMA;
>>   	rwlock_init(&kvm->arch.sca_lock);
>> -	/* start with basic SCA */
>> -	kvm->arch.sca = (struct bsca_block *) get_zeroed_page(alloc_flags);
>> -	if (!kvm->arch.sca)
>> -		goto out_err;
>>   	mutex_lock(&kvm_lock);
>> -	sca_offset += 16;
>> -	if (sca_offset + sizeof(struct bsca_block) > PAGE_SIZE)
>> -		sca_offset = 0;
>> -	kvm->arch.sca = (struct bsca_block *)
>> -			((char *) kvm->arch.sca + sca_offset);
>> +
>> +	kvm->arch.sca = alloc_pages_exact(sizeof(*kvm->arch.sca), alloc_flags);
>
> kvm->arch.sca is (void *) at the point of this patch, which makes this a 
> very bad idea. Granted, you fix that up in the next patch but this is 
> still wrong.
>
> Any reason why you have patch #3 at all?
> We could just squash it and avoid this problem?
>

Yes, I can just roll that up into a single patch. Just thought it would be a bit
easier to review this way.

>>   	mutex_unlock(&kvm_lock);
>> +	if (!kvm->arch.sca)
>> +		goto out_err;
>>   
>>   	sprintf(debug_name, "kvm-%u", current->pid);
>>   
>
> [...]
>
>>   /* needs disabled preemption to protect from TOD sync and vcpu_load/put */
>> @@ -3919,7 +3808,7 @@ static int kvm_s390_vcpu_setup(struct kvm_vcpu *vcpu)
>>   		vcpu->arch.sie_block->eca |= ECA_IB;
>>   	if (sclp.has_siif)
>>   		vcpu->arch.sie_block->eca |= ECA_SII;
>> -	if (sclp.has_sigpif)
>> +	if (kvm_s390_use_sca_entries())
>>   		vcpu->arch.sie_block->eca |= ECA_SIGPI;
>>   	if (test_kvm_facility(vcpu->kvm, 129)) {
>>   		vcpu->arch.sie_block->eca |= ECA_VX;
>> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
>> index 8d3bbb2dd8d27802bbde2a7bd1378033ad614b8e..2c8e177e4af8f2dab07fd42a904cefdea80f6855 100644
>> --- a/arch/s390/kvm/kvm-s390.h
>> +++ b/arch/s390/kvm/kvm-s390.h
>> @@ -531,7 +531,7 @@ int kvm_s390_handle_per_event(struct kvm_vcpu *vcpu);
>>   /* support for Basic/Extended SCA handling */
>>   static inline union ipte_control *kvm_s390_get_ipte_control(struct kvm *kvm)
>>   {
>> -	struct bsca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
>> +	struct esca_block *sca = kvm->arch.sca; /* SCA version doesn't matter */
>
> Remove the comment as well please
>

That's also fully removed in patch 3 along with he whole method.

>>   
>>   	return &sca->ipte_control;
>>   }
>> @@ -542,7 +542,7 @@ static inline int kvm_s390_use_sca_entries(void)
>>   	 * might use the entries. By not setting the entries and keeping them
>>   	 * invalid, hardware will not access them but intercept.
>>   	 */
>> -	return sclp.has_sigpif;
>> +	return sclp.has_sigpif && sclp.has_esca;
>>   }
>>   void kvm_s390_reinject_machine_check(struct kvm_vcpu *vcpu,
>>   				     struct mcck_volatile_info *mcck_info);
>>