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

[Halil Pasic <pasic@linux.ibm.com>]

On Tue, 24 May 2022 12:43:29 +0200
Thomas Huth <thuth@redhat.com> wrote:

> 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?

I don't fully understand the problem, and I don't have a strong opinion.
What I understand is KVM_CAP_S390_MEM_OP_EXTENSION tells me if some mem
op extensions may be available if non-zero or that none are available.
Which mem-op extensions are available is not yet actually defined.

I can think some more, but feel free to proceed without me.

Regards,
Halil