[PATCH] KVM: s390: vsie: Check alignment of BSCA header

[PATCH] KVM: s390: vsie: Check alignment of BSCA header

[Eric Farman]

The VSIE code currently checks that the BSCA struct fits within
a page, and returns a validity exception 0x003b if it doesn't.
The BSCA is pinned in memory rather than shadowed (see block
comment at end of kvm_s390_cpu_feat_init()), so enforcing the
CPU entries to be on the same pinned page makes sense.

Except those entries aren't going to be used below the guest,
and according to the definition of that validity exception only
the header of the BSCA (everything but the CPU entries) needs to
be within a page. Adjust the alignment check to account for that.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 arch/s390/kvm/vsie.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 347268f89f2f..d23ab5120888 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -782,7 +782,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 		else if ((gpa & ~0x1fffUL) == kvm_s390_get_prefix(vcpu))
 			rc = set_validity_icpt(scb_s, 0x0011U);
 		else if ((gpa & PAGE_MASK) !=
-			 ((gpa + sizeof(struct bsca_block) - 1) & PAGE_MASK))
+			 ((gpa + offsetof(struct bsca_block, cpu[0]) - 1) & PAGE_MASK))
 			rc = set_validity_icpt(scb_s, 0x003bU);
 		if (!rc) {
 			rc = pin_guest_page(vcpu->kvm, gpa, &hpa);
-- 
2.48.1

Re: [PATCH] KVM: s390: vsie: Check alignment of BSCA header

[Christian Borntraeger]

Am 07.11.25 um 03:49 schrieb Eric Farman:
> The VSIE code currently checks that the BSCA struct fits within
> a page, and returns a validity exception 0x003b if it doesn't.
> The BSCA is pinned in memory rather than shadowed (see block
> comment at end of kvm_s390_cpu_feat_init()), so enforcing the
> CPU entries to be on the same pinned page makes sense.
> 
> Except those entries aren't going to be used below the guest,
> and according to the definition of that validity exception only
> the header of the BSCA (everything but the CPU entries) needs to
> be within a page. Adjust the alignment check to account for that.

Right, we do not yet support sigp interpretion for vsie and the
validity is indeed defined only for bsca header alignment:
---
The SCA header crosses a 4 K-byte boundary
(VIR code 003B hex).

Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>


--- 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   arch/s390/kvm/vsie.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 347268f89f2f..d23ab5120888 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -782,7 +782,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>   		else if ((gpa & ~0x1fffUL) == kvm_s390_get_prefix(vcpu))
>   			rc = set_validity_icpt(scb_s, 0x0011U);
>   		else if ((gpa & PAGE_MASK) !=
> -			 ((gpa + sizeof(struct bsca_block) - 1) & PAGE_MASK))
> +			 ((gpa + offsetof(struct bsca_block, cpu[0]) - 1) & PAGE_MASK))
>   			rc = set_validity_icpt(scb_s, 0x003bU);
>   		if (!rc) {
>   			rc = pin_guest_page(vcpu->kvm, gpa, &hpa);

Re: [PATCH] KVM: s390: vsie: Check alignment of BSCA header

[Christoph Schlameuss]

On Fri Nov 7, 2025 at 3:49 AM CET, Eric Farman wrote:
> The VSIE code currently checks that the BSCA struct fits within
> a page, and returns a validity exception 0x003b if it doesn't.
> The BSCA is pinned in memory rather than shadowed (see block
> comment at end of kvm_s390_cpu_feat_init()), so enforcing the
> CPU entries to be on the same pinned page makes sense.
>
> Except those entries aren't going to be used below the guest,
> and according to the definition of that validity exception only
> the header of the BSCA (everything but the CPU entries) needs to
> be within a page. Adjust the alignment check to account for that.
>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  arch/s390/kvm/vsie.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 347268f89f2f..d23ab5120888 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -782,7 +782,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  		else if ((gpa & ~0x1fffUL) == kvm_s390_get_prefix(vcpu))
>  			rc = set_validity_icpt(scb_s, 0x0011U);
>  		else if ((gpa & PAGE_MASK) !=
> -			 ((gpa + sizeof(struct bsca_block) - 1) & PAGE_MASK))
> +			 ((gpa + offsetof(struct bsca_block, cpu[0]) - 1) & PAGE_MASK))

Did you test if this works with an esca, where the header is bigger than this?
Previously the esca header was covered by the whole bsca struct.

