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(-)
> 

[...]

> +enum vsie_sca_flags {
> +	VSIE_SCA_ESCA = 0,
> +	VSIE_SCA_PINNED = 1,
>   };
>   
>   struct vsie_page {
> @@ -62,7 +68,9 @@ struct vsie_page {
>   	 * looked up by other CPUs.
>   	 */
>   	unsigned long flags;			/* 0x0260 */
> -	__u8 reserved[0x0700 - 0x0268];		/* 0x0268 */
> +	/* vsie system control area */
> +	struct vsie_sca *sca;			/* 0x0268 */
> +	__u8 reserved[0x0700 - 0x0270];		/* 0x0270 */
>   	struct kvm_s390_crypto_cb crycb;	/* 0x0700 */
>   	__u8 fac[S390_ARCH_FAC_LIST_SIZE_BYTE];	/* 0x0800 */
>   };
> @@ -72,6 +80,41 @@ struct kvm_address_pair {
>   	hpa_t hpa;
>   };
>   
> +/*
> + * Store the vsie system configuration data.
> + */
> +struct vsie_sca {
> +	/* calculated guest addresses of the sca */

Can you move the comments to the right?
Well, actually I'm not sure why we need them at all.
Aren't the variable names enough?

> +	gpa_t			sca_gpa;
> +	atomic_t		ref_count;
> +	/* defined in enum vsie_sca_flags */

Move the enum above the struct please.

> +	unsigned long		flags;
> +	unsigned long		sca_o_nr_pages;
> +	struct kvm_address_pair	sca_o_pages[KVM_S390_MAX_SCA_PAGES];
> +	u64			mcn[4];
> +	struct ssca_block	*ssca;
> +	int			page_count;
> +	int			page_next;
> +	struct vsie_page	*pages[KVM_S390_MAX_VSIE_VCPUS];
> +};
> +
> +static inline bool use_vsie_sigpif(struct kvm *kvm)
> +{
> +	return kvm->arch.use_vsie_sigpif;
> +}
> +
> +static inline bool use_vsie_sigpif_for(struct kvm *kvm, struct vsie_page *vsie_page)

The "for" in the name is weird.

Also, why are we allocating fenced on use_vsie_sigpif() but then shadow 
on the EC bits and sigpif? If neither extcall nor SRS are interpreted, 
why shadow via vsigpif at all?

> +{
> +	return use_vsie_sigpif(kvm) &&
> +	       (vsie_page->scb_o->eca & ECA_SIGPI) &&
> +	       (vsie_page->scb_o->ecb & ECB_SRSI);

Is SIGPI a prereq for SRSI and vice versa for vsigpif?
If no, then this should not be anded.

> +}
> +
> +static inline bool sie_uses_esca(struct kvm_s390_sie_block *scb)
> +{
> +	return (scb->ecb2 & ECB2_ESCA);
> +}
> +
>   /**
>    * gmap_shadow_valid() - check if a shadow guest address space matches the
>    *                       given properties and is still valid
> @@ -630,6 +673,8 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>   		scb_s->ictl |= ICTL_ISKE | ICTL_SSKE | ICTL_RRBE;
>   
>   	scb_s->icpua = scb_o->icpua;
> +	write_scao(scb_s, virt_to_phys(vsie_page->sca->ssca));
> +	scb_s->osda = virt_to_phys(scb_o);
>   
>   	if (!(atomic_read(&scb_s->cpuflags) & CPUSTAT_SM))
>   		new_mso = READ_ONCE(scb_o->mso) & 0xfffffffffff00000UL;
> @@ -681,6 +726,8 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>   	/* Instruction Execution Prevention */
>   	if (test_kvm_facility(vcpu->kvm, 130))
>   		scb_s->ecb2 |= scb_o->ecb2 & ECB2_IEP;
> +	/* extended SCA */
> +	scb_s->ecb2 |= scb_o->ecb2 & ECB2_ESCA;
>   	/* Guarded Storage */
>   	if (test_kvm_facility(vcpu->kvm, 133)) {
>   		scb_s->ecb |= scb_o->ecb & ECB_GS;
> @@ -713,12 +760,250 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>   	return rc;
>   }
>   
> +/* Called with ssca_lock held. */
> +static void unpin_sca(struct kvm *kvm, struct vsie_sca *sca)
> +{
> +	if (!test_bit(VSIE_SCA_PINNED, &sca->flags))
> +		return;
> +
> +	unpin_guest_pages(kvm, sca->sca_o_pages, sca->sca_o_nr_pages);
> +	sca->sca_o_nr_pages = 0;
> +
> +	__clear_bit(VSIE_SCA_PINNED, &sca->flags);
> +}
> +
> +/* pin g2s original sca in g1 memory */
> +static int pin_sca(struct kvm *kvm, struct vsie_page *vsie_page, struct vsie_sca *sca)
> +{
> +	bool is_esca = sie_uses_esca(vsie_page->scb_o);
> +	int nr_pages = KVM_S390_MAX_SCA_PAGES;
> +
> +	if (test_bit(VSIE_SCA_PINNED, &sca->flags))
> +		return 0;
> +
> +	if (!is_esca) {
> +		nr_pages = 1;
> +		if ((sca->sca_gpa & ~PAGE_MASK) + sizeof(struct bsca_block) > PAGE_SIZE)
> +			nr_pages = 2;
> +	}
> +
> +	sca->sca_o_nr_pages = pin_guest_pages(kvm, sca->sca_gpa, nr_pages, sca->sca_o_pages);
> +	if (WARN_ON_ONCE(sca->sca_o_nr_pages != nr_pages)) {
> +		set_validity_icpt(&vsie_page->scb_s, 0x0034U);
> +		return -EIO;

Any idea when this would happen?
gpa translate to last page and following pages would be over the guests 
allowed memory?

> +	}
> +	__set_bit(VSIE_SCA_PINNED, &sca->flags);
> +
> +	return 0;
> +}
> +
> +static void get_sca_entry_addr(struct kvm *kvm, struct vsie_page *vsie_page, struct vsie_sca *sca,
> +			       u16 cpu_nr, gpa_t *gpa, hpa_t *hpa)
> +{
> +	hpa_t offset;
> +	int pn;
> +
> +	/*
> +	 * We cannot simply access the hva since the esca_block has typically
> +	 * 4 pages (arch max 5 pages) that might not be continuous in g1 memory.
> +	 * The bsca_block may also be stretched over two pages. Only the header
> +	 * is guaranteed to be on the same page.
> +	 */
> +	if (test_bit(VSIE_SCA_ESCA, &sca->flags))
> +		offset = offsetof(struct esca_block, cpu[cpu_nr]);
> +	else
> +		offset = offsetof(struct bsca_block, cpu[cpu_nr]);
> +	pn = ((vsie_page->sca->sca_gpa & ~PAGE_MASK) + offset) >> PAGE_SHIFT;
> +	if (WARN_ON_ONCE(pn > sca->sca_o_nr_pages))
> +		return;
> +
> +	if (gpa)
> +		*gpa = sca->sca_o_pages[pn].gpa + offset;
> +	if (hpa)
> +		*hpa = sca->sca_o_pages[pn].hpa + offset;
> +}