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

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

On 5/24/22 13:21, Thomas Huth wrote:
> On 24/05/2022 13.10, Christian Borntraeger wrote:
>>
>>
>> Am 24.05.22 um 12:43 schrieb Thomas Huth:
>>> On 19/05/2022 15.53, Janis Schoetterl-Glausch wrote:
>>>> 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.
>>>
>>> Oh well, never say that ... we've seen it in the past, that sometimes we want to get rid of features again, and if they don't have a separate feature flag bit somewhere, it's getting very ugly to disable them again.
>>>
>>> So since we don't have merged this patch yet, and thus we don't have a public userspace program using this interface yet, this is our last chance to redefine this interface before we might regret it later.
>>>
>>> I'm in strong favor of treating the KVM_CAP_S390_MEM_OP_EXTENSION as a flag field instead of a version number. What do others think? Christian? Halil?
>>
>> Its too late for that. This is part of 5.18.
> 
> Is it? We don't have to change the source code of the kernel,
> it's just about rewording what we have in api.rst documentation
> (which should be OK as long as there is no userspace program
> using this yet), e.g.:
> 
api.rst says about KVM_CHECK_EXTENSION:
:Returns: 0 if unsupported; 1 (or some other positive integer) if supported

but if we can return a negative value, we can define flags for possible future extensions
and flip the sign bit if we want to get rid of the storage key extension.

A bit ugly, but doesn't require any changes now.