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

[Thomas Huth]

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 ... ;-)

(would it be better to treat it as a flag field, so that certain extensions 
could go away again in the future? In that case, it would be better to check 
with "& 1" instead of "> 0" here)

 Thomas