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

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

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.