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@xxxxxxxxxxxxx>
>> ---
>> 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);
>>
