Re: [PATCH 2/2] target/s390x: kvm: Honor storage keys during emulation

[Janis Schoetterl-Glausch <scgl@linux.ibm.com>]

On 5/19/22 12:05, Thomas Huth wrote:
> On 06/05/2022 17.39, Janis Schoetterl-Glausch wrote:
>> Storage key controlled protection is currently not honored when
>> emulating instructions.
>> If available, enable key protection for the MEM_OP ioctl, thereby
>> enabling it for the s390_cpu_virt_mem_* functions, when using kvm.
>> As a result, the emulation of the following instructions honors storage
>> keys:
>>
>> * CLP
>>        The Synch I/O CLP command would need special handling in order
>>        to support storage keys, but is currently not supported.
>> * CHSC
>>     Performing commands asynchronously would require special
>>     handling, but commands are currently always synchronous.
>> * STSI
>> * TSCH
>>     Must (and does) not change channel if terminated due to
>>     protection.
>> * MSCH
>>     Suppressed on protection, works because fetching instruction.
>> * SSCH
>>     Suppressed on protection, works because fetching instruction.
>> * STSCH
>> * STCRW
>>     Suppressed on protection, this works because no partial store is
>>     possible, because the operand cannot span multiple pages.
>> * PCISTB
>> * MPCIFC
>> * STPCIFC
>>
>> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
>> ---
>>   target/s390x/kvm/kvm.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>> index 53098bf541..7bd8db0e7b 100644
>> --- a/target/s390x/kvm/kvm.c
>> +++ b/target/s390x/kvm/kvm.c
>> @@ -151,12 +151,15 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>>   static int cap_sync_regs;
>>   static int cap_async_pf;
>>   static int cap_mem_op;
>> +static int cap_mem_op_extension;
>>   static int cap_s390_irq;
>>   static int cap_ri;
>>   static int cap_hpage_1m;
>>   static int cap_vcpu_resets;
>>   static int cap_protected;
>>   +static bool mem_op_storage_key_support;
>> +
>>   static int active_cmma;
>>     static int kvm_s390_query_mem_limit(uint64_t *memory_limit)
>> @@ -354,6 +357,8 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>       cap_sync_regs = kvm_check_extension(s, KVM_CAP_SYNC_REGS);
>>       cap_async_pf = kvm_check_extension(s, KVM_CAP_ASYNC_PF);
>>       cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
>> +    cap_mem_op_extension = kvm_check_extension(s, KVM_CAP_S390_MEM_OP_EXTENSION);
>> +    mem_op_storage_key_support = cap_mem_op_extension > 0;
> 
> Ah, so KVM_CAP_S390_MEM_OP_EXTENSION is a "version number", not a boolean flag? ... ok, now I've finally understood that ... ;-)

Yeah, potentially having a bunch of memop capabilities didn't seem nice to me.
We can remove extensions if, when introducing an extension, we define that version x supports functionality y, z...,
but for the storage keys I've written in api.rst that it's supported if the cap > 0.
So we'd need a new cap if we want to get rid of the skey extension and still support some other extension, 
but that doesn't seem particularly likely.
> 
> (would it be better to treat it as a flag field, so that certain extensions could go away again in the future? In that case, it would be better to check with "& 1" instead of "> 0" here)
> 
>  Thomas
>