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>

[...]

> +/*
> + * Try to find an currently unused ssca_vsie from the vsie struct.
> + *
> + * Called with ssca_lock held.
> + */
> +static struct vsie_sca *get_free_existing_vsie_sca(struct kvm *kvm)
> +{
> +	struct vsie_sca *sca;
> +	int i, ref_count;
> +
> +	for (i = 0; i >= kvm->arch.vsie.sca_count; i++) {
> +		sca = kvm->arch.vsie.scas[kvm->arch.vsie.sca_next];
> +		kvm->arch.vsie.sca_next++;
> +		kvm->arch.vsie.sca_next %= kvm->arch.vsie.sca_count;
> +		ref_count = atomic_inc_return(&sca->ref_count);
> +		WARN_ON_ONCE(ref_count < 1);
> +		if (ref_count == 1)
> +			return sca;
> +		atomic_dec(&sca->ref_count);
> +	}
> +	return ERR_PTR(-EFAULT);

ENOENT?

> +}
> +
> +static void destroy_vsie_sca(struct kvm *kvm, struct vsie_sca *sca)
> +{
> +	radix_tree_delete(&kvm->arch.vsie.osca_to_sca, sca->sca_gpa);
> +	if (sca->ssca)
> +		free_pages_exact(sca->ssca, sca->page_count);
> +	sca->ssca = NULL;
> +	free_page((unsigned long)sca);
> +}
> +
> +static void put_vsie_sca(struct vsie_sca *sca)
> +{
> +	if (!sca)
> +		return;
> +
> +	WARN_ON_ONCE(atomic_dec_return(&sca->ref_count) < 0);
> +}
> +
> +/*
> + * 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);

This is wild.
validate_scao() returns 0/1 (once you fix the bool) and the rest of the 
function below returns -ERRNO. I think validate_scao() should return 
-EINVAL since the scao is clearly invalid if the function doesn't return 0.

> +
> +	/* 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.)
> +	 */
> +	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);

sca and sca_new are not FW structs, they are not scas that you can hand 
over to FW. As such they should not be exclusively named sca. Name them 
vsie_sca or some other name to make it clear that we're working with a 
KVM struct.

Is there a need for sca_new to be page allocated?
vsie_page's size is close to a page and it is similar to sie_page so 
that makes sense. But vsie_sca is only a copule of DWORDs until we reach 
the "pages" member and we could dynamically allocate vsie_sca based on 
the actual number of max pages since pages is at the end of the struct.

> +		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);
> +				sca_new = NULL;
> +				return ERR_PTR(-ENOMEM);
> +			}
> +		}
> +	}
> +
> +	/* 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));
> +	}
> +
> +	/* use ECB of shadow scb to determine SCA type */
> +	if (sie_uses_esca(vsie_page->scb_o))
> +		__set_bit(VSIE_SCA_ESCA, &sca->flags);
> +	sca->sca_gpa = sca_addr;
> +	sca->pages[vsie_page->scb_o->icpua] = vsie_page;
> +
> +	if (sca->sca_gpa != 0) {
> +		/*
> +		 * The pinned original sca will only be unpinned lazily to limit the
> +		 * required amount of pins/unpins on each vsie entry/exit.
> +		 * The unpin is done in the reuse vsie_sca allocation path above and
> +		 * kvm_s390_vsie_destroy().
> +		 */
> +		rc = pin_sca(kvm, vsie_page, sca);
> +		if (rc) {
> +			sca = ERR_PTR(rc);
> +			goto out;
> +		}
> +	}
> +
> +	atomic_set(&sca->ref_count, 1);
> +	radix_tree_insert(&kvm->arch.vsie.osca_to_sca, sca->sca_gpa, sca);
> +
> +out:
> +	up_write(&kvm->arch.vsie.ssca_lock);
> +	if (sca_new)
> +		destroy_vsie_sca(kvm, sca_new);
> +	return sca;
> +}