>  			rc = set_validity_icpt(scb_s, 0x003bU);
>  		if (!rc) {
>  			rc = pin_guest_page(vcpu->kvm, gpa, &hpa);

Re: [PATCH] KVM: s390: vsie: Check alignment of BSCA header

[Eric Farman]

On Tue, 2025-11-11 at 09:51 +0100, Christoph Schlameuss wrote:
> On Fri Nov 7, 2025 at 3:49 AM CET, Eric Farman wrote:
> > The VSIE code currently checks that the BSCA struct fits within
> > a page, and returns a validity exception 0x003b if it doesn't.
> > The BSCA is pinned in memory rather than shadowed (see block
> > comment at end of kvm_s390_cpu_feat_init()), so enforcing the
> > CPU entries to be on the same pinned page makes sense.
> > 
> > Except those entries aren't going to be used below the guest,
> > and according to the definition of that validity exception only
> > the header of the BSCA (everything but the CPU entries) needs to
> > be within a page. Adjust the alignment check to account for that.
> > 
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >  arch/s390/kvm/vsie.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> > index 347268f89f2f..d23ab5120888 100644
> > --- a/arch/s390/kvm/vsie.c
> > +++ b/arch/s390/kvm/vsie.c
> > @@ -782,7 +782,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> >  		else if ((gpa & ~0x1fffUL) == kvm_s390_get_prefix(vcpu))
> >  			rc = set_validity_icpt(scb_s, 0x0011U);
> >  		else if ((gpa & PAGE_MASK) !=
> > -			 ((gpa + sizeof(struct bsca_block) - 1) & PAGE_MASK))
> > +			 ((gpa + offsetof(struct bsca_block, cpu[0]) - 1) & PAGE_MASK))
> 
> Did you test if this works with an esca, where the header is bigger than this?
> Previously the esca header was covered by the whole bsca struct.

I had originally coded up an offset like you did in your vsie sigpif series [*] for just this point,
but since we don't surface KVM_S390_VM_CPU_FEAT_SIGPIF to the guest (that comes later in your
series), I was having to force my way into driving that path and for minimal benefit. Now that I'm
remembering your RFC, having a conditional length is certainly correct but this is a good first
step.

[*] https://lore.kernel.org/linux-s390/20251110-vsieie-v2-3-9e53a3618c8c@linux.ibm.com/

> 
> >  			rc = set_validity_icpt(scb_s, 0x003bU);
> >  		if (!rc) {
> >  			rc = pin_guest_page(vcpu->kvm, gpa, &hpa);

Re: [PATCH] KVM: s390: vsie: Check alignment of BSCA header

[Christoph Schlameuss]

On Tue Nov 11, 2025 at 4:36 PM CET, Eric Farman wrote:
> On Tue, 2025-11-11 at 09:51 +0100, Christoph Schlameuss wrote:
>> On Fri Nov 7, 2025 at 3:49 AM CET, Eric Farman wrote:
>> > The VSIE code currently checks that the BSCA struct fits within
>> > a page, and returns a validity exception 0x003b if it doesn't.
>> > The BSCA is pinned in memory rather than shadowed (see block
>> > comment at end of kvm_s390_cpu_feat_init()), so enforcing the
>> > CPU entries to be on the same pinned page makes sense.
>> > 
>> > Except those entries aren't going to be used below the guest,
>> > and according to the definition of that validity exception only
>> > the header of the BSCA (everything but the CPU entries) needs to
>> > be within a page. Adjust the alignment check to account for that.
>> > 
>> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> > ---
>> >  arch/s390/kvm/vsie.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > 
>> > diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> > index 347268f89f2f..d23ab5120888 100644
>> > --- a/arch/s390/kvm/vsie.c
>> > +++ b/arch/s390/kvm/vsie.c
>> > @@ -782,7 +782,7 @@ static int pin_blocks(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>> >  		else if ((gpa & ~0x1fffUL) == kvm_s390_get_prefix(vcpu))
>> >  			rc = set_validity_icpt(scb_s, 0x0011U);
>> >  		else if ((gpa & PAGE_MASK) !=
>> > -			 ((gpa + sizeof(struct bsca_block) - 1) & PAGE_MASK))
>> > +			 ((gpa + offsetof(struct bsca_block, cpu[0]) - 1) & PAGE_MASK))
>> 
>> Did you test if this works with an esca, where the header is bigger than this?
>> Previously the esca header was covered by the whole bsca struct.
>
> I had originally coded up an offset like you did in your vsie sigpif series [*] for just this point,
> but since we don't surface KVM_S390_VM_CPU_FEAT_SIGPIF to the guest (that comes later in your
> series), I was having to force my way into driving that path and for minimal benefit. Now that I'm
> remembering your RFC, having a conditional length is certainly correct but this is a good first
> step.
>
> [*] https://lore.kernel.org/linux-s390/20251110-vsieie-v2-3-9e53a3618c8c@linux.ibm.com/
>

I agree that this is a good step in that direction. I am only concerned if we
may still get a validity intercept from fw when entering SIE while the ESCA
header is crossing the page boundary. The chances of that happening are slim as
at least Linux does always place the ESCA on the beginning of the page, but
other guests might not.
But then again getting the validity intercept from fw is not that much worse
than getting it from us directly.

So either way:

Reviewed-by: Christoph Schlameuss <schlameuss@linux.ibm.com>

>> 
>> >  			rc = set_validity_icpt(scb_s, 0x003bU);
>> >  		if (!rc) {
>> >  			rc = pin_guest_page(vcpu->kvm, gpa, &hpa);