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

[Christian Borntraeger]

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.