Re: [PATCH 1/1] KVM: s390: fix LPSWEY handling

[Christian Borntraeger <borntraeger@linux.ibm.com>]

Am 27.06.24 um 11:44 schrieb Thomas Huth:
> On 27/06/2024 11.05, Christian Borntraeger wrote:
>> in rare cases, e.g. for injecting a machine check we do intercept all
>> load PSW instructions via ICTL_LPSW. With facility 193 a new variant
>> LPSWEY was added. KVM needs to handle that as well.
>>
>> Fixes: a3efa8429266 ("KVM: s390: gen_facilities: allow facilities 165, 193, 194 and 196")
>> Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
>> ---
>>   arch/s390/include/asm/kvm_host.h |  1 +
>>   arch/s390/kvm/kvm-s390.c         |  1 +
>>   arch/s390/kvm/kvm-s390.h         | 16 ++++++++++++++++
>>   arch/s390/kvm/priv.c             | 32 ++++++++++++++++++++++++++++++++
>>   4 files changed, 50 insertions(+)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index 95990461888f..9281063636a7 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -427,6 +427,7 @@ struct kvm_vcpu_stat {
>>       u64 instruction_io_other;
>>       u64 instruction_lpsw;
>>       u64 instruction_lpswe;
>> +    u64 instruction_lpswey;
>>       u64 instruction_pfmf;
>>       u64 instruction_ptff;
>>       u64 instruction_sck;
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 50b77b759042..8e04c7f0c90c 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -132,6 +132,7 @@ const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = {
>>       STATS_DESC_COUNTER(VCPU, instruction_io_other),
>>       STATS_DESC_COUNTER(VCPU, instruction_lpsw),
>>       STATS_DESC_COUNTER(VCPU, instruction_lpswe),
>> +    STATS_DESC_COUNTER(VCPU, instruction_lpswey),
>>       STATS_DESC_COUNTER(VCPU, instruction_pfmf),
>>       STATS_DESC_COUNTER(VCPU, instruction_ptff),
>>       STATS_DESC_COUNTER(VCPU, instruction_sck),
>> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
>> index 111eb5c74784..c61966cae121 100644
>> --- a/arch/s390/kvm/kvm-s390.h
>> +++ b/arch/s390/kvm/kvm-s390.h
>> @@ -138,6 +138,22 @@ static inline u64 kvm_s390_get_base_disp_s(struct kvm_vcpu *vcpu, u8 *ar)
>>       return (base2 ? vcpu->run->s.regs.gprs[base2] : 0) + disp2;
>>   }
>> +static inline u64 kvm_s390_get_base_disp_siy(struct kvm_vcpu *vcpu, u8 *ar)
>> +{
>> +    u32 base1 = vcpu->arch.sie_block->ipb >> 28;
>> +    u32 disp1 = ((vcpu->arch.sie_block->ipb & 0x0fff0000) >> 16) +
>> +            ((vcpu->arch.sie_block->ipb & 0xff00) << 4);
>> +
>> +    /* The displacement is a 20bit _SIGNED_ value */
>> +    if (disp1 & 0x80000)
>> +        disp1+=0xfff00000;
>> +
>> +    if (ar)
>> +        *ar = base1;
>> +
>> +    return (base1 ? vcpu->run->s.regs.gprs[base1] : 0) + (long)(int)disp1;
>> +}
>> +
>>   static inline void kvm_s390_get_base_disp_sse(struct kvm_vcpu *vcpu,
>>                             u64 *address1, u64 *address2,
>>                             u8 *ar_b1, u8 *ar_b2)
>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
>> index 1be19cc9d73c..1a49b89706f8 100644
>> --- a/arch/s390/kvm/priv.c
>> +++ b/arch/s390/kvm/priv.c
>> @@ -797,6 +797,36 @@ static int handle_lpswe(struct kvm_vcpu *vcpu)
>>       return 0;
>>   }
>> +static int handle_lpswey(struct kvm_vcpu *vcpu)
>> +{
>> +    psw_t new_psw;
>> +    u64 addr;
>> +    int rc;
>> +    u8 ar;
>> +
>> +    vcpu->stat.instruction_lpswey++;
>> +
>> +    if (!test_kvm_facility(vcpu->kvm, 193))
>> +        return kvm_s390_inject_program_int(vcpu, PGM_OPERATION);
>> +
>> +    if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
>> +        return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
>> +
>> +    addr = kvm_s390_get_base_disp_siy(vcpu, &ar);
>> +    if (addr & 7)
>> +        return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
>> +
>> +    rc = read_guest(vcpu, addr, ar, &new_psw, sizeof(new_psw));
>> +    if (rc)
>> +        return kvm_s390_inject_prog_cond(vcpu, rc);
> 
> Quoting the Principles of Operations:
> 
> "If the storage-key-removal facility is installed, a spe-
> cial-operation exception is recognized if the key value
> in bits 8-11 of the storage operand is nonzero."
> 
> Do we need to have such a check here, too?

We do not yet implement the storage key removal facility in KVM as far as
I can see. Only secure execution does that but there we do not have lpswe
intercepts. But yes, if we are going to implement this for normal guests
we need to have a look, also for LPSW(E)