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

[Thomas Huth]

On 24/05/2022 13.52, Janis Schoetterl-Glausch wrote:
> 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.

Oh well, I hope we'll never end up in that situation ...
I guess it will likely be better to drop the MEM_OP_EXTENSION capability in 
that case and come up with something new instead.

Anyway, since I'm apparently the only one with my opinion, and since it's 
very unlikely that we want to get rid of these extensions in the future 
again, and we still have the big hammer of removing MEM_OP_EXTENSION 
completely, I won't insist on a rework here.

Queued to s390x-next now:

 https://gitlab.com/thuth/qemu/-/commits/s390x-next/

 Thomas