Re: [PATCH RFC v2 07/11] KVM: s390: Shadow VSIE SCA in guest-1

[Janosch Frank <frankja@linux.ibm.com>]

On 11/10/25 18:16, Christoph Schlameuss wrote:
> Restructure kvm_s390_handle_vsie() to create a guest-1 shadow of the SCA
> if guest-2 attempts to enter SIE with an SCA. If the SCA is used the
> vsie_pages are stored in a new vsie_sca struct instead of the arch vsie
> struct.
> 
> When the VSIE-Interpretation-Extension Facility is active (minimum z17)
> the shadow SCA (ssca_block) will be created and shadows of all CPUs
> defined in the configuration are created.
> SCAOL/H in the VSIE control block are overwritten with references to the
> shadow SCA.
> 
> The shadow SCA contains the addresses of the original guest-3 SCA as
> well as the original VSIE control blocks. With these addresses the
> machine can directly monitor the intervention bits within the original
> SCA entries, enabling it to handle SENSE_RUNNING and EXTERNAL_CALL sigp
> instructions without exiting VSIE.
> 
> The original SCA will be pinned in guest-2 memory and only be unpinned
> before reuse. This means some pages might still be pinned even after the
> guest 3 VM does no longer exist.
> 
> The ssca_blocks are also kept within a radix tree to reuse already
> existing ssca_blocks efficiently. While the radix tree and array with
> references to the ssca_blocks are held in the vsie_sca struct.
> The use of vsie_scas is tracked using an ref_count.
> 
> Signed-off-by: Christoph Schlameuss <schlameuss@linux.ibm.com>
> ---
>   arch/s390/include/asm/kvm_host.h       |  11 +-
>   arch/s390/include/asm/kvm_host_types.h |   5 +-
>   arch/s390/kvm/kvm-s390.c               |   6 +-
>   arch/s390/kvm/kvm-s390.h               |   2 +-
>   arch/s390/kvm/vsie.c                   | 672 ++++++++++++++++++++++++++++-----
>   5 files changed, 596 insertions(+), 100 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index 647014edd3de8abc15067e7203c4855c066c53ad..191b23edf0ac7e9a3e1fd9cdc6fc4c9a9e6769f8 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -597,13 +597,22 @@ struct sie_page2 {
>   };
>   
>   struct vsie_page;
> +struct vsie_sca;
>   
> +/*
> + * vsie_pages, scas and accompanied management vars
> + */
>   struct kvm_s390_vsie {
>   	struct mutex mutex;
>   	struct radix_tree_root addr_to_page;
>   	int page_count;
>   	int next;
> -	struct vsie_page *pages[KVM_MAX_VCPUS];
> +	struct vsie_page *pages[KVM_S390_MAX_VSIE_VCPUS];
> +	struct rw_semaphore ssca_lock;

Might make sense to name it sca_lock, since we're not locking sscas.

> +	struct radix_tree_root osca_to_sca;
> +	int sca_count;
> +	int sca_next;
> +	struct vsie_sca *scas[KVM_S390_MAX_VSIE_VCPUS];
>   };
>   

[...]

> +
> +/*
> + * Pin and get an existing or new guest system control area.
> + *
> + * May sleep.
> + */
> +static struct vsie_sca *get_vsie_sca(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page,
> +				     gpa_t sca_addr)
> +{
> +	struct vsie_sca *sca, *sca_new = NULL;
> +	struct kvm *kvm = vcpu->kvm;
> +	unsigned int max_sca;
> +	int rc;
> +
> +	rc = validate_scao(vcpu, vsie_page->scb_o, vsie_page->sca_gpa);
> +	if (rc)
> +		return ERR_PTR(rc);
> +
> +	/* get existing sca */
> +	down_read(&kvm->arch.vsie.ssca_lock);
> +	sca = get_existing_vsie_sca(kvm, sca_addr);
> +	up_read(&kvm->arch.vsie.ssca_lock);
> +	if (sca)
> +		return sca;
> +
> +	/*
> +	 * Allocate new ssca, it will likely be needed below.
> +	 * We want at least #online_vcpus shadows, so every VCPU can execute the
> +	 * VSIE in parallel. (Worst case all single core VMs.)
> +	 */

We're allocating an SCA and then its SSCA.

> +	max_sca = MIN(atomic_read(&kvm->online_vcpus), KVM_S390_MAX_VSIE_VCPUS);
> +	if (kvm->arch.vsie.sca_count < max_sca) {
> +		BUILD_BUG_ON(sizeof(struct vsie_sca) > PAGE_SIZE);
> +		sca_new = (void *)__get_free_page(GFP_KERNEL_ACCOUNT | __GFP_ZERO);
> +		if (!sca_new)
> +			return ERR_PTR(-ENOMEM);
> +
> +		if (use_vsie_sigpif(vcpu->kvm)) {
> +			BUILD_BUG_ON(offsetof(struct ssca_block, cpu) != 64);
> +			sca_new->ssca = alloc_pages_exact(sizeof(*sca_new->ssca),
> +							  GFP_KERNEL_ACCOUNT | __GFP_ZERO);
> +			if (!sca_new->ssca) {
> +				free_page((unsigned long)sca);

Shouldn't this be sca_new which we just allocated?
I think it might have been a mistake to have both sca and sca_new in 
this function even though I understand why you need it.

> +				sca_new = NULL;

Why?
We're returning in the next line.

> +				return ERR_PTR(-ENOMEM);
> +			}
> +		}
> +	}

How about something like:

Now we're taking the ssca lock in write mode so that we can manipulate 
the radix tree and recheck for existing scas with exclusive access.

In the next lines we try three things to get an SCA:
  - Retry getting an existing SCA
  - Using our newly allocated SCA if we're under the limit
  - Reusing an SCA with a different osca

> +
> +	/* enter write lock and recheck to make sure ssca has not been created by other cpu */
> +	down_write(&kvm->arch.vsie.ssca_lock);
> +	sca = get_existing_vsie_sca(kvm, sca_addr);
> +	if (sca)
> +		goto out;> +
> +	/* check again under write lock if we are still under our sca_count limit */
> +	if (sca_new && kvm->arch.vsie.sca_count < max_sca) {
> +		/* make use of vsie_sca just created */
> +		sca = sca_new;
> +		sca_new = NULL;
> +
> +		kvm->arch.vsie.scas[kvm->arch.vsie.sca_count] = sca;
> +	} else {
> +		/* reuse previously created vsie_sca allocation for different osca */
> +		sca = get_free_existing_vsie_sca(kvm);
> +		/* with nr_vcpus scas one must be free */
> +		if (IS_ERR(sca))
> +			goto out;
> +
> +		unpin_sca(kvm, sca);
> +		radix_tree_delete(&kvm->arch.vsie.osca_to_sca, sca->sca_gpa);
> +		memset(sca, 0, sizeof(struct vsie_sca));
> +	}