* [PATCH 0/2] s390x: kvm: Honor storage keys during emulation @ 2022-05-06 15:39 Janis Schoetterl-Glausch 2022-05-06 15:39 ` [PATCH 1/2] Pull in MEMOP changes in linux-headers Janis Schoetterl-Glausch ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Janis Schoetterl-Glausch @ 2022-05-06 15:39 UTC (permalink / raw) To: qemu-s390x, Christian Borntraeger, Halil Pasic Cc: Janis Schoetterl-Glausch, Michael S. Tsirkin, Cornelia Huck, Paolo Bonzini, David Hildenbrand, Thomas Huth, qemu-devel Make use of the storage key support of the MEMOP ioctl, if available, in order to support storage key checking during emulation. I did not update all the headers, since that broke the build, not sure what the best way of dealing with that is. Janis Schoetterl-Glausch (2): Pull in MEMOP changes in linux-headers target/s390x: kvm: Honor storage keys during emulation linux-headers/linux/kvm.h | 11 +++++++++-- target/s390x/kvm/kvm.c | 9 +++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) base-commit: 31abf61c4929a91275fe32f1fafe6e6b3e840b2a -- 2.32.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] Pull in MEMOP changes in linux-headers 2022-05-06 15:39 [PATCH 0/2] s390x: kvm: Honor storage keys during emulation Janis Schoetterl-Glausch @ 2022-05-06 15:39 ` Janis Schoetterl-Glausch 2022-05-06 15:39 ` [PATCH 2/2] target/s390x: kvm: Honor storage keys during emulation Janis Schoetterl-Glausch 2022-05-09 8:06 ` [PATCH 0/2] s390x: " Cornelia Huck 2 siblings, 0 replies; 15+ messages in thread From: Janis Schoetterl-Glausch @ 2022-05-06 15:39 UTC (permalink / raw) To: qemu-s390x, Christian Borntraeger, Halil Pasic Cc: Janis Schoetterl-Glausch, Michael S. Tsirkin, Cornelia Huck, Paolo Bonzini, David Hildenbrand, Thomas Huth, qemu-devel, open list:Overall KVM CPUs Since a full update of the linux headers pulls in changes in vfio.h that break compilation, pull in only the required changes for storage key support. Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> --- linux-headers/linux/kvm.h | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h index d232feaae9..3948ffaad9 100644 --- a/linux-headers/linux/kvm.h +++ b/linux-headers/linux/kvm.h @@ -562,9 +562,12 @@ struct kvm_s390_mem_op { __u32 op; /* type of operation */ __u64 buf; /* buffer in userspace */ union { - __u8 ar; /* the access register number */ + struct { + __u8 ar; /* the access register number */ + __u8 key; /* access key, ignored if flag unset */ + }; __u32 sida_offset; /* offset into the sida */ - __u8 reserved[32]; /* should be set to 0 */ + __u8 reserved[32]; /* ignored */ }; }; /* types for kvm_s390_mem_op->op */ @@ -572,9 +575,12 @@ struct kvm_s390_mem_op { #define KVM_S390_MEMOP_LOGICAL_WRITE 1 #define KVM_S390_MEMOP_SIDA_READ 2 #define KVM_S390_MEMOP_SIDA_WRITE 3 +#define KVM_S390_MEMOP_ABSOLUTE_READ 4 +#define KVM_S390_MEMOP_ABSOLUTE_WRITE 5 /* flags for kvm_s390_mem_op->flags */ #define KVM_S390_MEMOP_F_CHECK_ONLY (1ULL << 0) #define KVM_S390_MEMOP_F_INJECT_EXCEPTION (1ULL << 1) +#define KVM_S390_MEMOP_F_SKEY_PROTECTION (1ULL << 2) /* for KVM_INTERRUPT */ struct kvm_interrupt { @@ -1134,6 +1140,7 @@ struct kvm_ppc_resize_hpt { #define KVM_CAP_VM_GPA_BITS 207 #define KVM_CAP_XSAVE2 208 #define KVM_CAP_SYS_ATTRIBUTES 209 +#define KVM_CAP_S390_MEM_OP_EXTENSION 211 #ifdef KVM_CAP_IRQ_ROUTING -- 2.32.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2] target/s390x: kvm: Honor storage keys during emulation 2022-05-06 15:39 [PATCH 0/2] s390x: kvm: Honor storage keys during emulation Janis Schoetterl-Glausch 2022-05-06 15:39 ` [PATCH 1/2] Pull in MEMOP changes in linux-headers Janis Schoetterl-Glausch @ 2022-05-06 15:39 ` Janis Schoetterl-Glausch 2022-05-19 10:05 ` Thomas Huth 2022-05-09 8:06 ` [PATCH 0/2] s390x: " Cornelia Huck 2 siblings, 1 reply; 15+ messages in thread From: Janis Schoetterl-Glausch @ 2022-05-06 15:39 UTC (permalink / raw) To: qemu-s390x, Christian Borntraeger, Halil Pasic Cc: Janis Schoetterl-Glausch, Michael S. Tsirkin, Cornelia Huck, Paolo Bonzini, David Hildenbrand, Thomas Huth, qemu-devel, Richard Henderson 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; cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ); cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS); cap_protected = kvm_check_extension(s, KVM_CAP_S390_PROTECTED); @@ -842,6 +847,7 @@ int kvm_s390_mem_op(S390CPU *cpu, vaddr addr, uint8_t ar, void *hostbuf, : KVM_S390_MEMOP_LOGICAL_READ, .buf = (uint64_t)hostbuf, .ar = ar, + .key = (cpu->env.psw.mask & PSW_MASK_KEY) >> PSW_SHIFT_KEY, }; int ret; @@ -851,6 +857,9 @@ int kvm_s390_mem_op(S390CPU *cpu, vaddr addr, uint8_t ar, void *hostbuf, if (!hostbuf) { mem_op.flags |= KVM_S390_MEMOP_F_CHECK_ONLY; } + if (mem_op_storage_key_support) { + mem_op.flags |= KVM_S390_MEMOP_F_SKEY_PROTECTION; + } ret = kvm_vcpu_ioctl(CPU(cpu), KVM_S390_MEM_OP, &mem_op); if (ret < 0) { -- 2.32.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] target/s390x: kvm: Honor storage keys during emulation 2022-05-06 15:39 ` [PATCH 2/2] target/s390x: kvm: Honor storage keys during emulation Janis Schoetterl-Glausch @ 2022-05-19 10:05 ` Thomas Huth 2022-05-19 13:53 ` Janis Schoetterl-Glausch 0 siblings, 1 reply; 15+ messages in thread From: Thomas Huth @ 2022-05-19 10:05 UTC (permalink / raw) To: Janis Schoetterl-Glausch, qemu-s390x, Christian Borntraeger, Halil Pasic Cc: Michael S. Tsirkin, Cornelia Huck, Paolo Bonzini, David Hildenbrand, qemu-devel, Richard Henderson 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] target/s390x: kvm: Honor storage keys during emulation 2022-05-19 10:05 ` Thomas Huth @ 2022-05-19 13:53 ` Janis Schoetterl-Glausch 2022-05-24 10:43 ` Thomas Huth 0 siblings, 1 reply; 15+ messages in thread From: Janis Schoetterl-Glausch @ 2022-05-19 13:53 UTC (permalink / raw) To: Thomas Huth, qemu-s390x, Christian Borntraeger, Halil Pasic Cc: Michael S. Tsirkin, Cornelia Huck, Paolo Bonzini, David Hildenbrand, qemu-devel, Richard Henderson 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. > > (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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] target/s390x: kvm: Honor storage keys during emulation 2022-05-19 13:53 ` Janis Schoetterl-Glausch @ 2022-05-24 10:43 ` Thomas Huth 2022-05-24 11:10 ` Christian Borntraeger 2022-05-24 16:08 ` Halil Pasic 0 siblings, 2 replies; 15+ messages in thread From: Thomas Huth @ 2022-05-24 10:43 UTC (permalink / raw) To: Janis Schoetterl-Glausch, qemu-s390x, Christian Borntraeger, Halil Pasic Cc: Michael S. Tsirkin, Cornelia Huck, Paolo Bonzini, David Hildenbrand, qemu-devel, Richard Henderson 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? Thomas ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] target/s390x: kvm: Honor storage keys during emulation 2022-05-24 10:43 ` Thomas Huth @ 2022-05-24 11:10 ` Christian Borntraeger 2022-05-24 11:21 ` Thomas Huth 2022-05-24 16:08 ` Halil Pasic 1 sibling, 1 reply; 15+ messages in thread From: Christian Borntraeger @ 2022-05-24 11:10 UTC (permalink / raw) To: Thomas Huth, Janis Schoetterl-Glausch, qemu-s390x, Halil Pasic Cc: Michael S. Tsirkin, Cornelia Huck, Paolo Bonzini, David Hildenbrand, qemu-devel, Richard Henderson 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] target/s390x: kvm: Honor storage keys during emulation 2022-05-24 11:10 ` Christian Borntraeger @ 2022-05-24 11:21 ` Thomas Huth 2022-05-24 11:52 ` Janis Schoetterl-Glausch 0 siblings, 1 reply; 15+ messages in thread From: Thomas Huth @ 2022-05-24 11:21 UTC (permalink / raw) To: Christian Borntraeger, Janis Schoetterl-Glausch, qemu-s390x, Halil Pasic Cc: Michael S. Tsirkin, Cornelia Huck, Paolo Bonzini, David Hildenbrand, qemu-devel, Richard Henderson 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.: diff a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst --- a/Documentation/virt/kvm/api.rst +++ b/Documentation/virt/kvm/api.rst @@ -3759,7 +3759,7 @@ If the KVM_S390_MEMOP_F_SKEY_PROTECTION flag is set, storage key protection is also in effect and may cause exceptions if accesses are prohibited given the access key designated by "key"; the valid range is 0..15. KVM_S390_MEMOP_F_SKEY_PROTECTION is available if KVM_CAP_S390_MEM_OP_EXTENSION -is > 0. +has the lowest bit set. Absolute read/write: ^^^^^^^^^^^^^^^^^^^^ @@ -3770,7 +3770,7 @@ the checks required for storage key protection as one operation (as opposed to user space getting the storage keys, performing the checks, and accessing memory thereafter, which could lead to a delay between check and access). Absolute accesses are permitted for the VM ioctl if KVM_CAP_S390_MEM_OP_EXTENSION -is > 0. +has the lowest bit set. Currently absolute accesses are not permitted for VCPU ioctls. Absolute accesses are permitted for non-protected guests only. What do you think? Thomas ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] target/s390x: kvm: Honor storage keys during emulation 2022-05-24 11:21 ` Thomas Huth @ 2022-05-24 11:52 ` Janis Schoetterl-Glausch 2022-05-25 9:00 ` Thomas Huth 0 siblings, 1 reply; 15+ messages in thread From: Janis Schoetterl-Glausch @ 2022-05-24 11:52 UTC (permalink / raw) To: Thomas Huth, Christian Borntraeger, qemu-s390x, Halil Pasic Cc: Michael S. Tsirkin, Cornelia Huck, Paolo Bonzini, David Hildenbrand, qemu-devel, Richard Henderson 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] target/s390x: kvm: Honor storage keys during emulation 2022-05-24 11:52 ` Janis Schoetterl-Glausch @ 2022-05-25 9:00 ` Thomas Huth 0 siblings, 0 replies; 15+ messages in thread From: Thomas Huth @ 2022-05-25 9:00 UTC (permalink / raw) To: Janis Schoetterl-Glausch, Christian Borntraeger, qemu-s390x, Halil Pasic Cc: Michael S. Tsirkin, Cornelia Huck, Paolo Bonzini, David Hildenbrand, qemu-devel, Richard Henderson 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] target/s390x: kvm: Honor storage keys during emulation 2022-05-24 10:43 ` Thomas Huth 2022-05-24 11:10 ` Christian Borntraeger @ 2022-05-24 16:08 ` Halil Pasic 1 sibling, 0 replies; 15+ messages in thread From: Halil Pasic @ 2022-05-24 16:08 UTC (permalink / raw) To: Thomas Huth Cc: Janis Schoetterl-Glausch, qemu-s390x, Christian Borntraeger, Michael S. Tsirkin, Cornelia Huck, Paolo Bonzini, David Hildenbrand, qemu-devel, Richard Henderson, Halil Pasic 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] s390x: kvm: Honor storage keys during emulation 2022-05-06 15:39 [PATCH 0/2] s390x: kvm: Honor storage keys during emulation Janis Schoetterl-Glausch 2022-05-06 15:39 ` [PATCH 1/2] Pull in MEMOP changes in linux-headers Janis Schoetterl-Glausch 2022-05-06 15:39 ` [PATCH 2/2] target/s390x: kvm: Honor storage keys during emulation Janis Schoetterl-Glausch @ 2022-05-09 8:06 ` Cornelia Huck 2022-05-10 13:32 ` Janis Schoetterl-Glausch 2 siblings, 1 reply; 15+ messages in thread From: Cornelia Huck @ 2022-05-09 8:06 UTC (permalink / raw) To: Janis Schoetterl-Glausch, qemu-s390x, Christian Borntraeger, Halil Pasic Cc: Janis Schoetterl-Glausch, Michael S. Tsirkin, Paolo Bonzini, David Hildenbrand, Thomas Huth, qemu-devel On Fri, May 06 2022, Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote: > Make use of the storage key support of the MEMOP ioctl, if available, > in order to support storage key checking during emulation. > > I did not update all the headers, since that broke the build, > not sure what the best way of dealing with that is. Yeah, the vfio change is expected to break the build; the fix should be easy (simple rename), and the code affected is deprecated anyway (there hasn't been any upstream implementation that actually exposed the interfaces). I think we should do that in a single commit to preserve bisectability; I have not seen any patches posted yet to actually use the new vfio migration interface, so a simple compile fixup should be all that is needed. > > Janis Schoetterl-Glausch (2): > Pull in MEMOP changes in linux-headers > target/s390x: kvm: Honor storage keys during emulation > > linux-headers/linux/kvm.h | 11 +++++++++-- > target/s390x/kvm/kvm.c | 9 +++++++++ > 2 files changed, 18 insertions(+), 2 deletions(-) > > > base-commit: 31abf61c4929a91275fe32f1fafe6e6b3e840b2a > -- > 2.32.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] s390x: kvm: Honor storage keys during emulation 2022-05-09 8:06 ` [PATCH 0/2] s390x: " Cornelia Huck @ 2022-05-10 13:32 ` Janis Schoetterl-Glausch 2022-05-10 13:43 ` Cornelia Huck 0 siblings, 1 reply; 15+ messages in thread From: Janis Schoetterl-Glausch @ 2022-05-10 13:32 UTC (permalink / raw) To: Cornelia Huck, qemu-s390x, Christian Borntraeger, Halil Pasic Cc: Michael S. Tsirkin, Paolo Bonzini, David Hildenbrand, Thomas Huth, qemu-devel, Matthew Rosato On 5/9/22 10:06, Cornelia Huck wrote: > On Fri, May 06 2022, Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote: > >> Make use of the storage key support of the MEMOP ioctl, if available, >> in order to support storage key checking during emulation. >> >> I did not update all the headers, since that broke the build, >> not sure what the best way of dealing with that is. > > Yeah, the vfio change is expected to break the build; the fix should be > easy (simple rename), and the code affected is deprecated anyway (there > hasn't been any upstream implementation that actually exposed the > interfaces). I think we should do that in a single commit to preserve > bisectability; I have not seen any patches posted yet to actually use > the new vfio migration interface, so a simple compile fixup should be > all that is needed. So basically this patch (pasted below) https://lore.kernel.org/qemu-devel/20220404181726.60291-3-mjrosato@linux.ibm.com/ squashed with the updated headers. Subject: [PATCH v5 2/9] vfio: tolerate migration protocol v1 uapi renames Date: Mon, 4 Apr 2022 14:17:19 -0400 [thread overview] Message-ID: <20220404181726.60291-3-mjrosato@linux.ibm.com> (raw) In-Reply-To: <20220404181726.60291-1-mjrosato@linux.ibm.com> The v1 uapi is deprecated and will be replaced by v2 at some point; this patch just tolerates the renaming of uapi fields to reflect v1 / deprecated status. Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> --- hw/vfio/common.c | 2 +- hw/vfio/migration.c | 19 +++++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 080046e3f5..7b1e12fb69 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -380,7 +380,7 @@ static bool vfio_devices_all_running_and_saving(VFIOContainer *container) return false; } - if ((migration->device_state & VFIO_DEVICE_STATE_SAVING) && + if ((migration->device_state & VFIO_DEVICE_STATE_V1_SAVING) && (migration->device_state & VFIO_DEVICE_STATE_RUNNING)) { continue; } else { diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index ff6b45de6b..e109cee551 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -432,7 +432,7 @@ static int vfio_save_setup(QEMUFile *f, void *opaque) } ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_MASK, - VFIO_DEVICE_STATE_SAVING); + VFIO_DEVICE_STATE_V1_SAVING); if (ret) { error_report("%s: Failed to set state SAVING", vbasedev->name); return ret; @@ -532,7 +532,7 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque) int ret; ret = vfio_migration_set_state(vbasedev, ~VFIO_DEVICE_STATE_RUNNING, - VFIO_DEVICE_STATE_SAVING); + VFIO_DEVICE_STATE_V1_SAVING); if (ret) { error_report("%s: Failed to set state STOP and SAVING", vbasedev->name); @@ -569,7 +569,7 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque) return ret; } - ret = vfio_migration_set_state(vbasedev, ~VFIO_DEVICE_STATE_SAVING, 0); + ret = vfio_migration_set_state(vbasedev, ~VFIO_DEVICE_STATE_V1_SAVING, 0); if (ret) { error_report("%s: Failed to set state STOPPED", vbasedev->name); return ret; @@ -730,7 +730,7 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state) * start saving data. */ if (state == RUN_STATE_SAVE_VM) { - value = VFIO_DEVICE_STATE_SAVING; + value = VFIO_DEVICE_STATE_V1_SAVING; } else { value = 0; } @@ -768,8 +768,9 @@ static void vfio_migration_state_notifier(Notifier *notifier, void *data) case MIGRATION_STATUS_FAILED: bytes_transferred = 0; ret = vfio_migration_set_state(vbasedev, - ~(VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RESUMING), - VFIO_DEVICE_STATE_RUNNING); + ~(VFIO_DEVICE_STATE_V1_SAVING | + VFIO_DEVICE_STATE_RESUMING), + VFIO_DEVICE_STATE_RUNNING); if (ret) { error_report("%s: Failed to set state RUNNING", vbasedev->name); } @@ -864,8 +865,10 @@ int vfio_migration_probe(VFIODevice *vbasedev, Error **errp) goto add_blocker; } - ret = vfio_get_dev_region_info(vbasedev, VFIO_REGION_TYPE_MIGRATION, - VFIO_REGION_SUBTYPE_MIGRATION, &info); + ret = vfio_get_dev_region_info(vbasedev, + VFIO_REGION_TYPE_MIGRATION_DEPRECATED, + VFIO_REGION_SUBTYPE_MIGRATION_DEPRECATED, + &info); if (ret) { goto add_blocker; } > >> >> Janis Schoetterl-Glausch (2): >> Pull in MEMOP changes in linux-headers >> target/s390x: kvm: Honor storage keys during emulation >> >> linux-headers/linux/kvm.h | 11 +++++++++-- >> target/s390x/kvm/kvm.c | 9 +++++++++ >> 2 files changed, 18 insertions(+), 2 deletions(-) >> >> >> base-commit: 31abf61c4929a91275fe32f1fafe6e6b3e840b2a >> -- >> 2.32.0 > ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] s390x: kvm: Honor storage keys during emulation 2022-05-10 13:32 ` Janis Schoetterl-Glausch @ 2022-05-10 13:43 ` Cornelia Huck 2022-05-12 8:52 ` Thomas Huth 0 siblings, 1 reply; 15+ messages in thread From: Cornelia Huck @ 2022-05-10 13:43 UTC (permalink / raw) To: Janis Schoetterl-Glausch, qemu-s390x, Christian Borntraeger, Halil Pasic Cc: Michael S. Tsirkin, Paolo Bonzini, David Hildenbrand, Thomas Huth, qemu-devel, Matthew Rosato, Alex Williamson On Tue, May 10 2022, Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote: > On 5/9/22 10:06, Cornelia Huck wrote: >> On Fri, May 06 2022, Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote: >> >>> Make use of the storage key support of the MEMOP ioctl, if available, >>> in order to support storage key checking during emulation. >>> >>> I did not update all the headers, since that broke the build, >>> not sure what the best way of dealing with that is. >> >> Yeah, the vfio change is expected to break the build; the fix should be >> easy (simple rename), and the code affected is deprecated anyway (there >> hasn't been any upstream implementation that actually exposed the >> interfaces). I think we should do that in a single commit to preserve >> bisectability; I have not seen any patches posted yet to actually use >> the new vfio migration interface, so a simple compile fixup should be >> all that is needed. > > So basically this patch (pasted below) > https://lore.kernel.org/qemu-devel/20220404181726.60291-3-mjrosato@linux.ibm.com/ > squashed with the updated headers. Yes. We should probably queue that seperately, just to disarm that trap for everyone; unless there's already a vfio update in flight? (Sorry, I've lost track a bit.) > > Subject: [PATCH v5 2/9] vfio: tolerate migration protocol v1 uapi renames > Date: Mon, 4 Apr 2022 14:17:19 -0400 [thread overview] > Message-ID: <20220404181726.60291-3-mjrosato@linux.ibm.com> (raw) > In-Reply-To: <20220404181726.60291-1-mjrosato@linux.ibm.com> > > The v1 uapi is deprecated and will be replaced by v2 at some point; > this patch just tolerates the renaming of uapi fields to reflect > v1 / deprecated status. > > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > --- > hw/vfio/common.c | 2 +- > hw/vfio/migration.c | 19 +++++++++++-------- > 2 files changed, 12 insertions(+), 9 deletions(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 080046e3f5..7b1e12fb69 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -380,7 +380,7 @@ static bool vfio_devices_all_running_and_saving(VFIOContainer *container) > return false; > } > > - if ((migration->device_state & VFIO_DEVICE_STATE_SAVING) && > + if ((migration->device_state & VFIO_DEVICE_STATE_V1_SAVING) && > (migration->device_state & VFIO_DEVICE_STATE_RUNNING)) { > continue; > } else { > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c > index ff6b45de6b..e109cee551 100644 > --- a/hw/vfio/migration.c > +++ b/hw/vfio/migration.c > @@ -432,7 +432,7 @@ static int vfio_save_setup(QEMUFile *f, void *opaque) > } > > ret = vfio_migration_set_state(vbasedev, VFIO_DEVICE_STATE_MASK, > - VFIO_DEVICE_STATE_SAVING); > + VFIO_DEVICE_STATE_V1_SAVING); > if (ret) { > error_report("%s: Failed to set state SAVING", vbasedev->name); > return ret; > @@ -532,7 +532,7 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque) > int ret; > > ret = vfio_migration_set_state(vbasedev, ~VFIO_DEVICE_STATE_RUNNING, > - VFIO_DEVICE_STATE_SAVING); > + VFIO_DEVICE_STATE_V1_SAVING); > if (ret) { > error_report("%s: Failed to set state STOP and SAVING", > vbasedev->name); > @@ -569,7 +569,7 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque) > return ret; > } > > - ret = vfio_migration_set_state(vbasedev, ~VFIO_DEVICE_STATE_SAVING, 0); > + ret = vfio_migration_set_state(vbasedev, ~VFIO_DEVICE_STATE_V1_SAVING, 0); > if (ret) { > error_report("%s: Failed to set state STOPPED", vbasedev->name); > return ret; > @@ -730,7 +730,7 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state) > * start saving data. > */ > if (state == RUN_STATE_SAVE_VM) { > - value = VFIO_DEVICE_STATE_SAVING; > + value = VFIO_DEVICE_STATE_V1_SAVING; > } else { > value = 0; > } > @@ -768,8 +768,9 @@ static void vfio_migration_state_notifier(Notifier *notifier, void *data) > case MIGRATION_STATUS_FAILED: > bytes_transferred = 0; > ret = vfio_migration_set_state(vbasedev, > - ~(VFIO_DEVICE_STATE_SAVING | VFIO_DEVICE_STATE_RESUMING), > - VFIO_DEVICE_STATE_RUNNING); > + ~(VFIO_DEVICE_STATE_V1_SAVING | > + VFIO_DEVICE_STATE_RESUMING), > + VFIO_DEVICE_STATE_RUNNING); > if (ret) { > error_report("%s: Failed to set state RUNNING", vbasedev->name); > } > @@ -864,8 +865,10 @@ int vfio_migration_probe(VFIODevice *vbasedev, Error **errp) > goto add_blocker; > } > > - ret = vfio_get_dev_region_info(vbasedev, VFIO_REGION_TYPE_MIGRATION, > - VFIO_REGION_SUBTYPE_MIGRATION, &info); > + ret = vfio_get_dev_region_info(vbasedev, > + VFIO_REGION_TYPE_MIGRATION_DEPRECATED, > + VFIO_REGION_SUBTYPE_MIGRATION_DEPRECATED, > + &info); > if (ret) { > goto add_blocker; > } >> >>> >>> Janis Schoetterl-Glausch (2): >>> Pull in MEMOP changes in linux-headers >>> target/s390x: kvm: Honor storage keys during emulation >>> >>> linux-headers/linux/kvm.h | 11 +++++++++-- >>> target/s390x/kvm/kvm.c | 9 +++++++++ >>> 2 files changed, 18 insertions(+), 2 deletions(-) >>> >>> >>> base-commit: 31abf61c4929a91275fe32f1fafe6e6b3e840b2a >>> -- >>> 2.32.0 >> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/2] s390x: kvm: Honor storage keys during emulation 2022-05-10 13:43 ` Cornelia Huck @ 2022-05-12 8:52 ` Thomas Huth 0 siblings, 0 replies; 15+ messages in thread From: Thomas Huth @ 2022-05-12 8:52 UTC (permalink / raw) To: Cornelia Huck, Janis Schoetterl-Glausch, qemu-s390x, Christian Borntraeger, Halil Pasic Cc: Michael S. Tsirkin, Paolo Bonzini, David Hildenbrand, qemu-devel, Matthew Rosato, Alex Williamson On 10/05/2022 15.43, Cornelia Huck wrote: > On Tue, May 10 2022, Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote: > >> On 5/9/22 10:06, Cornelia Huck wrote: >>> On Fri, May 06 2022, Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote: >>> >>>> Make use of the storage key support of the MEMOP ioctl, if available, >>>> in order to support storage key checking during emulation. >>>> >>>> I did not update all the headers, since that broke the build, >>>> not sure what the best way of dealing with that is. >>> >>> Yeah, the vfio change is expected to break the build; the fix should be >>> easy (simple rename), and the code affected is deprecated anyway (there >>> hasn't been any upstream implementation that actually exposed the >>> interfaces). I think we should do that in a single commit to preserve >>> bisectability; I have not seen any patches posted yet to actually use >>> the new vfio migration interface, so a simple compile fixup should be >>> all that is needed. >> >> So basically this patch (pasted below) >> https://lore.kernel.org/qemu-devel/20220404181726.60291-3-mjrosato@linux.ibm.com/ >> squashed with the updated headers. > > Yes. We should probably queue that seperately, just to disarm that trap > for everyone; unless there's already a vfio update in flight? (Sorry, I've > lost track a bit.) Unless somebody else has queued this already, I can try to come up with a separate pull request for the header update today or tomorrow. Thomas ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-05-25 9:06 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-06 15:39 [PATCH 0/2] s390x: kvm: Honor storage keys during emulation Janis Schoetterl-Glausch 2022-05-06 15:39 ` [PATCH 1/2] Pull in MEMOP changes in linux-headers Janis Schoetterl-Glausch 2022-05-06 15:39 ` [PATCH 2/2] target/s390x: kvm: Honor storage keys during emulation Janis Schoetterl-Glausch 2022-05-19 10:05 ` Thomas Huth 2022-05-19 13:53 ` Janis Schoetterl-Glausch 2022-05-24 10:43 ` Thomas Huth 2022-05-24 11:10 ` Christian Borntraeger 2022-05-24 11:21 ` Thomas Huth 2022-05-24 11:52 ` Janis Schoetterl-Glausch 2022-05-25 9:00 ` Thomas Huth 2022-05-24 16:08 ` Halil Pasic 2022-05-09 8:06 ` [PATCH 0/2] s390x: " Cornelia Huck 2022-05-10 13:32 ` Janis Schoetterl-Glausch 2022-05-10 13:43 ` Cornelia Huck 2022-05-12 8:52 ` Thomas Huth
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.