KVM: s390: Add support for protected VMs
Subsystems (5)
Re: [RFC 19/37] KVM: s390: protvirt: Add new gprs location handling
In reply to David HildenbrandQuoted 54 lines · David Hildenbrand
Quoted 40 lines · Janosch Frank
Guest registers for protected guests are stored at offset
0x380.Signed-off-by Janosch Frank <[email protected]>---
arch/s390/include/asm/kvm_host.h| 4 +++- arch/s390/kvm/kvm-s390.c | 11 +++++++++++2 files changed, 14 insertions(+), 1 deletion(-)arch/s390/include/asm/kvm_host.h +0
-0@@ -336,7 +336,9 @@ struct kvm_s390_itdb { struct sie_page { struct kvm_s390_sie_block sie_block;struct
mcck_volatile_infomcck_info; /*0x0200*/ -__u8reserved218[1000]; /*0x0218*/ +__u8reserved218[360]; /*0x0218*/
+__u64pv_grregs[16]; /*0x380*/
+__u8reserved400[512];
structkvm_s390_itdbitdb; /*0x0600*/__u8reserved700[2304]; /*0x0700*/ };arch/s390/kvm/kvm-s390.c +3
-0@@ -3965,6 +3965,7 @@ static int vcpu_post_run(struct kvm_vcpu *vcpu, int exit_reason) static int __vcpu_run(struct kvm_vcpu *vcpu) { int rc, exit_reason; + struct sie_page *sie_page = (struct sie_page *)vcpu->arch.sie_block; /** We try to hold kvm->srcu during most of vcpu_run (except when run- @@ -3986,8 +3987,18 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) guest_enter_irqoff(); __disable_cpu_timer_accounting(vcpu); local_irq_enable(); + if (kvm_s390_pv_is_protected(vcpu->kvm)) { + memcpy(sie_page->pv_grregs,arch/s390/kvm/kvm-s390.c static int __vcpu_run(struct kvm_vcpu *vcpu) · line 4001
Quoted 10 lines
arch/s390/kvm/kvm-s390.c +8
-0+ vcpu->run->s.regs.gprs, + sizeof(sie_page->pv_grregs)); + } exit_reason = sie64a(vcpu->arch.sie_block, vcpu->run->s.regs.gprs); + if (kvm_s390_pv_is_protected(vcpu->kvm)) { + memcpy(vcpu->run->s.regs.gprs, + sie_page->pv_grregs, + sizeof(sie_page->pv_grregs)); + }sie64a will load/save gprs 0-13 from to
vcpu->run->s.regs.gprs.
Quoted 1 line
I would have assume that this is not required for prot virt, because the HW has direct access via the sie block?
Yes, that is correct. The load/save in sie64a is not necessary for pv guests.
Quoted 3 lines
1. Would it make sense to have a specialized
sie64a()(or a parameter, e.g., if you pass in NULL in r3), that optimizes this loading/saving? Eventually we can also optimize which host registers to save/restore then.
Having 2 kinds of sie64a seems not very nice for just saving a small number of cycles.
Quoted 2 lines
2. Avoid this copying here. We have to store the state to
vcpu->run->s.regs.gprswhen returning to user space and restore the state when coming from user space.
I like this proposal better than the first one and
Quoted 5 lines
Also, we access the GPRS from interception handlers, there we might use wrappers like
kvm_s390_set_gprs()kvm_s390_get_gprs()
having register accessors might be useful anyway.
But I would like to defer that to a later point in time to keep the changes in here minimal?
We can add a "TODO" comment in here so that we do not forget about this for a future patch. Makes sense?
Quoted 2 lines
to route to the right location. There are multiple options to optimize this.
Janosch Frank
maintainerRe: [RFC 19/37] KVM: s390: protvirt: Add new gprs location handling
Quoted 77 lines · Christian Borntraeger
Quoted 54 lines · David Hildenbrand
Quoted 40 lines · Janosch Frank
Guest registers for protected guests are stored at offset
0x380.Signed-off-by Janosch Frank <[email protected]>---
arch/s390/include/asm/kvm_host.h| 4 +++- arch/s390/kvm/kvm-s390.c | 11 +++++++++++2 files changed, 14 insertions(+), 1 deletion(-)arch/s390/include/asm/kvm_host.h +0
-0@@ -336,7 +336,9 @@ struct kvm_s390_itdb { struct sie_page { struct kvm_s390_sie_block sie_block;struct
mcck_volatile_infomcck_info; /*0x0200*/ -__u8reserved218[1000]; /*0x0218*/ +__u8reserved218[360]; /*0x0218*/
+__u64pv_grregs[16]; /*0x380*/
+__u8reserved400[512];
structkvm_s390_itdbitdb; /*0x0600*/__u8reserved700[2304]; /*0x0700*/ };arch/s390/kvm/kvm-s390.c +3
-0@@ -3965,6 +3965,7 @@ static int vcpu_post_run(struct kvm_vcpu *vcpu, int exit_reason) static int __vcpu_run(struct kvm_vcpu *vcpu) { int rc, exit_reason; + struct sie_page *sie_page = (struct sie_page *)vcpu->arch.sie_block; /** We try to hold kvm->srcu during most of vcpu_run (except when run- @@ -3986,8 +3987,18 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) guest_enter_irqoff(); __disable_cpu_timer_accounting(vcpu); local_irq_enable(); + if (kvm_s390_pv_is_protected(vcpu->kvm)) { + memcpy(sie_page->pv_grregs,arch/s390/kvm/kvm-s390.c static int __vcpu_run(struct kvm_vcpu *vcpu) · line 4001
Quoted 10 lines
arch/s390/kvm/kvm-s390.c +8
-0+ vcpu->run->s.regs.gprs, + sizeof(sie_page->pv_grregs)); + } exit_reason = sie64a(vcpu->arch.sie_block, vcpu->run->s.regs.gprs); + if (kvm_s390_pv_is_protected(vcpu->kvm)) { + memcpy(vcpu->run->s.regs.gprs, + sie_page->pv_grregs, + sizeof(sie_page->pv_grregs)); + }sie64a will load/save gprs 0-13 from to
vcpu->run->s.regs.gprs.Quoted 1 line
I would have assume that this is not required for prot virt, because the HW has direct access via the sie block?
Yes, that is correct. The load/save in sie64a is not necessary for pv guests.
Quoted 3 lines
1. Would it make sense to have a specialized
sie64a()(or a parameter, e.g., if you pass in NULL in r3), that optimizes this loading/saving? Eventually we can also optimize which host registers to save/restore then.Having 2 kinds of sie64a seems not very nice for just saving a small number of cycles.
Quoted 2 lines
2. Avoid this copying here. We have to store the state to
vcpu->run->s.regs.gprswhen returning to user space and restore the state when coming from user space.I like this proposal better than the first one and
Quoted 5 lines
Also, we access the GPRS from interception handlers, there we might use wrappers like
kvm_s390_set_gprs()kvm_s390_get_gprs()
Quoted 6 lines
having register accessors might be useful anyway.
But I would like to defer that to a later point in time to keep the changes in here minimal?We can add a "TODO" comment in here so that we do not forget about this for a future patch. Makes sense?
I second all of that :-)
Re: [RFC 19/37] KVM: s390: protvirt: Add new gprs location handling
Quoted 64 lines · Janosch Frank
Quoted 63 lines · Christian Borntraeger
Quoted 54 lines · David Hildenbrand
Quoted 40 lines · Janosch Frank
Guest registers for protected guests are stored at offset
0x380.Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/asm/kvm_host.h | 4 +++- arch/s390/kvm/kvm-s390.c | 11 +++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-)arch/s390/include/asm/kvm_host.h +3
-1@@ -336,7 +336,9 @@ struct kvm_s390_itdb { struct sie_page { struct kvm_s390_sie_block sie_block; struct mcck_volatile_info mcck_info; /* 0x0200 */ - __u8 reserved218[1000]; /* 0x0218 */ + __u8 reserved218[360]; /* 0x0218 */ + __u64 pv_grregs[16]; /* 0x380 */ + __u8 reserved400[512]; struct kvm_s390_itdb itdb; /* 0x0600 */ __u8 reserved700[2304]; /* 0x0700 */ };arch/s390/kvm/kvm-s390.c +3
-0@@ -3965,6 +3965,7 @@ static int vcpu_post_run(struct kvm_vcpu *vcpu, int exit_reason) static int __vcpu_run(struct kvm_vcpu *vcpu) { int rc, exit_reason; + struct sie_page *sie_page = (struct sie_page *)vcpu->arch.sie_block; /** We try to hold kvm->srcu during most of vcpu_run (except when run- @@ -3986,8 +3987,18 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) guest_enter_irqoff(); __disable_cpu_timer_accounting(vcpu); local_irq_enable(); + if (kvm_s390_pv_is_protected(vcpu->kvm)) { + memcpy(sie_page->pv_grregs,arch/s390/kvm/kvm-s390.c static int __vcpu_run(struct kvm_vcpu *vcpu) · line 4001
Quoted 10 lines
arch/s390/kvm/kvm-s390.c +8
-0+ vcpu->run->s.regs.gprs, + sizeof(sie_page->pv_grregs)); + } exit_reason = sie64a(vcpu->arch.sie_block, vcpu->run->s.regs.gprs); + if (kvm_s390_pv_is_protected(vcpu->kvm)) { + memcpy(vcpu->run->s.regs.gprs, + sie_page->pv_grregs, + sizeof(sie_page->pv_grregs)); + }sie64a will load/save gprs 0-13 from to
vcpu->run->s.regs.gprs.Quoted 1 line
I would have assume that this is not required for prot virt, because the HW has direct access via the sie block?
Yes, that is correct. The load/save in sie64a is not necessary for pv guests.
Quoted 2 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
1. Would it make sense to have a specialized
sie64a()(or a parameter, e.g., if you pass in NULL in r3), that optimizes this loading/saving? Eventually we can also optimize which host registers to save/restore then.Having 2 kinds of sie64a seems not very nice for just saving a small number of cycles.
Quoted 2 lines
2. Avoid this copying here. We have to store the state to
vcpu->run->s.regs.gprswhen returning to user space and restore the state when coming from user space.I like this proposal better than the first one and
It was actually an additional proposal :)
1. avoids unnecessary saving/loading/saving/restoring
2. avoids the two memcpy
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Also, we access the GPRS from interception handlers, there we might use wrappers like
kvm_s390_set_gprs()kvm_s390_get_gprs()
Quoted 6 lines
Quoted 6 lines
having register accessors might be useful anyway.
But I would like to defer that to a later point in time to keep the changes in here minimal?We can add a "TODO" comment in here so that we do not forget about this for a future patch. Makes sense?
While it makes sense, I guess one could come up with a patch for 2. in less than 30 minutes ... but yeah, whatever you prefer. ;)
Signature · 3 lines
Thanks, David / dhildenb
Janosch Frank
maintainerRe: [RFC 19/37] KVM: s390: protvirt: Add new gprs location handling
Quoted 89 lines · David Hildenbrand
Quoted 64 lines · Janosch Frank
Quoted 63 lines · Christian Borntraeger
Quoted 54 lines · David Hildenbrand
Quoted 40 lines · Janosch Frank
Guest registers for protected guests are stored at offset
0x380.Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/asm/kvm_host.h | 4 +++- arch/s390/kvm/kvm-s390.c | 11 +++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-)arch/s390/include/asm/kvm_host.h +3
-1@@ -336,7 +336,9 @@ struct kvm_s390_itdb { struct sie_page { struct kvm_s390_sie_block sie_block; struct mcck_volatile_info mcck_info; /* 0x0200 */ - __u8 reserved218[1000]; /* 0x0218 */ + __u8 reserved218[360]; /* 0x0218 */ + __u64 pv_grregs[16]; /* 0x380 */ + __u8 reserved400[512]; struct kvm_s390_itdb itdb; /* 0x0600 */ __u8 reserved700[2304]; /* 0x0700 */ };arch/s390/kvm/kvm-s390.c +3
-0@@ -3965,6 +3965,7 @@ static int vcpu_post_run(struct kvm_vcpu *vcpu, int exit_reason) static int __vcpu_run(struct kvm_vcpu *vcpu) { int rc, exit_reason; + struct sie_page *sie_page = (struct sie_page *)vcpu->arch.sie_block; /** We try to hold kvm->srcu during most of vcpu_run (except when run- @@ -3986,8 +3987,18 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) guest_enter_irqoff(); __disable_cpu_timer_accounting(vcpu); local_irq_enable(); + if (kvm_s390_pv_is_protected(vcpu->kvm)) { + memcpy(sie_page->pv_grregs,arch/s390/kvm/kvm-s390.c static int __vcpu_run(struct kvm_vcpu *vcpu) · line 4001
Quoted 10 lines
arch/s390/kvm/kvm-s390.c +8
-0+ vcpu->run->s.regs.gprs, + sizeof(sie_page->pv_grregs)); + } exit_reason = sie64a(vcpu->arch.sie_block, vcpu->run->s.regs.gprs); + if (kvm_s390_pv_is_protected(vcpu->kvm)) { + memcpy(vcpu->run->s.regs.gprs, + sie_page->pv_grregs, + sizeof(sie_page->pv_grregs)); + }sie64a will load/save gprs 0-13 from to
vcpu->run->s.regs.gprs.Quoted 1 line
I would have assume that this is not required for prot virt, because the HW has direct access via the sie block?
Yes, that is correct. The load/save in sie64a is not necessary for pv guests.
Quoted 2 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
1. Would it make sense to have a specialized
sie64a()(or a parameter, e.g., if you pass in NULL in r3), that optimizes this loading/saving? Eventually we can also optimize which host registers to save/restore then.Having 2 kinds of sie64a seems not very nice for just saving a small number of cycles.
Quoted 2 lines
2. Avoid this copying here. We have to store the state to
vcpu->run->s.regs.gprswhen returning to user space and restore the state when coming from user space.I like this proposal better than the first one and
It was actually an additional proposal :)
1. avoids unnecessary saving/loading/saving/restoring
2. avoids the two memcpyQuoted 10 lines
Quoted 10 lines
Quoted 5 lines
Also, we access the GPRS from interception handlers, there we might use wrappers like
kvm_s390_set_gprs()kvm_s390_get_gprs()having register accessors might be useful anyway.
But I would like to defer that to a later point in time to keep the changes in here minimal?
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
We can add a "TODO" comment in here so that we do not forget about this for a future patch. Makes sense?
While it makes sense, I guess one could come up with a patch for 2. in less than 30 minutes ... but yeah, whatever you prefer. ;)
Just to get it fully right we'd need to:
a. Synchronize registers into/from vcpu run in sync_regs/store_regs b. Sprinkle get/set_gpr(int nr) over most of the files in arch/s390/kvm
That's your proposal?
Re: [RFC 19/37] KVM: s390: protvirt: Add new gprs location handling
Quoted 97 lines · Janosch Frank
Quoted 89 lines · David Hildenbrand
Quoted 64 lines · Janosch Frank
Quoted 63 lines · Christian Borntraeger
Quoted 54 lines · David Hildenbrand
Quoted 40 lines · Janosch Frank
Guest registers for protected guests are stored at offset
0x380.Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/asm/kvm_host.h | 4 +++- arch/s390/kvm/kvm-s390.c | 11 +++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-)arch/s390/include/asm/kvm_host.h +3
-1@@ -336,7 +336,9 @@ struct kvm_s390_itdb { struct sie_page { struct kvm_s390_sie_block sie_block; struct mcck_volatile_info mcck_info; /* 0x0200 */ - __u8 reserved218[1000]; /* 0x0218 */ + __u8 reserved218[360]; /* 0x0218 */ + __u64 pv_grregs[16]; /* 0x380 */ + __u8 reserved400[512]; struct kvm_s390_itdb itdb; /* 0x0600 */ __u8 reserved700[2304]; /* 0x0700 */ };arch/s390/kvm/kvm-s390.c +3
-0@@ -3965,6 +3965,7 @@ static int vcpu_post_run(struct kvm_vcpu *vcpu, int exit_reason) static int __vcpu_run(struct kvm_vcpu *vcpu) { int rc, exit_reason; + struct sie_page *sie_page = (struct sie_page *)vcpu->arch.sie_block; /** We try to hold kvm->srcu during most of vcpu_run (except when run- @@ -3986,8 +3987,18 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) guest_enter_irqoff(); __disable_cpu_timer_accounting(vcpu); local_irq_enable(); + if (kvm_s390_pv_is_protected(vcpu->kvm)) { + memcpy(sie_page->pv_grregs,arch/s390/kvm/kvm-s390.c static int __vcpu_run(struct kvm_vcpu *vcpu) · line 4001
Quoted 10 lines
arch/s390/kvm/kvm-s390.c +8
-0+ vcpu->run->s.regs.gprs, + sizeof(sie_page->pv_grregs)); + } exit_reason = sie64a(vcpu->arch.sie_block, vcpu->run->s.regs.gprs); + if (kvm_s390_pv_is_protected(vcpu->kvm)) { + memcpy(vcpu->run->s.regs.gprs, + sie_page->pv_grregs, + sizeof(sie_page->pv_grregs)); + }sie64a will load/save gprs 0-13 from to
vcpu->run->s.regs.gprs.Quoted 1 line
I would have assume that this is not required for prot virt, because the HW has direct access via the sie block?
Yes, that is correct. The load/save in sie64a is not necessary for pv guests.
Quoted 2 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
1. Would it make sense to have a specialized
sie64a()(or a parameter, e.g., if you pass in NULL in r3), that optimizes this loading/saving? Eventually we can also optimize which host registers to save/restore then.Having 2 kinds of sie64a seems not very nice for just saving a small number of cycles.
Quoted 2 lines
2. Avoid this copying here. We have to store the state to
vcpu->run->s.regs.gprswhen returning to user space and restore the state when coming from user space.I like this proposal better than the first one and
It was actually an additional proposal :)
1. avoids unnecessary saving/loading/saving/restoring
2. avoids the two memcpyQuoted 10 lines
Quoted 10 lines
Quoted 5 lines
Also, we access the GPRS from interception handlers, there we might use wrappers like
kvm_s390_set_gprs()kvm_s390_get_gprs()having register accessors might be useful anyway.
But I would like to defer that to a later point in time to keep the changes in here minimal?Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
We can add a "TODO" comment in here so that we do not forget about this for a future patch. Makes sense?
While it makes sense, I guess one could come up with a patch for 2. in less than 30 minutes ... but yeah, whatever you prefer. ;)
Quoted 5 lines
Just to get it fully right we'd need to:
a. Synchronize registers into/from vcpu run in sync_regs/store_regs b. Sprinkle get/set_gpr(int nr) over most of the files inarch/s390/kvmThat's your proposal?
Yes. Patch 1, factor out gprs access. Patch 2, avoid the memcpy by fixing the gprs access functions and removing the memcpys. (both as addons to this patch)
I guess that should be it ... but maybe we'll stumble over surprises :)
Signature · 3 lines
Thanks, David / dhildenb
Re: [RFC 19/37] KVM: s390: protvirt: Add new gprs location handling
Quoted 117 lines · David Hildenbrand
Quoted 116 lines · Janosch Frank
Quoted 103 lines · David Hildenbrand
Quoted 79 lines · Janosch Frank
Quoted 78 lines · Christian Borntraeger
Quoted 58 lines · David Hildenbrand
Quoted 44 lines · Janosch Frank
Guest registers for protected guests are stored at offset
0x380.Signed-off-by Janosch Frank <[email protected]>---
arch/s390/include/asm/kvm_host.h| 4 +++- arch/s390/kvm/kvm-s390.c | 11 +++++++++++2 files changed, 14 insertions(+), 1 deletion(-)arch/s390/include/asm/kvm_host.h +0
-0@@ -336,7 +336,9 @@ struct kvm_s390_itdb { struct sie_page { struct kvm_s390_sie_block sie_block;struct
mcck_volatile_infomcck_info; /*0x0200*/ -__u8reserved218[1000]; /*0x0218*/
+__u8reserved218[360]; /*0x0218*/
+__u64pv_grregs[16]; /*0x380*/
+__u8reserved400[512];
structkvm_s390_itdbitdb; /*0x0600*/__u8reserved700[2304]; /*0x0700*/ };arch/s390/kvm/kvm-s390.c +1
-0@@ -3965,6 +3965,7 @@ static int vcpu_post_run(struct kvm_vcpu *vcpu, int exit_reason) static int __vcpu_run(struct kvm_vcpu *vcpu) { int rc, exit_reason; + struct sie_page *sie_page = (struct sie_page *)vcpu->arch.sie_block; /** We try to hold
kvm->srcuduring most ofvcpu_run(except when run-arch/s390/kvm/kvm-s390.c +2
-0@@ -3986,8 +3987,18 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) guest_enter_irqoff(); __disable_cpu_timer_accounting(vcpu); local_irq_enable(); + if (kvm_s390_pv_is_protected(vcpu->kvm)) { + memcpy(sie_page->pv_grregs,arch/s390/kvm/kvm-s390.c static int __vcpu_run(struct kvm_vcpu *vcpu) · line 4001
Quoted 10 lines
arch/s390/kvm/kvm-s390.c +8
-0+ vcpu->run->s.regs.gprs, + sizeof(sie_page->pv_grregs)); + } exit_reason = sie64a(vcpu->arch.sie_block, vcpu->run->s.regs.gprs); + if (kvm_s390_pv_is_protected(vcpu->kvm)) { + memcpy(vcpu->run->s.regs.gprs, + sie_page->pv_grregs, + sizeof(sie_page->pv_grregs)); + }sie64a will load/save gprs 0-13 from to
vcpu->run->s.regs.gprs.Quoted 2 lines
I would have assume that this is not required for prot virt, because the HW has direct access via the sie block?
Yes, that is correct. The load/save in sie64a is not necessary for pv guests.
Quoted 6 lines
1. Would it make sense to have a specialized
sie64a()(or a parameter, e.g., if you pass in NULL in r3), that optimizes this loading/saving? Eventually we can also optimize which host registers to save/restore then.Having 2 kinds of sie64a seems not very nice for just saving a small number of cycles.
Quoted 1 line
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
2. Avoid this copying here. We have to store the state to
vcpu->run->s.regs.gprswhen returning to user space and restore the state when coming from user space.I like this proposal better than the first one and
It was actually an additional proposal :)
1. avoids unnecessary saving/loading/saving/restoring
2. avoids the two memcpyQuoted 12 lines
Quoted 12 lines
Quoted 6 lines
Also, we access the GPRS from interception handlers, there we might use wrappers like
kvm_s390_set_gprs()kvm_s390_get_gprs()having register accessors might be useful anyway.
But I would like to defer that to a later point in time to keep the changes in here
minimal?Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
We can add a "TODO" comment in here so that we do not forget about this
for a future patch. Makes sense?While it makes sense, I guess one could come up with a patch for 2. in less than 30 minutes ... but yeah, whatever you prefer. ;)
Just to get it fully right we'd need to:
a. Synchronize registers into/from vcpu run in sync_regs/store_regs b. Sprinkle get/set_gpr(int nr) over most of the files inarch/s390/kvm
Quoted 7 lines
Quoted 1 line
That's your proposal?
Yes. Patch 1, factor out gprs access. Patch 2, avoid the memcpy by fixing the gprs access functions and removing the memcpys. (both as addons to this patch)
I guess that should be it ... but maybe we'll stumble over surprises :)
That's likely a good idea, but I think I agree with Christian that this should rather be done in a later, separate patch. This patch set is already big enough, so I'd prefer to keep it shorter for now and do optimizations later.
Thomas
Re: [RFC 19/37] KVM: s390: protvirt: Add new gprs location handling
In reply to Janosch FrankQuoted 13 lines · Janosch Frank
Guest registers for protected guests are stored at offset
0x380.Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/asm/kvm_host.h | 4 +++- arch/s390/kvm/kvm-s390.c | 11 +++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-)arch/s390/include/asm/kvm_host.h +0
-0
arch/s390/include/asm/kvm_host.h struct kvm_s390_itdb { · line 341
Quoted 7 lines
arch/s390/include/asm/kvm_host.h +3
-1@@ -336,7 +336,9 @@ struct kvm_s390_itdb {struct sie_page {struct kvm_s390_sie_block sie_block;struct mcck_volatile_info mcck_info;/* 0x0200 */ -__u8 reserved218[1000 ];/* 0x0218 */ +__u8 reserved218[360 ];/* 0x0218 */ +__u64 pv_grregs[16 ];/* 0x380 */ +__u8 reserved400[512 ];
Maybe add a "/* 0x400 */" comment to be consisten with the other lines?
arch/s390/include/asm/kvm_host.h struct kvm_s390_itdb { · line 344
Quoted 3 lines
arch/s390/include/asm/kvm_host.h +0
-0struct kvm_s390_itdb itdb;/* 0x0600 */ __u8 reserved700[2304 ];/* 0x0700 */ };
Thomas
Janosch Frank
maintainerRe: [RFC 19/37] KVM: s390: protvirt: Add new gprs location handling
Quoted 9 lines · Thomas Huth
Quoted 8 lines · Janosch Frank
Guest registers for protected guests are stored at offset
0x380.Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/asm/kvm_host.h | 4 +++- arch/s390/kvm/kvm-s390.c | 11 +++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-)
Quoted 14 lines
arch/s390/include/asm/kvm_host.h struct kvm_s390_itdb { · line 341
Quoted 12 lines
arch/s390/include/asm/kvm_host.h +3
-1@@ -336,7 +336,9 @@ struct kvm_s390_itdb {struct sie_page {struct kvm_s390_sie_block sie_block;struct mcck_volatile_info mcck_info;/* 0x0200 */ -__u8 reserved218[1000 ];/* 0x0218 */ +__u8 reserved218[360 ];/* 0x0218 */ +__u64 pv_grregs[16 ];/* 0x380 */ +__u8 reserved400[512 ];Maybe add a "/*
0x400*/" comment to be consisten with the other lines?
Sure
Quoted 7 lines
arch/s390/include/asm/kvm_host.h struct kvm_s390_itdb { · line 344
Quoted 4 lines
arch/s390/include/asm/kvm_host.h +0
-0struct kvm_s390_itdb itdb;/* 0x0600 */ __u8 reserved700[2304 ];/* 0x0700 */ };Thomas
Janosch Frank
maintainerRFC 20/37 KVM: S390: protvirt: Introduce instruction data area bounce buffer
In reply to Janosch FrankNow that we can't access guest memory anymore, we have a dedicated sattelite block that's a bounce buffer for instruction data.
We re-use the memop interface to copy the instruction data to / from userspace. This lets us re-use a lot of QEMU code which used that interface to make logical guest memory accesses which are not possible anymore in protected mode anyway.
--- arch/s390/include/asm/kvm_host.h | 5 ++++- arch/s390/kvm/kvm-s390.c | 31 +++++++++++++++++++++++++++++++ arch/s390/kvm/pv.c | 9 +++++++++ 3 files changed, 44 insertions(+), 1 deletion(-)
arch/s390/include/asm/kvm_host.h +4 -1
@@ -308,7 +308,10 @@ struct kvm_s390_sie_block {#define CRYCB_FORMAT2 0x00000003__u32 crycbd;/* 0x00fc */ __u64 gcr[16 ];/* 0x0100 */ -__u64 gbea;/* 0x0180 */ +union { +__u64 gbea;/* 0x0180 */ +__u64 sidad; + };__u8 reserved188[8 ];/* 0x0188 */ __u64 sdnxo;/* 0x0190 */ __u8 reserved198[8 ];/* 0x0198 */
arch/s390/kvm/kvm-s390.c +31 -0
@@ -4416,6 +4416,13 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,if (mop->size >MEM_OP_MAX_SIZE )return -E2BIG ; +/* Protected guests move instruction data over the satellite +* block which has its own size limit +*/ +if (kvm_s390_pv_is_protected (vcpu->kvm) && + mop->size > ((vcpu->arch.sie_block->sidad &0x0f ) +1 ) *PAGE_SIZE ) +return -E2BIG ; +if (!(mop->flags &KVM_S390_MEMOP_F_CHECK_ONLY )) { tmpbuf =vmalloc (mop->size);if (!tmpbuf)
@@ -4427,10 +4434,22 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,switch (mop->op) {case KVM_S390_MEMOP_LOGICAL_READ :if (mop->flags &KVM_S390_MEMOP_F_CHECK_ONLY ) { +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + r =0 ; +break ; + } r =check_gva_range (vcpu, mop->gaddr, mop->ar, mop->size,GACC_FETCH );break ; } +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + r =0 ; +if (copy_to_user (uaddr, (void *)vcpu->arch.sie_block->sidad + + (mop->gaddr & ~PAGE_MASK ), + mop->size)) + r = -EFAULT ; +break ; + } r =read_guest (vcpu, mop->gaddr, mop->ar, tmpbuf, mop->size);if (r ==0 ) {if (copy_to_user (uaddr, tmpbuf, mop->size))
@@ -4439,10 +4458,22 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,break ;case KVM_S390_MEMOP_LOGICAL_WRITE :if (mop->flags &KVM_S390_MEMOP_F_CHECK_ONLY ) { +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + r =0 ; +break ; + } r =check_gva_range (vcpu, mop->gaddr, mop->ar, mop->size,GACC_STORE );break ; } +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + r =0 ; +if (copy_from_user ((void *)vcpu->arch.sie_block->sidad + + (mop->gaddr & ~PAGE_MASK ), uaddr, + mop->size)) + r = -EFAULT ; +break ; + }if (copy_from_user (tmpbuf, uaddr, mop->size)) { r = -EFAULT ;break ;
arch/s390/kvm/pv.c +9 -0
@@ -119,6 +119,7 @@ int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu)free_pages (vcpu->arch.pv.stor_base,get_order (uv_info.guest_cpu_stor_len)); +free_page (vcpu->arch.sie_block->sidad);/* Clear cpu and vm handle */ memset (&vcpu->arch.sie_block->reserved10,0 ,sizeof (vcpu->arch.sie_block->reserved10));
@@ -150,6 +151,14 @@ int kvm_s390_pv_create_cpu(struct kvm_vcpu *vcpu) uvcb.state_origin = (u64 )vcpu->arch.sie_block; uvcb.stor_origin = (u64 )vcpu->arch.pv.stor_base; +/* Alloc Secure Instruction Data Area Designation */ + vcpu->arch.sie_block->sidad =__get_free_page (GFP_KERNEL | __GFP_ZERO); +if (!vcpu->arch.sie_block->sidad) { +free_pages (vcpu->arch.pv.stor_base, +get_order (uv_info.guest_cpu_stor_len)); +return -ENOMEM ; + } + rc =uv_call (0 , (u64)&uvcb); VCPU_EVENT(vcpu, 3, "PROTVIRT CREATE VCPU: cpu %d handle %llx rc %x rrc %x", vcpu->vcpu_id, uvcb.cpu_handle, uvcb.header.rc,
Signature · 1 line
2.20.1
Re: [RFC 20/37] KVM: S390: protvirt: Introduce instruction data area bounce buffer
Quoted 2 lines · Janosch Frank
Now that we can't access guest memory anymore, we have a dedicated sattelite block that's a bounce buffer for instruction data.
"satellite block that is ..."
Quoted 47 lines
We re-use the memop interface to copy the instruction data to / from userspace. This lets us re-use a lot of QEMU code which used that interface to make logical guest memory accesses which are not possible anymore in protected mode anyway.
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/asm/kvm_host.h | 5 ++++- arch/s390/kvm/kvm-s390.c | 31 +++++++++++++++++++++++++++++++ arch/s390/kvm/pv.c | 9 +++++++++ 3 files changed, 44 insertions(+), 1 deletion(-)arch/s390/include/asm/kvm_host.h +4
-1@@ -308,7 +308,10 @@ struct kvm_s390_sie_block {#define CRYCB_FORMAT2 0x00000003__u32 crycbd;/* 0x00fc */ __u64 gcr[16 ];/* 0x0100 */ -__u64 gbea;/* 0x0180 */ +union { +__u64 gbea;/* 0x0180 */ +__u64 sidad; + };__u8 reserved188[8 ];/* 0x0188 */ __u64 sdnxo;/* 0x0190 */ __u8 reserved198[8 ];/* 0x0198 */ arch/s390/kvm/kvm-s390.c +7
-0@@ -4416,6 +4416,13 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,if (mop->size >MEM_OP_MAX_SIZE )return -E2BIG ; +/* Protected guests move instruction data over the satellite +* block which has its own size limit +*/ +if (kvm_s390_pv_is_protected (vcpu->kvm) && + mop->size > ((vcpu->arch.sie_block->sidad &0x0f ) +1 ) *PAGE_SIZE ) +return -E2BIG ; +if (!(mop->flags &KVM_S390_MEMOP_F_CHECK_ONLY )) { tmpbuf =vmalloc (mop->size);if (!tmpbuf)
arch/s390/kvm/kvm-s390.c static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu, · line 4439
Quoted 6 lines
arch/s390/kvm/kvm-s390.c +3
-0@@ -4427,10 +4434,22 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,switch (mop->op) {case KVM_S390_MEMOP_LOGICAL_READ :if (mop->flags &KVM_S390_MEMOP_F_CHECK_ONLY ) { +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + r =0 ; +break ;
Please add a short comment to the code why this is required / ok.
arch/s390/kvm/kvm-s390.c static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu, · line 4448
Quoted 9 lines
arch/s390/kvm/kvm-s390.c +5
-0+ } r =check_gva_range (vcpu, mop->gaddr, mop->ar, mop->size,GACC_FETCH );break ; } +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + r =0 ; + if (copy_to_user(uaddr, (void *)vcpu->arch.sie_block->sidad + + (mop->gaddr & ~PAGE_MASK),
That looks bogus. Couldn't userspace use mop->gaddr = 4095 and mop->size = 4095 to read most of the page beyond the sidad page (assuming that it is mapped, too)?
I think you have to take mop->gaddr into account in your new check at the beginning of the function, too.
Or should the ioctl maybe even be restricted to mop->gaddr == 0 now? Is there maybe also a way to validate that gaddr & PAGE_MASK really matches the page that we have in sidad?
Quoted 14 lines
arch/s390/kvm/kvm-s390.c +7
-0+ mop->size)) + r = -EFAULT ; +break ; + } r =read_guest (vcpu, mop->gaddr, mop->ar, tmpbuf, mop->size);if (r ==0 ) {if (copy_to_user (uaddr, tmpbuf, mop->size))@@ -4439,10 +4458,22 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,break ;case KVM_S390_MEMOP_LOGICAL_WRITE :if (mop->flags &KVM_S390_MEMOP_F_CHECK_ONLY ) { +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + r =0 ; +break ;
arch/s390/kvm/kvm-s390.c static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu, · line 4473
Quoted 10 lines
arch/s390/kvm/kvm-s390.c +6
-0+ } r =check_gva_range (vcpu, mop->gaddr, mop->ar, mop->size,GACC_STORE );break ; } +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + r =0 ; +if (copy_from_user ((void *)vcpu->arch.sie_block->sidad + + (mop->gaddr & ~PAGE_MASK ), uaddr, + mop->size))
dito, of course.
arch/s390/kvm/kvm-s390.c static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu, · line 4479
Quoted 6 lines
arch/s390/kvm/kvm-s390.c +3
-0+ r = -EFAULT ; +break ; + }if (copy_from_user (tmpbuf, uaddr, mop->size)) { r = -EFAULT ;break ;
Thomas
Janosch Frank
maintainerRe: [RFC 20/37] KVM: S390: protvirt: Introduce instruction data area bounce buffer
Quoted 62 lines · Thomas Huth
Quoted 2 lines · Janosch Frank
Now that we can't access guest memory anymore, we have a dedicated sattelite block that's a bounce buffer for instruction data.
"satellite block that is ..."
Quoted 47 lines
We re-use the memop interface to copy the instruction data to / from userspace. This lets us re-use a lot of QEMU code which used that interface to make logical guest memory accesses which are not possible anymore in protected mode anyway.
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/asm/kvm_host.h | 5 ++++- arch/s390/kvm/kvm-s390.c | 31 +++++++++++++++++++++++++++++++ arch/s390/kvm/pv.c | 9 +++++++++ 3 files changed, 44 insertions(+), 1 deletion(-)arch/s390/include/asm/kvm_host.h +4
-1@@ -308,7 +308,10 @@ struct kvm_s390_sie_block {#define CRYCB_FORMAT2 0x00000003__u32 crycbd;/* 0x00fc */ __u64 gcr[16 ];/* 0x0100 */ -__u64 gbea;/* 0x0180 */ +union { +__u64 gbea;/* 0x0180 */ +__u64 sidad; + };__u8 reserved188[8 ];/* 0x0188 */ __u64 sdnxo;/* 0x0190 */ __u8 reserved198[8 ];/* 0x0198 */ arch/s390/kvm/kvm-s390.c +7
-0@@ -4416,6 +4416,13 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,if (mop->size >MEM_OP_MAX_SIZE )return -E2BIG ; +/* Protected guests move instruction data over the satellite +* block which has its own size limit +*/ +if (kvm_s390_pv_is_protected (vcpu->kvm) && + mop->size > ((vcpu->arch.sie_block->sidad &0x0f ) +1 ) *PAGE_SIZE ) +return -E2BIG ; +if (!(mop->flags &KVM_S390_MEMOP_F_CHECK_ONLY )) { tmpbuf =vmalloc (mop->size);if (!tmpbuf)arch/s390/kvm/kvm-s390.c static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu, · line 4439
Quoted 6 lines
arch/s390/kvm/kvm-s390.c +3
-0@@ -4427,10 +4434,22 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,switch (mop->op) {case KVM_S390_MEMOP_LOGICAL_READ :if (mop->flags &KVM_S390_MEMOP_F_CHECK_ONLY ) { +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + r =0 ; +break ;Please add a short comment to the code why this is required / ok.
Quoted 15 lines
arch/s390/kvm/kvm-s390.c static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu, · line 4448
Quoted 9 lines
arch/s390/kvm/kvm-s390.c +5
-0+ } r =check_gva_range (vcpu, mop->gaddr, mop->ar, mop->size,GACC_FETCH );break ; } +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + r =0 ; + if (copy_to_user(uaddr, (void *)vcpu->arch.sie_block->sidad + + (mop->gaddr & ~PAGE_MASK),That looks bogus. Couldn't userspace use
mop->gaddr= 4095 andmop->size= 4095 to read most of the page beyond the sidad page (assuming that it is mapped, too)?
I think you have to takemop->gaddrinto account in your new check at the beginning of the function, too.
Ah, right, that needs some fixing.
Quoted 4 lines
Or should the ioctl maybe even be restricted to
mop->gaddr== 0 now? Is there maybe also a way to validate that gaddr &PAGE_MASKreally matches the page that we have in sidad?
There was one lonely usage of the ioctl where we still read from an offset, either in IO or SCLP. Having 0 as a requirement would certainly help, but I was a bit afraid of changing too many things in qemu.
Quoted 37 lines
Quoted 15 lines
arch/s390/kvm/kvm-s390.c +7
-0+ mop->size)) + r = -EFAULT ; +break ; + } r =read_guest (vcpu, mop->gaddr, mop->ar, tmpbuf, mop->size);if (r ==0 ) {if (copy_to_user (uaddr, tmpbuf, mop->size))@@ -4439,10 +4458,22 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,break ;case KVM_S390_MEMOP_LOGICAL_WRITE :if (mop->flags &KVM_S390_MEMOP_F_CHECK_ONLY ) { +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + r =0 ; +break ;arch/s390/kvm/kvm-s390.c static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu, · line 4473
Quoted 10 lines
arch/s390/kvm/kvm-s390.c +6
-0+ } r =check_gva_range (vcpu, mop->gaddr, mop->ar, mop->size,GACC_STORE );break ; } +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + r =0 ; +if (copy_from_user ((void *)vcpu->arch.sie_block->sidad + + (mop->gaddr & ~PAGE_MASK ), uaddr, + mop->size))dito, of course.
arch/s390/kvm/kvm-s390.c static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu, · line 4479
Quoted 6 lines
arch/s390/kvm/kvm-s390.c +3
-0+ r = -EFAULT ; +break ; + }if (copy_from_user (tmpbuf, uaddr, mop->size)) { r = -EFAULT ;break ;Thomas
Janosch Frank
maintainerPATCH 1/1 Fixup sida bouncing
In reply to Thomas Huth--- arch/s390/kvm/kvm-s390.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)
arch/s390/kvm/kvm-s390.c +13 -6
@@ -4432,13 +4432,21 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,if (mop->size >MEM_OP_MAX_SIZE )return -E2BIG ; -/* Protected guests move instruction data over the satellite +/* +* Protected guests move instruction data over the satellite * block which has its own size limit */ if (kvm_s390_pv_is_protected (vcpu->kvm) && - mop->size > ((vcpu->arch.sie_block->sidad &0x0f ) +1 ) *PAGE_SIZE ) + mop->size > ((vcpu->arch.sie_block->sidad &0xff ) +1 ) *PAGE_SIZE )return -E2BIG ; +/* We can currently only offset into the one SIDA page. */ +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + mop->gaddr &= ~PAGE_MASK ; +if (mop->gaddr + mop->size >PAGE_SIZE ) +return -EINVAL ; + } +if (!(mop->flags &KVM_S390_MEMOP_F_CHECK_ONLY )) { tmpbuf =vmalloc (mop->size);if (!tmpbuf)
@@ -4451,6 +4459,7 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,case KVM_S390_MEMOP_LOGICAL_READ :if (mop->flags &KVM_S390_MEMOP_F_CHECK_ONLY ) {if (kvm_s390_pv_is_protected (vcpu->kvm)) { +/* We can always copy into the SIDA */ r =0 ;break ; }
@@ -4461,8 +4470,7 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,if (kvm_s390_pv_is_protected (vcpu->kvm)) { r =0 ;if (copy_to_user (uaddr, (void *)vcpu->arch.sie_block->sidad + - (mop->gaddr & ~PAGE_MASK ), - mop->size)) + mop->gaddr, mop->size)) r = -EFAULT ;break ; }
@@ -4485,8 +4493,7 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,if (kvm_s390_pv_is_protected (vcpu->kvm)) { r =0 ;if (copy_from_user ((void *)vcpu->arch.sie_block->sidad + - (mop->gaddr & ~PAGE_MASK ), uaddr, - mop->size)) + mop->gaddr, uaddr, mop->size)) r = -EFAULT ;break ; }
Signature · 1 line
2.20.1
Re: [PATCH] Fixup sida bouncing
Quoted 53 lines · Janosch Frank
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/kvm-s390.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)arch/s390/kvm/kvm-s390.c +12
-4@@ -4432,13 +4432,21 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,if (mop->size >MEM_OP_MAX_SIZE )return -E2BIG ; -/* Protected guests move instruction data over the satellite +/* +* Protected guests move instruction data over the satellite * block which has its own size limit */ if (kvm_s390_pv_is_protected (vcpu->kvm) && - mop->size > ((vcpu->arch.sie_block->sidad &0x0f ) +1 ) *PAGE_SIZE ) + mop->size > ((vcpu->arch.sie_block->sidad &0xff ) +1 ) *PAGE_SIZE )return -E2BIG ; +/* We can currently only offset into the one SIDA page. */ +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + mop->gaddr &= ~PAGE_MASK ; +if (mop->gaddr + mop->size >PAGE_SIZE ) +return -EINVAL ; + } +if (!(mop->flags &KVM_S390_MEMOP_F_CHECK_ONLY )) { tmpbuf =vmalloc (mop->size);if (!tmpbuf)@@ -4451,6 +4459,7 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,case KVM_S390_MEMOP_LOGICAL_READ :if (mop->flags &KVM_S390_MEMOP_F_CHECK_ONLY ) {if (kvm_s390_pv_is_protected (vcpu->kvm)) { +/* We can always copy into the SIDA */ r =0 ;break ; }@@ -4461,8 +4470,7 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,if (kvm_s390_pv_is_protected (vcpu->kvm)) { r =0 ;if (copy_to_user (uaddr, (void *)vcpu->arch.sie_block->sidad + - (mop->gaddr & ~PAGE_MASK ), - mop->size)) + mop->gaddr, mop->size)) r = -EFAULT ;break ; }
arch/s390/kvm/kvm-s390.c static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu, · line 4499
Quoted 9 lines
arch/s390/kvm/kvm-s390.c +1
-2@@ -4485,8 +4493,7 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,if (kvm_s390_pv_is_protected (vcpu->kvm)) { r =0 ;if (copy_from_user ((void *)vcpu->arch.sie_block->sidad + - (mop->gaddr & ~PAGE_MASK ), uaddr, - mop->size)) + mop->gaddr, uaddr, mop->size)) r = -EFAULT ;break ; }
That looks better, indeed.
Still, is there a way you could also verify that gaddr references the right page that is mirrored in the sidad?
Thomas
Janosch Frank
maintainerRe: [PATCH] Fixup sida bouncing
Quoted 66 lines · Thomas Huth
Quoted 53 lines · Janosch Frank
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/kvm-s390.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)arch/s390/kvm/kvm-s390.c +12
-4@@ -4432,13 +4432,21 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,if (mop->size >MEM_OP_MAX_SIZE )return -E2BIG ; -/* Protected guests move instruction data over the satellite +/* +* Protected guests move instruction data over the satellite * block which has its own size limit */ if (kvm_s390_pv_is_protected (vcpu->kvm) && - mop->size > ((vcpu->arch.sie_block->sidad &0x0f ) +1 ) *PAGE_SIZE ) + mop->size > ((vcpu->arch.sie_block->sidad &0xff ) +1 ) *PAGE_SIZE )return -E2BIG ; +/* We can currently only offset into the one SIDA page. */ +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + mop->gaddr &= ~PAGE_MASK ; +if (mop->gaddr + mop->size >PAGE_SIZE ) +return -EINVAL ; + } +if (!(mop->flags &KVM_S390_MEMOP_F_CHECK_ONLY )) { tmpbuf =vmalloc (mop->size);if (!tmpbuf)@@ -4451,6 +4459,7 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,case KVM_S390_MEMOP_LOGICAL_READ :if (mop->flags &KVM_S390_MEMOP_F_CHECK_ONLY ) {if (kvm_s390_pv_is_protected (vcpu->kvm)) { +/* We can always copy into the SIDA */ r =0 ;break ; }@@ -4461,8 +4470,7 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,if (kvm_s390_pv_is_protected (vcpu->kvm)) { r =0 ;if (copy_to_user (uaddr, (void *)vcpu->arch.sie_block->sidad + - (mop->gaddr & ~PAGE_MASK ), - mop->size)) + mop->gaddr, mop->size)) r = -EFAULT ;break ; }arch/s390/kvm/kvm-s390.c static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu, · line 4499
Quoted 9 lines
arch/s390/kvm/kvm-s390.c +1
-2@@ -4485,8 +4493,7 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,if (kvm_s390_pv_is_protected (vcpu->kvm)) { r =0 ;if (copy_from_user ((void *)vcpu->arch.sie_block->sidad + - (mop->gaddr & ~PAGE_MASK ), uaddr, - mop->size)) + mop->gaddr, uaddr, mop->size)) r = -EFAULT ;break ; }That looks better, indeed.
Quoted 5 lines
Still, is there a way you could also verify that gaddr references the right page that is mirrored in the sidad?
Thomas
I'm not completely sure if I understand your question correctly. Checking that is not possible here without also looking at the instruction bytecode and register contents which would make this patch ridiculously large with no real benefit.
Re: [PATCH] Fixup sida bouncing
Quoted 1 line · Janosch Frank
On 11/15/19 9:19 AM, Thomas Huth wrote:
[...]
Quoted 10 lines
Quoted 5 lines
Still, is there a way you could also verify that gaddr references the right page that is mirrored in the sidad?
Thomas
I'm not completely sure if I understand your question correctly. Checking that is not possible here without also looking at the instruction bytecode and register contents which would make this patch ridiculously large with no real benefit.
Yes, I was thinking about something like that. I mean, how can you be sure that the userspace really only wants to read the contents that are references by the sidad? It could also try to read or write e.g. the lowcore data inbetween (assuming that there are some code paths left which are not aware of protected virtualization yet)?
Well, it does not have to be right now and in this patch, but I still think that's something that should be added in the future if somehow possible...
Thomas
Janosch Frank
maintainerRFC 21/37 KVM: S390: protvirt: Instruction emulation
In reply to Janosch FrankWe have two new SIE exit codes 104 for a secure instruction interception, on which the SIE needs hypervisor action to complete the instruction.
And 108 which is merely a notification and provides data for tracking and management, like for the lowcore we set notification bits for the lowcore pages.
--- arch/s390/include/asm/kvm_host.h | 2 ++ arch/s390/kvm/intercept.c | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+)
arch/s390/include/asm/kvm_host.h +2 -0
@@ -212,6 +212,8 @@ struct kvm_s390_sie_block {#define ICPT_KSS 0x5c#define ICPT_PV_MCHKR 0x60#define ICPT_PV_INT_EN 0x64 +#define ICPT_PV_INSTR 0x68 +#define ICPT_PV_NOT 0x6c__u8 icptcode;/* 0x0050 */ __u8 icptstatus;/* 0x0051 */ __u16 ihcpu;/* 0x0052 */
arch/s390/kvm/intercept.c +23 -0
@@ -451,6 +451,23 @@ static int handle_operexc(struct kvm_vcpu *vcpu)return kvm_s390_inject_program_int (vcpu,PGM_OPERATION ); } +static int handle_pv_spx (struct kvm_vcpu *vcpu) +{ +u32 pref = *(u32 *)vcpu->arch.sie_block->sidad; + +kvm_s390_set_prefix (vcpu, pref); +trace_kvm_s390_handle_prefix (vcpu,1 , pref); +return 0 ; +} + +static int handle_pv_not (struct kvm_vcpu *vcpu) +{ +if (vcpu->arch.sie_block->ipa ==0xb210 ) +return handle_pv_spx (vcpu); + +return handle_instruction (vcpu); +} +int kvm_handle_sie_intercept (struct kvm_vcpu *vcpu) {int rc, per_rc =0 ;
@@ -505,6 +522,12 @@ int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)*/ rc =0 ;break ; +case ICPT_PV_INSTR : + rc =handle_instruction (vcpu); +break ; +case ICPT_PV_NOT : + rc =handle_pv_not (vcpu); +break ;default :return -EOPNOTSUPP ; }
Signature · 1 line
2.20.1
Re: [RFC 21/37] KVM: S390: protvirt: Instruction emulation
Quoted 7 lines · On Thu, 24 Oct 2019 07:40:43 -0400 Janosch Frank <[email protected]> wrote:
We have two new SIE exit codes 104 for a secure instruction interception, on which the SIE needs hypervisor action to complete the instruction.
And 108 which is merely a notification and provides data for tracking and management, like for the lowcore we set notification bits for the lowcore pages.
What about the following:
"With protected virtualization, we have two new SIE exit codes:
- 104 indicates a secure instruction interception; the hypervisor needs to complete emulation of the instruction.
- 108 is merely a notification providing data for tracking and management in the hypervisor; for example, we set notification bits for the lowcore pages."
?
Quoted 12 lines
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/asm/kvm_host.h | 2 ++ arch/s390/kvm/intercept.c | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+)arch/s390/include/asm/kvm_host.h +0
-0
arch/s390/include/asm/kvm_host.h struct kvm_s390_sie_block { · line 216
Quoted 5 lines
arch/s390/include/asm/kvm_host.h +2
-0@@ -212,6 +212,8 @@ struct kvm_s390_sie_block {#define ICPT_KSS 0x5c#define ICPT_PV_MCHKR 0x60#define ICPT_PV_INT_EN 0x64 +#define ICPT_PV_INSTR 0x68 +#define ICPT_PV_NOT 0x6c
Maybe ICPT_PV_NOTIF?
Quoted 16 lines
arch/s390/include/asm/kvm_host.h +0
-0__u8 icptcode;/* 0x0050 */ __u8 icptstatus;/* 0x0051 */ __u16 ihcpu;/* 0x0052 */ arch/s390/kvm/intercept.c +5
-0@@ -451,6 +451,23 @@ static int handle_operexc(struct kvm_vcpu *vcpu)return kvm_s390_inject_program_int (vcpu,PGM_OPERATION ); } +static int handle_pv_spx (struct kvm_vcpu *vcpu) +{ +u32 pref = *(u32 *)vcpu->arch.sie_block->sidad; + +kvm_s390_set_prefix (vcpu, pref);
arch/s390/kvm/intercept.c static int handle_operexc(struct kvm_vcpu *vcpu) · line 468
Quoted 10 lines
arch/s390/kvm/intercept.c +10
-0+trace_kvm_s390_handle_prefix (vcpu,1 , pref); +return 0 ; +} + +static int handle_pv_not (struct kvm_vcpu *vcpu) +{ +if (vcpu->arch.sie_block->ipa ==0xb210 ) +return handle_pv_spx (vcpu); + +return handle_instruction (vcpu);
Hm... if I understood it correctly, we are getting this one because the SIE informs us about things that it handled itself (but which we should be aware of). What can handle_instruction() do in this case?
Quoted 18 lines
arch/s390/kvm/intercept.c +8
-0+} +int kvm_handle_sie_intercept (struct kvm_vcpu *vcpu) {int rc, per_rc =0 ;@@ -505,6 +522,12 @@ int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)*/ rc =0 ;break ; +case ICPT_PV_INSTR : + rc =handle_instruction (vcpu); +break ; +case ICPT_PV_NOT : + rc =handle_pv_not (vcpu); +break ;default :return -EOPNOTSUPP ; }
Janosch Frank
maintainerRe: [RFC 21/37] KVM: S390: protvirt: Instruction emulation
Quoted 30 lines · Cornelia Huck
Quoted 7 lines · On Thu, 24 Oct 2019 07:40:43 -0400 Janosch Frank <[email protected]> wrote:
We have two new SIE exit codes 104 for a secure instruction interception, on which the SIE needs hypervisor action to complete the instruction.
And 108 which is merely a notification and provides data for tracking and management, like for the lowcore we set notification bits for the lowcore pages.
What about the following:
"With protected virtualization, we have two new SIE exit codes:
- 104 indicates a secure instruction interception; the hypervisor needs to complete emulation of the instruction.
- 108 is merely a notification providing data for tracking and management in the hypervisor; for example, we set notification bits for the lowcore pages."?
Quoted 7 lines
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/asm/kvm_host.h | 2 ++ arch/s390/kvm/intercept.c | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+)
Quoted 12 lines
arch/s390/include/asm/kvm_host.h struct kvm_s390_sie_block { · line 216
Quoted 10 lines
arch/s390/include/asm/kvm_host.h +2
-0@@ -212,6 +212,8 @@ struct kvm_s390_sie_block {#define ICPT_KSS 0x5c#define ICPT_PV_MCHKR 0x60#define ICPT_PV_INT_EN 0x64 +#define ICPT_PV_INSTR 0x68 +#define ICPT_PV_NOT 0x6cMaybe
ICPT_PV_NOTIF?
NOTF?
Quoted 23 lines
Quoted 23 lines
arch/s390/include/asm/kvm_host.h +0
-0__u8 icptcode;/* 0x0050 */ __u8 icptstatus;/* 0x0051 */ __u16 ihcpu;/* 0x0052 */ arch/s390/kvm/intercept.c +11
-0@@ -451,6 +451,23 @@ static int handle_operexc(struct kvm_vcpu *vcpu)return kvm_s390_inject_program_int (vcpu,PGM_OPERATION ); } +static int handle_pv_spx (struct kvm_vcpu *vcpu) +{ +u32 pref = *(u32 *)vcpu->arch.sie_block->sidad; + +kvm_s390_set_prefix (vcpu, pref); +trace_kvm_s390_handle_prefix (vcpu,1 , pref); +return 0 ; +} + +static int handle_pv_not (struct kvm_vcpu *vcpu) +{
Quoted 8 lines
arch/s390/kvm/intercept.c static int handle_operexc(struct kvm_vcpu *vcpu) · line 468
Quoted 4 lines
arch/s390/kvm/intercept.c +4
-0+if (vcpu->arch.sie_block->ipa ==0xb210 ) +return handle_pv_spx (vcpu); + +return handle_instruction (vcpu);Hm... if I understood it correctly, we are getting this one because the SIE informs us about things that it handled itself (but which we should be aware of). What can
handle_instruction()do in this case?
There used to be an instruction which I could just pipe through normal instruction handling. But I can't really remember what it was, too many firmware changes in that area since then.
I'll mark it as a TODO for thinking about it with some coffee.
Quoted 20 lines
Quoted 19 lines
arch/s390/kvm/intercept.c +8
-0+} +int kvm_handle_sie_intercept (struct kvm_vcpu *vcpu) {int rc, per_rc =0 ;@@ -505,6 +522,12 @@ int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)*/ rc =0 ;break ; +case ICPT_PV_INSTR : + rc =handle_instruction (vcpu); +break ; +case ICPT_PV_NOT : + rc =handle_pv_not (vcpu); +break ;default :return -EOPNOTSUPP ; }
Re: [RFC 21/37] KVM: S390: protvirt: Instruction emulation
Quoted 44 lines · On Thu, 14 Nov 2019 17:00:41 +0100 Janosch Frank <[email protected]> wrote:
Quoted 30 lines · Cornelia Huck
Quoted 7 lines · On Thu, 24 Oct 2019 07:40:43 -0400 Janosch Frank <[email protected]> wrote:
We have two new SIE exit codes 104 for a secure instruction interception, on which the SIE needs hypervisor action to complete the instruction.
And 108 which is merely a notification and provides data for tracking and management, like for the lowcore we set notification bits for the lowcore pages.
What about the following:
"With protected virtualization, we have two new SIE exit codes:
- 104 indicates a secure instruction interception; the hypervisor needs to complete emulation of the instruction.
- 108 is merely a notification providing data for tracking and management in the hypervisor; for example, we set notification bits for the lowcore pages."?
Quoted 7 lines
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/asm/kvm_host.h | 2 ++ arch/s390/kvm/intercept.c | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+)Quoted 12 lines
arch/s390/include/asm/kvm_host.h struct kvm_s390_sie_block { · line 216
Quoted 10 lines
arch/s390/include/asm/kvm_host.h +2
-0@@ -212,6 +212,8 @@ struct kvm_s390_sie_block {#define ICPT_KSS 0x5c#define ICPT_PV_MCHKR 0x60#define ICPT_PV_INT_EN 0x64 +#define ICPT_PV_INSTR 0x68 +#define ICPT_PV_NOT 0x6cMaybe
ICPT_PV_NOTIF?
Quoted 1 line
NOTF?
Sounds good.
Quoted 33 lines
Quoted 24 lines
Quoted 24 lines
arch/s390/include/asm/kvm_host.h +0
-0__u8 icptcode;/* 0x0050 */ __u8 icptstatus;/* 0x0051 */ __u16 ihcpu;/* 0x0052 */ arch/s390/kvm/intercept.c +11
-0@@ -451,6 +451,23 @@ static int handle_operexc(struct kvm_vcpu *vcpu)return kvm_s390_inject_program_int (vcpu,PGM_OPERATION ); } +static int handle_pv_spx (struct kvm_vcpu *vcpu) +{ +u32 pref = *(u32 *)vcpu->arch.sie_block->sidad; + +kvm_s390_set_prefix (vcpu, pref); +trace_kvm_s390_handle_prefix (vcpu,1 , pref); +return 0 ; +} + +static int handle_pv_not (struct kvm_vcpu *vcpu) +{Quoted 8 lines
arch/s390/kvm/intercept.c static int handle_operexc(struct kvm_vcpu *vcpu) · line 468
Quoted 4 lines
arch/s390/kvm/intercept.c +4
-0+if (vcpu->arch.sie_block->ipa ==0xb210 ) +return handle_pv_spx (vcpu); + +return handle_instruction (vcpu);Hm... if I understood it correctly, we are getting this one because the SIE informs us about things that it handled itself (but which we should be aware of). What can
handle_instruction()do in this case?
Quoted 5 lines
There used to be an instruction which I could just pipe through normal instruction handling. But I can't really remember what it was, too many firmware changes in that area since then.
I'll mark it as a TODO for thinking about it with some coffee.
ok :)
Quoted 23 lines
Quoted 21 lines
Quoted 20 lines
arch/s390/kvm/intercept.c +8
-0+} +int kvm_handle_sie_intercept (struct kvm_vcpu *vcpu) {int rc, per_rc =0 ;@@ -505,6 +522,12 @@ int kvm_handle_sie_intercept(struct kvm_vcpu *vcpu)*/ rc =0 ;break ; +case ICPT_PV_INSTR : + rc =handle_instruction (vcpu); +break ; +case ICPT_PV_NOT : + rc =handle_pv_not (vcpu); +break ;default :return -EOPNOTSUPP ; }
Janosch Frank
maintainerRFC 22/37 KVM: s390: protvirt: Add SCLP handling
In reply to Janosch FrankFrom: Christian Borntraeger <[email protected]>
Unmask the SCLP IRQ once we get the notification intercept on SCLP.
--- arch/s390/kvm/intercept.c | 25 +++++++++++++++++++++++++ arch/s390/kvm/kvm-s390.c | 2 ++ 2 files changed, 27 insertions(+)
arch/s390/kvm/intercept.c +25 -0
@@ -460,11 +460,36 @@ static int handle_pv_spx(struct kvm_vcpu *vcpu)return 0 ; } +static int handle_pv_sclp (struct kvm_vcpu *vcpu) +{ +struct kvm_s390_float_interrupt *fi = &vcpu->kvm->arch.float_int; + +spin_lock (&fi->lock); +/* +* 2 cases: +* a: an sccb answering interrupt was already pending or in flight. +* As the sccb value is not used we can simply set some more bits +* and make sure that we deliver something +* b: an error sccb interrupt needs to be injected so we also inject +* something and let firmware do the right thing. +* This makes sure, that both errors and real sccb returns will only +* be delivered when we are unmasked. +*/ + fi->srv_signal.ext_params |=0x43000 ; +set_bit (IRQ_PEND_EXT_SERVICE , &fi->pending_irqs); +clear_bit (IRQ_PEND_EXT_SERVICE , &fi->masked_irqs); +spin_unlock (&fi->lock); +return 0 ; +} +static int handle_pv_not (struct kvm_vcpu *vcpu) {if (vcpu->arch.sie_block->ipa ==0xb210 )return handle_pv_spx (vcpu); +if (vcpu->arch.sie_block->ipa ==0xb220 ) +return handle_pv_sclp (vcpu); +return handle_instruction (vcpu); }
arch/s390/kvm/kvm-s390.c +2 -0
@@ -2189,6 +2189,8 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)if (!r) r =kvm_s390_pv_create_vm (kvm);kvm_s390_vcpu_unblock_all (kvm); +/* we need to block service interrupts from now on */ +set_bit (IRQ_PEND_EXT_SERVICE ,&kvm->arch.float_int.masked_irqs);mutex_unlock (&kvm->lock);break ; }
Signature · 1 line
2.20.1
Janosch Frank
maintainerRFC 23/37 KVM: s390: protvirt: Make sure prefix is always protected
In reply to Janosch Frank--- arch/s390/kvm/kvm-s390.c | 9 +++++++++ 1 file changed, 9 insertions(+)
arch/s390/kvm/kvm-s390.c +9 -0
@@ -3646,6 +3646,15 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) rc =gmap_mprotect_notify (vcpu->arch.gmap,kvm_s390_get_prefix (vcpu),PAGE_SIZE *2 ,PROT_WRITE ); +if (!rc &&kvm_s390_pv_is_protected (vcpu->kvm)) { + rc =uv_convert_to_secure (vcpu->arch.gmap, +kvm_s390_get_prefix (vcpu)); +WARN_ON_ONCE (rc && rc != -EEXIST ); + rc =uv_convert_to_secure (vcpu->arch.gmap, +kvm_s390_get_prefix (vcpu) +PAGE_SIZE ); +WARN_ON_ONCE (rc && rc != -EEXIST ); + rc =0 ; + }if (rc) {kvm_make_request (KVM_REQ_MMU_RELOAD , vcpu);return rc;
Signature · 1 line
2.20.1
Re: [RFC 23/37] KVM: s390: protvirt: Make sure prefix is always protected
On Thu, 24 Oct 2019 07:40:45 -0400
Janosch Frank <[email protected]> wrote:
Add at least a short sentence here?
Quoted 11 lines
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/kvm-s390.c | 9 +++++++++ 1 file changed, 9 insertions(+)arch/s390/kvm/kvm-s390.c +0
-0@@ -3646,6 +3646,15 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) rc = gmap_mprotect_notify(vcpu->arch.gmap,
arch/s390/kvm/kvm-s390.c static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) · line 3656
Quoted 10 lines
arch/s390/kvm/kvm-s390.c +8
-0kvm_s390_get_prefix (vcpu), PAGE_SIZE * 2, PROT_WRITE); +if (!rc &&kvm_s390_pv_is_protected (vcpu->kvm)) { + rc =uv_convert_to_secure (vcpu->arch.gmap, +kvm_s390_get_prefix (vcpu)); +WARN_ON_ONCE (rc && rc != -EEXIST ); + rc =uv_convert_to_secure (vcpu->arch.gmap, +kvm_s390_get_prefix (vcpu) +PAGE_SIZE ); +WARN_ON_ONCE (rc && rc != -EEXIST ); + rc =0 ;
So, what happens if we have an error other than -EEXIST (which presumably means that we already protected it) here? The page is not protected, and further accesses will get an error? (Another question:
what can actually go wrong here?)
Quoted 4 lines
arch/s390/kvm/kvm-s390.c +1
-0+ }if (rc) {kvm_make_request (KVM_REQ_MMU_RELOAD , vcpu);return rc;
Janosch Frank
maintainerRe: [RFC 23/37] KVM: s390: protvirt: Make sure prefix is always protected
Quoted 4 lines · Cornelia Huck
On Thu, 24 Oct 2019 07:40:45 -0400
Janosch Frank <[email protected]> wrote:Add at least a short sentence here?
For protected VMs the lowcore does not only need to be mapped, but also needs to be protected memory, if not we'll get a validity interception when trying to run it.
Quoted 19 lines
Quoted 19 lines
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/kvm-s390.c | 9 +++++++++ 1 file changed, 9 insertions(+)arch/s390/kvm/kvm-s390.c +5
-0@@ -3646,6 +3646,15 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) rc =gmap_mprotect_notify (vcpu->arch.gmap,kvm_s390_get_prefix (vcpu),PAGE_SIZE *2 ,PROT_WRITE ); +if (!rc &&kvm_s390_pv_is_protected (vcpu->kvm)) { + rc =uv_convert_to_secure (vcpu->arch.gmap, +kvm_s390_get_prefix (vcpu)); +WARN_ON_ONCE (rc && rc != -EEXIST ); + rc = uv_convert_to_secure(vcpu->arch.gmap,
Quoted 8 lines
arch/s390/kvm/kvm-s390.c static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) · line 3656
Quoted 3 lines
arch/s390/kvm/kvm-s390.c +3
-0+ kvm_s390_get_prefix(vcpu) + PAGE_SIZE); +WARN_ON_ONCE (rc && rc != -EEXIST ); + rc =0 ;So, what happens if we have an error other than
-EEXIST(which presumably means that we already protected it) here? The page is not protected, and further accesses will get an error? (Another question:
what can actually go wrong here?)
If KVM or QEMU write into a lowcore, we will fail the integrity check on import and this cpu will never run again.
In retrospect a warn_on_once might be the wrong error handling here.
Quoted 6 lines
Quoted 5 lines
arch/s390/kvm/kvm-s390.c +1
-0+ }if (rc) {kvm_make_request (KVM_REQ_MMU_RELOAD , vcpu);return rc;
Re: [RFC 23/37] KVM: s390: protvirt: Make sure prefix is always protected
Quoted 9 lines · On Tue, 19 Nov 2019 09:11:11 +0100 Janosch Frank <[email protected]> wrote:
Quoted 4 lines · Cornelia Huck
On Thu, 24 Oct 2019 07:40:45 -0400
Janosch Frank <[email protected]> wrote:Add at least a short sentence here?
For protected VMs the lowcore does not only need to be mapped, but also needs to be protected memory, if not we'll get a validity interception when trying to run it.
Much better, thanks!
Quoted 29 lines
Quoted 20 lines
Quoted 20 lines
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/kvm-s390.c | 9 +++++++++ 1 file changed, 9 insertions(+)arch/s390/kvm/kvm-s390.c +5
-0@@ -3646,6 +3646,15 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) rc =gmap_mprotect_notify (vcpu->arch.gmap,kvm_s390_get_prefix (vcpu),PAGE_SIZE *2 ,PROT_WRITE ); +if (!rc &&kvm_s390_pv_is_protected (vcpu->kvm)) { + rc =uv_convert_to_secure (vcpu->arch.gmap, +kvm_s390_get_prefix (vcpu)); +WARN_ON_ONCE (rc && rc != -EEXIST ); + rc = uv_convert_to_secure(vcpu->arch.gmap,Quoted 8 lines
arch/s390/kvm/kvm-s390.c static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) · line 3656
Quoted 3 lines
arch/s390/kvm/kvm-s390.c +3
-0+ kvm_s390_get_prefix(vcpu) + PAGE_SIZE); +WARN_ON_ONCE (rc && rc != -EEXIST ); + rc =0 ;So, what happens if we have an error other than
-EEXIST(which presumably means that we already protected it) here? The page is not protected, and further accesses will get an error? (Another question:
what can actually go wrong here?)
Quoted 2 lines
If KVM or QEMU write into a lowcore, we will fail the integrity check on import and this cpu will never run again.
From the guest's POV, is that similar to a cpu going into xstop?
Quoted 10 lines
In retrospect a
warn_on_oncemight be the wrong error handling here.Quoted 6 lines
Quoted 5 lines
arch/s390/kvm/kvm-s390.c +1
-0+ }if (rc) {kvm_make_request (KVM_REQ_MMU_RELOAD , vcpu);return rc;
Janosch Frank
maintainerRe: [RFC 23/37] KVM: s390: protvirt: Make sure prefix is always protected
Quoted 1 line · Cornelia Huck
On Tue, 19 Nov 2019 09:11:11 +0100
[...]
Quoted 10 lines
Quoted 8 lines
Quoted 5 lines
So, what happens if we have an error other than
-EEXIST(which presumably means that we already protected it) here? The page is not protected, and further accesses will get an error? (Another question:
what can actually go wrong here?)If KVM or QEMU write into a lowcore, we will fail the integrity check on import and this cpu will never run again.
From the guest's POV, is that similar to a cpu going into xstop?
Not really, I'm wondering what happens, if we try to send a restart to such a cpu.
Quoted 12 lines
Quoted 11 lines
In retrospect a
warn_on_oncemight be the wrong error handling here.Quoted 6 lines
Quoted 5 lines
arch/s390/kvm/kvm-s390.c +1
-0+ }if (rc) {kvm_make_request (KVM_REQ_MMU_RELOAD , vcpu);return rc;
Re: [RFC 23/37] KVM: s390: protvirt: Make sure prefix is always protected
In reply to Janosch FrankQuoted 12 lines · Janosch Frank
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/kvm-s390.c | 9 +++++++++ 1 file changed, 9 insertions(+)arch/s390/kvm/kvm-s390.c +0
-0@@ -3646,6 +3646,15 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) rc = gmap_mprotect_notify(vcpu->arch.gmap, kvm_s390_get_prefix(vcpu),
arch/s390/kvm/kvm-s390.c static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) · line 3657
Quoted 10 lines
arch/s390/kvm/kvm-s390.c +9
-0PAGE_SIZE * 2, PROT_WRITE); +if (!rc &&kvm_s390_pv_is_protected (vcpu->kvm)) { + rc =uv_convert_to_secure (vcpu->arch.gmap, +kvm_s390_get_prefix (vcpu)); +WARN_ON_ONCE (rc && rc != -EEXIST ); + rc =uv_convert_to_secure (vcpu->arch.gmap, +kvm_s390_get_prefix (vcpu) +PAGE_SIZE ); +WARN_ON_ONCE (rc && rc != -EEXIST ); + rc =0 ; + }
... what if userspace reads the prefix pages just after these calls? validity? :/
Quoted 4 lines
arch/s390/kvm/kvm-s390.c +0
-0if (rc) {kvm_make_request (KVM_REQ_MMU_RELOAD , vcpu);return rc;
Signature · 3 lines
Thanks, David / dhildenb
Janosch Frank
maintainerRe: [RFC 23/37] KVM: s390: protvirt: Make sure prefix is always protected
Quoted 18 lines · David Hildenbrand
Quoted 17 lines · Janosch Frank
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/kvm-s390.c | 9 +++++++++ 1 file changed, 9 insertions(+)arch/s390/kvm/kvm-s390.c +4
-0@@ -3646,6 +3646,15 @@ static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) rc =gmap_mprotect_notify (vcpu->arch.gmap,kvm_s390_get_prefix (vcpu),PAGE_SIZE *2 ,PROT_WRITE ); +if (!rc &&kvm_s390_pv_is_protected (vcpu->kvm)) { + rc =uv_convert_to_secure (vcpu->arch.gmap, +kvm_s390_get_prefix (vcpu)); +WARN_ON_ONCE (rc && rc != -EEXIST );
Quoted 8 lines
arch/s390/kvm/kvm-s390.c static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu) · line 3657
Quoted 5 lines
arch/s390/kvm/kvm-s390.c +5
-0+ rc =uv_convert_to_secure (vcpu->arch.gmap, +kvm_s390_get_prefix (vcpu) +PAGE_SIZE ); +WARN_ON_ONCE (rc && rc != -EEXIST ); + rc =0 ; + }... what if userspace reads the prefix pages just after these calls? validity? :/
Currently yes, we're working with firmware to fix this.
Quoted 6 lines
Quoted 5 lines
arch/s390/kvm/kvm-s390.c +0
-0if (rc) {kvm_make_request (KVM_REQ_MMU_RELOAD , vcpu);return rc;
Janosch Frank
maintainerRFC 24/37 KVM: s390: protvirt: Write sthyi data to instruction data area
In reply to Janosch FrankSTHYI data has to go through the bounce buffer.
--- arch/s390/kvm/intercept.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
arch/s390/kvm/intercept.c +10 -5
@@ -391,7 +391,7 @@ int handle_sthyi(struct kvm_vcpu *vcpu) goto out; } -if (addr & ~PAGE_MASK ) +if (!kvm_s390_pv_is_protected (vcpu->kvm) && (addr & ~PAGE_MASK ))return kvm_s390_inject_program_int (vcpu,PGM_SPECIFICATION ); sctns = (void *)get_zeroed_page (GFP_KERNEL );
@@ -402,10 +402,15 @@ int handle_sthyi(struct kvm_vcpu *vcpu) out:if (!cc) { - r =write_guest (vcpu, addr, reg2, sctns,PAGE_SIZE ); -if (r) { -free_page ((unsigned long )sctns); -return kvm_s390_inject_prog_cond (vcpu, r); +if (kvm_s390_pv_is_protected (vcpu->kvm)) { +memcpy ((void *)vcpu->arch.sie_block->sidad, sctns, +PAGE_SIZE ); + }else { + r =write_guest (vcpu, addr, reg2, sctns,PAGE_SIZE ); +if (r) { +free_page ((unsigned long )sctns); +return kvm_s390_inject_prog_cond (vcpu, r); + } } }
Signature · 1 line
2.20.1
Re: [RFC 24/37] KVM: s390: protvirt: Write sthyi data to instruction data area
Quoted 21 lines · Janosch Frank
STHYI data has to go through the bounce buffer.
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/intercept.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)arch/s390/kvm/intercept.c +1
-1@@ -391,7 +391,7 @@ int handle_sthyi(struct kvm_vcpu *vcpu) goto out; } -if (addr & ~PAGE_MASK ) +if (!kvm_s390_pv_is_protected (vcpu->kvm) && (addr & ~PAGE_MASK ))return kvm_s390_inject_program_int (vcpu,PGM_SPECIFICATION ); sctns = (void *)get_zeroed_page (GFP_KERNEL );
arch/s390/kvm/intercept.c int handle_sthyi(struct kvm_vcpu *vcpu) · line 406
Quoted 9 lines
arch/s390/kvm/intercept.c +2
-4@@ -402,10 +402,15 @@ int handle_sthyi(struct kvm_vcpu *vcpu) out:if (!cc) { - r =write_guest (vcpu, addr, reg2, sctns,PAGE_SIZE ); -if (r) { -free_page ((unsigned long )sctns); -return kvm_s390_inject_prog_cond (vcpu, r); +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + memcpy((void *)vcpu->arch.sie_block->sidad, sctns,
sidad & PAGE_MASK, just to be sure?
arch/s390/kvm/intercept.c int handle_sthyi(struct kvm_vcpu *vcpu) · line 416
Quoted 11 lines
arch/s390/kvm/intercept.c +7
-0+ PAGE_SIZE); + }else { + r =write_guest (vcpu, addr, reg2, sctns,PAGE_SIZE ); +if (r) { +free_page ((unsigned long )sctns); +return kvm_s390_inject_prog_cond (vcpu, r); + } } }
With "& PAGE_MASK":
Janosch Frank
maintainerRe: [RFC 24/37] KVM: s390: protvirt: Write sthyi data to instruction data area
Quoted 23 lines · Thomas Huth
Quoted 22 lines · Janosch Frank
STHYI data has to go through the bounce buffer.
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/intercept.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)arch/s390/kvm/intercept.c +1
-1@@ -391,7 +391,7 @@ int handle_sthyi(struct kvm_vcpu *vcpu) goto out; } -if (addr & ~PAGE_MASK ) +if (!kvm_s390_pv_is_protected (vcpu->kvm) && (addr & ~PAGE_MASK ))return kvm_s390_inject_program_int (vcpu,PGM_SPECIFICATION ); sctns = (void *)get_zeroed_page (GFP_KERNEL );@@ -402,10 +402,15 @@ int handle_sthyi(struct kvm_vcpu *vcpu)
Quoted 10 lines
arch/s390/kvm/intercept.c int handle_sthyi(struct kvm_vcpu *vcpu) · line 406
Quoted 8 lines
arch/s390/kvm/intercept.c +2
-4out:if (!cc) { - r =write_guest (vcpu, addr, reg2, sctns,PAGE_SIZE ); -if (r) { -free_page ((unsigned long )sctns); -return kvm_s390_inject_prog_cond (vcpu, r); +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + memcpy((void *)vcpu->arch.sie_block->sidad, sctns,sidad &
PAGE_MASK, just to be sure?
How about a macro or just saving the pointer in an arch struct?
Quoted 17 lines
arch/s390/kvm/intercept.c int handle_sthyi(struct kvm_vcpu *vcpu) · line 416
Quoted 12 lines
arch/s390/kvm/intercept.c +7
-0+ PAGE_SIZE); + }else { + r =write_guest (vcpu, addr, reg2, sctns,PAGE_SIZE ); +if (r) { +free_page ((unsigned long )sctns); +return kvm_s390_inject_prog_cond (vcpu, r); + } } }With "&
PAGE_MASK":Reviewed-by Thomas Huth <[email protected]>
Re: [RFC 24/37] KVM: s390: protvirt: Write sthyi data to instruction data area
Quoted 35 lines · Janosch Frank
Quoted 23 lines · Thomas Huth
Quoted 22 lines · Janosch Frank
STHYI data has to go through the bounce buffer.
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/intercept.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)arch/s390/kvm/intercept.c +1
-1@@ -391,7 +391,7 @@ int handle_sthyi(struct kvm_vcpu *vcpu) goto out; } -if (addr & ~PAGE_MASK ) +if (!kvm_s390_pv_is_protected (vcpu->kvm) && (addr & ~PAGE_MASK ))return kvm_s390_inject_program_int (vcpu,PGM_SPECIFICATION ); sctns = (void *)get_zeroed_page (GFP_KERNEL );@@ -402,10 +402,15 @@ int handle_sthyi(struct kvm_vcpu *vcpu)Quoted 10 lines
arch/s390/kvm/intercept.c int handle_sthyi(struct kvm_vcpu *vcpu) · line 406
Quoted 8 lines
arch/s390/kvm/intercept.c +2
-4out:if (!cc) { - r =write_guest (vcpu, addr, reg2, sctns,PAGE_SIZE ); -if (r) { -free_page ((unsigned long )sctns); -return kvm_s390_inject_prog_cond (vcpu, r); +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + memcpy((void *)vcpu->arch.sie_block->sidad, sctns,sidad &
PAGE_MASK, just to be sure?
Quoted 1 line
How about a macro or just saving the pointer in an arch struct?
Sounds fine, too. I think I'd personally slightly prefer a macro.
Thomas
Janosch Frank
maintainerPATCH 1/1 SIDAD macro fixup
Additionally I would need to use it in the other patches...
--- arch/s390/include/asm/kvm_host.h | 4 ++++ arch/s390/kvm/kvm-s390.c | 4 ++-- arch/s390/kvm/pv.c | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-)
arch/s390/include/asm/kvm_host.h +4 -0
@@ -122,6 +122,10 @@ struct mcck_volatile_info {__u32 reserved; }; +#define SIDAD_SIZE_MASK 0xff +#define sidad_origin (sie_block) \ + (sie_block->sidad & PAGE_MASK) +#define CPUSTAT_STOPPED 0x80000000#define CPUSTAT_WAIT 0x10000000#define CPUSTAT_ECALL_PEND 0x08000000
arch/s390/kvm/kvm-s390.c +2 -2
@@ -4436,7 +4436,7 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,* block which has its own size limit */ if (kvm_s390_pv_is_protected (vcpu->kvm) && - mop->size > ((vcpu->arch.sie_block->sidad &0x0f ) +1 ) *PAGE_SIZE ) + mop->size > ((vcpu->arch.sie_block->sidad &SIDAD_SIZE_MASK ) +1 ) *PAGE_SIZE )return -E2BIG ;if (!(mop->flags &KVM_S390_MEMOP_F_CHECK_ONLY )) {
@@ -4460,7 +4460,7 @@ static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu, }if (kvm_s390_pv_is_protected (vcpu->kvm)) { r =0 ; -if (copy_to_user (uaddr, (void *)vcpu->arch.sie_block->sidad + +if (copy_to_user (uaddr, (void *)sidad_origin (vcpu->arch.sie_block) + (mop->gaddr & ~PAGE_MASK ), mop->size)) r = -EFAULT ;
arch/s390/kvm/pv.c +1 -1
@@ -119,7 +119,7 @@ int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu)free_pages (vcpu->arch.pv.stor_base,get_order (uv_info.guest_cpu_stor_len)); -free_page (vcpu->arch.sie_block->sidad); +free_page (sidad_origin (vcpu->arch.sie_block));/* Clear cpu and vm handle */ memset (&vcpu->arch.sie_block->reserved10,0 ,sizeof (vcpu->arch.sie_block->reserved10));
Signature · 1 line
2.20.1
Janosch Frank
maintainerRFC 25/37 KVM: s390: protvirt: STSI handling
In reply to Janosch FrankSave response to sidad and disable address checking for protected guests.
--- arch/s390/kvm/priv.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
arch/s390/kvm/priv.c +7 -2
@@ -872,7 +872,7 @@ static int handle_stsi(struct kvm_vcpu *vcpu) operand2 =kvm_s390_get_base_disp_s (vcpu, &ar); -if (operand2 &0xfff ) +if (!kvm_s390_pv_is_protected (vcpu->kvm) && (operand2 &0xfff ))return kvm_s390_inject_program_int (vcpu,PGM_SPECIFICATION );switch (fc) {
@@ -893,8 +893,13 @@ static int handle_stsi(struct kvm_vcpu *vcpu)handle_stsi_3_2_2 (vcpu, (void *) mem);break ; } +if (kvm_s390_pv_is_protected (vcpu->kvm)) { +memcpy ((void *)vcpu->arch.sie_block->sidad, (void *)mem, +PAGE_SIZE ); + rc =0 ; + }else + rc =write_guest (vcpu, operand2, ar, (void *)mem,PAGE_SIZE ); - rc =write_guest (vcpu, operand2, ar, (void *)mem,PAGE_SIZE );if (rc) { rc =kvm_s390_inject_prog_cond (vcpu, rc); goto out;
Signature · 1 line
2.20.1
Re: [RFC 25/37] KVM: s390: protvirt: STSI handling
Quoted 13 lines · Janosch Frank
Save response to sidad and disable address checking for protected guests.
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/priv.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)arch/s390/kvm/priv.c +0
-0
arch/s390/kvm/priv.c static int handle_stsi(struct kvm_vcpu *vcpu) · line 876
Quoted 6 lines
arch/s390/kvm/priv.c +1
-1@@ -872,7 +872,7 @@ static int handle_stsi(struct kvm_vcpu *vcpu) operand2 =kvm_s390_get_base_disp_s (vcpu, &ar); -if (operand2 &0xfff ) +if (!kvm_s390_pv_is_protected (vcpu->kvm) && (operand2 &0xfff ))return kvm_s390_inject_program_int (vcpu,PGM_SPECIFICATION );
I'd prefer if you could put the calculation of operand2 also under the !pv if-statement:
if (!kvm_s390_pv_is_protected(vcpu->kvm)) {
operand2 = kvm_s390_get_base_disp_s(vcpu, &ar);
if (operand2 & 0xfff)
return kvm_s390_inject_program_int(vcpu,
PGM_SPECIFICATION);
}... that makes it more obvious that operand2 is only valid in the !pv case and you should get automatic compiler warnings if you use it otherwise.
arch/s390/kvm/priv.c static int handle_stsi(struct kvm_vcpu *vcpu) · line 901
Quoted 11 lines
arch/s390/kvm/priv.c +6
-0switch (fc) {@@ -893,8 +893,13 @@ static int handle_stsi(struct kvm_vcpu *vcpu)handle_stsi_3_2_2 (vcpu, (void *) mem);break ; } +if (kvm_s390_pv_is_protected (vcpu->kvm)) { +memcpy ((void *)vcpu->arch.sie_block->sidad, (void *)mem, +PAGE_SIZE ); + rc =0 ; + }else + rc =write_guest (vcpu, operand2, ar, (void *)mem,PAGE_SIZE );
Please also use braces for the else-branch (according to Documentation/process/coding-style.rst).
Thomas
Janosch Frank
maintainerRFC 26/37 KVM: s390: protvirt: Only sync fmt4 registers
In reply to Janosch FrankA lot of the registers are controlled by the Ultravisor and never visible to KVM. Also some registers are overlayed, like gbea is with sidad, which might leak data to userspace.
Hence we sync a minimal set of registers for both SIE formats and then check and sync format 2 registers if necessary.
Also we disable set/get one reg for the same reason. It's an old interface anyway.
--- arch/s390/kvm/kvm-s390.c | 138 +++++++++++++++++++++++---------------- 1 file changed, 82 insertions(+), 56 deletions(-)
arch/s390/kvm/kvm-s390.c +82 -56
@@ -2997,7 +2997,8 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu)/* make sure the new fpc will be lazily loaded */ save_fpu_regs (); current->thread.fpu.fpc =0 ; - vcpu->arch.sie_block->gbea =1 ; +if (!kvm_s390_pv_is_protected (vcpu->kvm)) + vcpu->arch.sie_block->gbea =1 ; vcpu->arch.sie_block->pp =0 ; vcpu->arch.sie_block->fpf &= ~FPF_BPBC ; vcpu->arch.pfault_token =KVM_S390_PFAULT_TOKEN_INVALID ;
@@ -3367,6 +3368,10 @@ static int kvm_arch_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, (u64 __user *)reg->addr);break ;case KVM_REG_S390_GBEA : +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + r =0 ; +break ; + } r =put_user (vcpu->arch.sie_block->gbea, (u64 __user *)reg->addr);break ;
@@ -3420,6 +3425,10 @@ static int kvm_arch_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, (u64 __user *)reg->addr);break ;case KVM_REG_S390_GBEA : +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + r =0 ; +break ; + } r =get_user (vcpu->arch.sie_block->gbea, (u64 __user *)reg->addr);break ;
@@ -4023,28 +4032,19 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)return rc; } -static void sync_regs (struct kvm_vcpu *vcpu,struct kvm_run *kvm_run) +static void sync_regs_fmt2 (struct kvm_vcpu *vcpu,struct kvm_run *kvm_run) {struct runtime_instr_cb *riccb;struct gs_cb *gscb; riccb = (struct runtime_instr_cb *) &kvm_run->s.regs.riccb; gscb = (struct gs_cb *) &kvm_run->s.regs.gscb; - vcpu->arch.sie_block->gpsw.mask = kvm_run->psw_mask; - vcpu->arch.sie_block->gpsw.addr = kvm_run->psw_addr; -if (kvm_run->kvm_dirty_regs &KVM_SYNC_PREFIX ) -kvm_s390_set_prefix (vcpu, kvm_run->s.regs.prefix); -if (kvm_run->kvm_dirty_regs &KVM_SYNC_CRS ) { -memcpy (&vcpu->arch.sie_block->gcr, &kvm_run->s.regs.crs,128 ); -/* some control register changes require a tlb flush */ -kvm_make_request (KVM_REQ_TLB_FLUSH , vcpu); - } -if (kvm_run->kvm_dirty_regs &KVM_SYNC_ARCH0 ) { -kvm_s390_set_cpu_timer (vcpu, kvm_run->s.regs.cputm); - vcpu->arch.sie_block->ckc = kvm_run->s.regs.ckc; - vcpu->arch.sie_block->todpr = kvm_run->s.regs.todpr; - vcpu->arch.sie_block->pp = kvm_run->s.regs.pp; - vcpu->arch.sie_block->gbea = kvm_run->s.regs.gbea; +if (kvm_run->kvm_dirty_regs &KVM_SYNC_ARCH0 ) + vcpu->arch.sie_block->gbea = kvm_run->s.regs.gbea; +if ((kvm_run->kvm_dirty_regs &KVM_SYNC_BPBC ) && +test_kvm_facility (vcpu->kvm,82 )) { + vcpu->arch.sie_block->fpf &= ~FPF_BPBC ; + vcpu->arch.sie_block->fpf |= kvm_run->s.regs.bpbc ?FPF_BPBC :0 ; }if (kvm_run->kvm_dirty_regs &KVM_SYNC_PFAULT ) { vcpu->arch.pfault_token = kvm_run->s.regs.pft;
@@ -4077,25 +4077,6 @@ static void sync_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) vcpu->arch.sie_block->ecd |=ECD_HOSTREGMGMT ; vcpu->arch.gs_enabled =1 ; } -if ((kvm_run->kvm_dirty_regs &KVM_SYNC_BPBC ) && -test_kvm_facility (vcpu->kvm,82 )) { - vcpu->arch.sie_block->fpf &= ~FPF_BPBC ; - vcpu->arch.sie_block->fpf |= kvm_run->s.regs.bpbc ?FPF_BPBC :0 ; - } -save_access_regs (vcpu->arch.host_acrs); -restore_access_regs (vcpu->run->s.regs.acrs); -/* save host (userspace) fprs/vrs */ -save_fpu_regs (); - vcpu->arch.host_fpregs.fpc = current->thread.fpu.fpc; - vcpu->arch.host_fpregs.regs = current->thread.fpu.regs; -if (MACHINE_HAS_VX ) - current->thread.fpu.regs = vcpu->run->s.regs.vrs; -else - current->thread.fpu.regs = vcpu->run->s.regs.fprs; - current->thread.fpu.fpc = vcpu->run->s.regs.fpc; -if (test_fp_ctl (current->thread.fpu.fpc)) -/* User space provided an invalid FPC, let's clear it */ - current->thread.fpu.fpc =0 ;if (MACHINE_HAS_GS ) {preempt_disable ();__ctl_set_bit (2 ,4 );
@@ -4111,33 +4092,50 @@ static void sync_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)preempt_enable (); }/* SIE will load etoken directly from SDNX and therefore kvm_run */ +} +static void sync_regs (struct kvm_vcpu *vcpu,struct kvm_run *kvm_run) +{ + vcpu->arch.sie_block->gpsw.mask = kvm_run->psw_mask; + vcpu->arch.sie_block->gpsw.addr = kvm_run->psw_addr; +if (kvm_run->kvm_dirty_regs &KVM_SYNC_PREFIX ) +kvm_s390_set_prefix (vcpu, kvm_run->s.regs.prefix); +if (kvm_run->kvm_dirty_regs &KVM_SYNC_CRS ) { +memcpy (&vcpu->arch.sie_block->gcr, &kvm_run->s.regs.crs,128 ); +/* some control register changes require a tlb flush */ +kvm_make_request (KVM_REQ_TLB_FLUSH , vcpu); + } +if (kvm_run->kvm_dirty_regs &KVM_SYNC_ARCH0 ) { +kvm_s390_set_cpu_timer (vcpu, kvm_run->s.regs.cputm); + vcpu->arch.sie_block->ckc = kvm_run->s.regs.ckc; + vcpu->arch.sie_block->todpr = kvm_run->s.regs.todpr; + vcpu->arch.sie_block->pp = kvm_run->s.regs.pp; + } +save_access_regs (vcpu->arch.host_acrs); +restore_access_regs (vcpu->run->s.regs.acrs); +/* save host (userspace) fprs/vrs */ +save_fpu_regs (); + vcpu->arch.host_fpregs.fpc = current->thread.fpu.fpc; + vcpu->arch.host_fpregs.regs = current->thread.fpu.regs; +if (MACHINE_HAS_VX ) + current->thread.fpu.regs = vcpu->run->s.regs.vrs; +else + current->thread.fpu.regs = vcpu->run->s.regs.fprs; + current->thread.fpu.fpc = vcpu->run->s.regs.fpc; +if (test_fp_ctl (current->thread.fpu.fpc)) +/* User space provided an invalid FPC, let's clear it */ + current->thread.fpu.fpc =0 ; + +/* Sync fmt2 only data */ +if (likely (!kvm_s390_pv_is_protected (vcpu->kvm))) +sync_regs_fmt2 (vcpu, kvm_run); kvm_run->kvm_dirty_regs =0 ; } -static void store_regs (struct kvm_vcpu *vcpu,struct kvm_run *kvm_run) +static void store_regs_fmt2 (struct kvm_vcpu *vcpu,struct kvm_run *kvm_run) { - kvm_run->psw_mask = vcpu->arch.sie_block->gpsw.mask; - kvm_run->psw_addr = vcpu->arch.sie_block->gpsw.addr; - kvm_run->s.regs.prefix =kvm_s390_get_prefix (vcpu); -memcpy (&kvm_run->s.regs.crs, &vcpu->arch.sie_block->gcr,128 ); - kvm_run->s.regs.cputm =kvm_s390_get_cpu_timer (vcpu); - kvm_run->s.regs.ckc = vcpu->arch.sie_block->ckc; - kvm_run->s.regs.todpr = vcpu->arch.sie_block->todpr; - kvm_run->s.regs.pp = vcpu->arch.sie_block->pp; kvm_run->s.regs.gbea = vcpu->arch.sie_block->gbea; - kvm_run->s.regs.pft = vcpu->arch.pfault_token; - kvm_run->s.regs.pfs = vcpu->arch.pfault_select; - kvm_run->s.regs.pfc = vcpu->arch.pfault_compare; kvm_run->s.regs.bpbc = (vcpu->arch.sie_block->fpf &FPF_BPBC ) ==FPF_BPBC ; -save_access_regs (vcpu->run->s.regs.acrs); -restore_access_regs (vcpu->arch.host_acrs); -/* Save guest register state */ -save_fpu_regs (); - vcpu->run->s.regs.fpc = current->thread.fpu.fpc; -/* Restore will be done lazily at return */ - current->thread.fpu.fpc = vcpu->arch.host_fpregs.fpc; - current->thread.fpu.regs = vcpu->arch.host_fpregs.regs;if (MACHINE_HAS_GS ) {__ctl_set_bit (2 ,4 );if (vcpu->arch.gs_enabled)
@@ -4153,6 +4151,31 @@ static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)/* SIE will save etoken directly into SDNX and therefore kvm_run */ } +static void store_regs (struct kvm_vcpu *vcpu,struct kvm_run *kvm_run) +{ + kvm_run->psw_mask = vcpu->arch.sie_block->gpsw.mask; + kvm_run->psw_addr = vcpu->arch.sie_block->gpsw.addr; + kvm_run->s.regs.prefix =kvm_s390_get_prefix (vcpu); +memcpy (&kvm_run->s.regs.crs, &vcpu->arch.sie_block->gcr,128 ); + kvm_run->s.regs.cputm =kvm_s390_get_cpu_timer (vcpu); + kvm_run->s.regs.ckc = vcpu->arch.sie_block->ckc; + kvm_run->s.regs.todpr = vcpu->arch.sie_block->todpr; + kvm_run->s.regs.pp = vcpu->arch.sie_block->pp; + kvm_run->s.regs.pft = vcpu->arch.pfault_token; + kvm_run->s.regs.pfs = vcpu->arch.pfault_select; + kvm_run->s.regs.pfc = vcpu->arch.pfault_compare; +save_access_regs (vcpu->run->s.regs.acrs); +restore_access_regs (vcpu->arch.host_acrs); +/* Save guest register state */ +save_fpu_regs (); + vcpu->run->s.regs.fpc = current->thread.fpu.fpc; +/* Restore will be done lazily at return */ + current->thread.fpu.fpc = vcpu->arch.host_fpregs.fpc; + current->thread.fpu.regs = vcpu->arch.host_fpregs.regs; +if (likely (!kvm_s390_pv_is_protected (vcpu->kvm))) +store_regs_fmt2 (vcpu, kvm_run); +} +int kvm_arch_vcpu_ioctl_run (struct kvm_vcpu *vcpu,struct kvm_run *kvm_run) {int rc;
@@ -4585,6 +4608,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,case KVM_SET_ONE_REG :case KVM_GET_ONE_REG : {struct kvm_one_reg reg; + r = -EINVAL ; +if (kvm_s390_pv_is_protected (vcpu->kvm)) +break ; r = -EFAULT ;if (copy_from_user (®, argp,sizeof (reg)))break ;
Signature · 1 line
2.20.1
Re: [RFC 26/37] KVM: s390: protvirt: Only sync fmt4 registers
Quoted 41 lines · Janosch Frank
A lot of the registers are controlled by the Ultravisor and never visible to KVM. Also some registers are overlayed, like gbea is with sidad, which might leak data to userspace.
Hence we sync a minimal set of registers for both SIE formats and then check and sync format 2 registers if necessary.
Also we disable set/get one reg for the same reason. It's an old interface anyway.
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/kvm-s390.c | 138 +++++++++++++++++++++++---------------- 1 file changed, 82 insertions(+), 56 deletions(-)arch/s390/kvm/kvm-s390.c +6
-1@@ -2997,7 +2997,8 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu)/* make sure the new fpc will be lazily loaded */ save_fpu_regs (); current->thread.fpu.fpc =0 ; - vcpu->arch.sie_block->gbea =1 ; +if (!kvm_s390_pv_is_protected (vcpu->kvm)) + vcpu->arch.sie_block->gbea =1 ; vcpu->arch.sie_block->pp =0 ; vcpu->arch.sie_block->fpf &= ~FPF_BPBC ; vcpu->arch.pfault_token =KVM_S390_PFAULT_TOKEN_INVALID ;@@ -3367,6 +3368,10 @@ static int kvm_arch_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, (u64 __user *)reg->addr);break ;case KVM_REG_S390_GBEA : +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + r =0 ; +break ; + } r =put_user (vcpu->arch.sie_block->gbea, (u64 __user *)reg->addr);break ;
arch/s390/kvm/kvm-s390.c static int kvm_arch_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, · line 3431
Quoted 7 lines
arch/s390/kvm/kvm-s390.c +4
-0@@ -3420,6 +3425,10 @@ static int kvm_arch_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, (u64 __user *)reg->addr);break ;case KVM_REG_S390_GBEA : +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + r =0 ; +break ; + }
Wouldn't it be better to return EINVAL in this case? ... the callers definitely do not get what they expected here...
Thomas
Janosch Frank
maintainerRe: [RFC 26/37] KVM: s390: protvirt: Only sync fmt4 registers
Quoted 53 lines · Thomas Huth
Quoted 41 lines · Janosch Frank
A lot of the registers are controlled by the Ultravisor and never visible to KVM. Also some registers are overlayed, like gbea is with sidad, which might leak data to userspace.
Hence we sync a minimal set of registers for both SIE formats and then check and sync format 2 registers if necessary.
Also we disable set/get one reg for the same reason. It's an old interface anyway.
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/kvm-s390.c | 138 +++++++++++++++++++++++---------------- 1 file changed, 82 insertions(+), 56 deletions(-)arch/s390/kvm/kvm-s390.c +6
-1@@ -2997,7 +2997,8 @@ static void kvm_s390_vcpu_initial_reset(struct kvm_vcpu *vcpu)/* make sure the new fpc will be lazily loaded */ save_fpu_regs (); current->thread.fpu.fpc =0 ; - vcpu->arch.sie_block->gbea =1 ; +if (!kvm_s390_pv_is_protected (vcpu->kvm)) + vcpu->arch.sie_block->gbea =1 ; vcpu->arch.sie_block->pp =0 ; vcpu->arch.sie_block->fpf &= ~FPF_BPBC ; vcpu->arch.pfault_token =KVM_S390_PFAULT_TOKEN_INVALID ;@@ -3367,6 +3368,10 @@ static int kvm_arch_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, (u64 __user *)reg->addr);break ;case KVM_REG_S390_GBEA : +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + r =0 ; +break ; + } r =put_user (vcpu->arch.sie_block->gbea, (u64 __user *)reg->addr);break ;arch/s390/kvm/kvm-s390.c static int kvm_arch_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, · line 3431
Quoted 7 lines
arch/s390/kvm/kvm-s390.c +4
-0@@ -3420,6 +3425,10 @@ static int kvm_arch_vcpu_ioctl_set_one_reg(struct kvm_vcpu *vcpu, (u64 __user *)reg->addr);break ;case KVM_REG_S390_GBEA : +if (kvm_s390_pv_is_protected (vcpu->kvm)) { + r =0 ; +break ; + }Wouldn't it be better to return EINVAL in this case? ... the callers definitely do not get what they expected here...
Quoted 2 lines
Thomas
Hrm, new QEMUs will use cpu run anyway and hence I guess it would make sense as old QEMUs hopefully won't have pv support.
Janosch Frank
maintainerRFC 27/37 KVM: s390: protvirt: SIGP handling
In reply to Janosch Frank--- arch/s390/kvm/intercept.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
arch/s390/kvm/intercept.c +2 -1
@@ -72,7 +72,8 @@ static int handle_stop(struct kvm_vcpu *vcpu)if (!stop_pending)return 0 ; -if (flags &KVM_S390_STOP_FLAG_STORE_STATUS ) { +if (flags &KVM_S390_STOP_FLAG_STORE_STATUS && + !kvm_s390_pv_is_protected (vcpu->kvm)) { rc =kvm_s390_vcpu_store_status (vcpu,KVM_S390_STORE_STATUS_NOADDR );if (rc)
Signature · 1 line
2.20.1
Re: [RFC 27/37] KVM: s390: protvirt: SIGP handling
Quoted 1 line · Janosch Frank
Signed-off-by Janosch Frank <[email protected]>
Can you add why this is necessary and how handle_stop() is intended to work in prot mode?
How is SIGP handled in general in prot mode? (which intercepts are handled by QEMU)
Would it be valid for user space to inject a STOP interrupt with "flags & KVM_S390_STOP_FLAG_STORE_STATUS" - I think not (legacy QEMU only)
I think we should rather disallow injecting such stop interrupts (KVM_S390_STOP_FLAG_STORE_STATUS) in prot mode in the first place. Also, we should disallow prot virt without user_sigp.
Quoted 19 lines
--- arch/s390/kvm/intercept.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)arch/s390/kvm/intercept.c +2
-1@@ -72,7 +72,8 @@ static int handle_stop(struct kvm_vcpu *vcpu)if (!stop_pending)return 0 ; -if (flags &KVM_S390_STOP_FLAG_STORE_STATUS ) { +if (flags &KVM_S390_STOP_FLAG_STORE_STATUS && + !kvm_s390_pv_is_protected (vcpu->kvm)) { rc =kvm_s390_vcpu_store_status (vcpu,KVM_S390_STORE_STATUS_NOADDR );if (rc)
Signature · 3 lines
Thanks, David / dhildenb
Re: [RFC 27/37] KVM: s390: protvirt: SIGP handling
In reply to Janosch FrankQuoted 10 lines · Janosch Frank
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/intercept.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)arch/s390/kvm/intercept.c +0
-0
arch/s390/kvm/intercept.c static int handle_stop(struct kvm_vcpu *vcpu) · line 79
Quoted 9 lines
arch/s390/kvm/intercept.c +2
-1@@ -72,7 +72,8 @@ static int handle_stop(struct kvm_vcpu *vcpu)if (!stop_pending)return 0 ; -if (flags &KVM_S390_STOP_FLAG_STORE_STATUS ) { +if (flags &KVM_S390_STOP_FLAG_STORE_STATUS && + !kvm_s390_pv_is_protected (vcpu->kvm)) { rc =kvm_s390_vcpu_store_status (vcpu,KVM_S390_STORE_STATUS_NOADDR );if (rc)
Can this still happen at all that we get here with KVM_S390_STOP_FLAG_STORE_STATUS in the protected case? I'd rather expect that SIGP is completely handled by the UV already, so userspace should have no need to inject a SIGP_STOP anymore? Or did I get that wrong?
Anyway, I guess it can not hurt to add this check anyway, so:
Janosch Frank
maintainerRFC 28/37 KVM: s390: protvirt: Add program exception injection
In reply to Janosch FrankOnly two program exceptions can be injected for a protected guest:
specification and operand
Both have a code in offset 248 of the state description, as the lowcore is not accessible by KVM for such guests.
--- arch/s390/kvm/interrupt.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
arch/s390/kvm/interrupt.c +18 -0
@@ -826,6 +826,21 @@ static int __must_check __deliver_external_call(struct kvm_vcpu *vcpu)return rc ? -EFAULT :0 ; } +static int __deliver_prog_pv (struct kvm_vcpu *vcpu,u16 code) +{ +switch (code) { +case PGM_SPECIFICATION : + vcpu->arch.sie_block->iictl =IICTL_CODE_SPECIFICATION ; +break ; +case PGM_OPERAND : + vcpu->arch.sie_block->iictl =IICTL_CODE_OPERAND ; +break ; +default : +return -EINVAL ; + } +return 0 ; +} +static int __must_check __deliver_prog (struct kvm_vcpu *vcpu) {struct kvm_s390_local_interrupt *li = &vcpu->arch.local_int;
@@ -846,6 +861,9 @@ static int __must_check __deliver_prog(struct kvm_vcpu *vcpu)trace_kvm_s390_deliver_interrupt (vcpu->vcpu_id,KVM_S390_PROGRAM_INT , pgm_info.code,0 ); +if (kvm_s390_pv_is_protected (vcpu->kvm)) +return __deliver_prog_pv (vcpu, pgm_info.code & ~PGM_PER ); +switch (pgm_info.code & ~PGM_PER ) {case PGM_AFX_TRANSLATION :case PGM_ASX_TRANSLATION :
Signature · 1 line
2.20.1
Janosch Frank
maintainerRFC 29/37 KVM: s390: protvirt: Sync pv state
In reply to Janosch FrankIndicate via register sync if the VM is in secure mode.
--- arch/s390/include/uapi/asm/kvm.h | 5 ++++- arch/s390/kvm/kvm-s390.c | 7 ++++++- 2 files changed, 10 insertions(+), 2 deletions(-)
arch/s390/include/uapi/asm/kvm.h +4 -1
@@ -231,11 +231,13 @@ struct kvm_guest_debug_arch {#define KVM_SYNC_GSCB (1UL << 9)#define KVM_SYNC_BPBC (1UL << 10)#define KVM_SYNC_ETOKEN (1UL << 11) +#define KVM_SYNC_PV (1UL << 12)#define KVM_SYNC_S390_VALID_FIELDS \ (KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \ KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \ - KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN) + KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN | \ + KVM_SYNC_PV)/* length and alignment of the sdnx as a power of two */ #define SDNXC 8
@@ -261,6 +263,7 @@ struct kvm_sync_regs {__u8 reserved[512 ];/* for future vector expansion */ __u32 fpc;/* valid on KVM_SYNC_VRS or KVM_SYNC_FPRS */ __u8 bpbc : 1; /* bp mode */ + __u8 pv : 1; /* pv mode */ __u8 reserved2 : 7;__u8 padding1[51 ];/* riccb needs to be 64byte aligned */ __u8 riccb[64 ];/* runtime instrumentation controls block */
arch/s390/kvm/kvm-s390.c +6 -1
@@ -2856,6 +2856,8 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu) vcpu->run->kvm_valid_regs |=KVM_SYNC_GSCB ;if (test_kvm_facility (vcpu->kvm,156 )) vcpu->run->kvm_valid_regs |=KVM_SYNC_ETOKEN ; +if (test_kvm_facility (vcpu->kvm,161 )) + vcpu->run->kvm_valid_regs |=KVM_SYNC_PV ;/* fprs can be synchronized via vrs, even if the guest has no vx. With * MACHINE_HAS_VX, (load|store)_fpu_regs() will work with vrs format. */
@@ -4136,6 +4138,7 @@ static void store_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) { kvm_run->s.regs.gbea = vcpu->arch.sie_block->gbea; kvm_run->s.regs.bpbc = (vcpu->arch.sie_block->fpf &FPF_BPBC ) ==FPF_BPBC ; + kvm_run->s.regs.pv =0 ;if (MACHINE_HAS_GS ) {__ctl_set_bit (2 ,4 );if (vcpu->arch.gs_enabled)
@@ -4172,8 +4175,10 @@ static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)/* Restore will be done lazily at return */ current->thread.fpu.fpc = vcpu->arch.host_fpregs.fpc; current->thread.fpu.regs = vcpu->arch.host_fpregs.regs; -if (likely (!kvm_s390_pv_is_protected (vcpu->kvm))) +if (likely (!kvm_s390_pv_handle_cpu (vcpu)))store_regs_fmt2 (vcpu, kvm_run); +else + kvm_run->s.regs.pv =1 ; }int kvm_arch_vcpu_ioctl_run (struct kvm_vcpu *vcpu,struct kvm_run *kvm_run)
Signature · 1 line
2.20.1
Re: [RFC 29/37] KVM: s390: protvirt: Sync pv state
Quoted 28 lines · Janosch Frank
Indicate via register sync if the VM is in secure mode.
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/uapi/asm/kvm.h | 5 ++++- arch/s390/kvm/kvm-s390.c | 7 ++++++- 2 files changed, 10 insertions(+), 2 deletions(-)arch/s390/include/uapi/asm/kvm.h +3
-1@@ -231,11 +231,13 @@ struct kvm_guest_debug_arch {#define KVM_SYNC_GSCB (1UL << 9)#define KVM_SYNC_BPBC (1UL << 10)#define KVM_SYNC_ETOKEN (1UL << 11) +#define KVM_SYNC_PV (1UL << 12)#define KVM_SYNC_S390_VALID_FIELDS \ (KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \ KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \ - KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN) + KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN | \ + KVM_SYNC_PV)/* length and alignment of the sdnx as a power of two */ #define SDNXC 8
arch/s390/include/uapi/asm/kvm.h struct kvm_sync_regs { · line 267
Quoted 5 lines
arch/s390/include/uapi/asm/kvm.h +1
-0@@ -261,6 +263,7 @@ struct kvm_sync_regs {__u8 reserved[512 ];/* for future vector expansion */ __u32 fpc;/* valid on KVM_SYNC_VRS or KVM_SYNC_FPRS */ __u8 bpbc : 1; /* bp mode */ + __u8 pv : 1; /* pv mode */ __u8 reserved2 : 7;
Don't you want to decrease the reserved2 bits to 6 ? ...
arch/s390/include/uapi/asm/kvm.h struct kvm_sync_regs { · line 268
Quoted 1 line
arch/s390/include/uapi/asm/kvm.h +0
-0__u8 padding1[51 ];/* riccb needs to be 64byte aligned */
... otherwise you might mess up the alignment here!
Quoted 23 lines
arch/s390/include/uapi/asm/kvm.h +0
-0__u8 riccb[64 ];/* runtime instrumentation controls block */ arch/s390/kvm/kvm-s390.c +3
-0@@ -2856,6 +2856,8 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu) vcpu->run->kvm_valid_regs |=KVM_SYNC_GSCB ;if (test_kvm_facility (vcpu->kvm,156 )) vcpu->run->kvm_valid_regs |=KVM_SYNC_ETOKEN ; +if (test_kvm_facility (vcpu->kvm,161 )) + vcpu->run->kvm_valid_regs |=KVM_SYNC_PV ;/* fprs can be synchronized via vrs, even if the guest has no vx. With * MACHINE_HAS_VX, (load|store)_fpu_regs() will work with vrs format. */ @@ -4136,6 +4138,7 @@ static void store_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) { kvm_run->s.regs.gbea = vcpu->arch.sie_block->gbea; kvm_run->s.regs.bpbc = (vcpu->arch.sie_block->fpf &FPF_BPBC ) ==FPF_BPBC ; + kvm_run->s.regs.pv =0 ;if (MACHINE_HAS_GS ) {__ctl_set_bit (2 ,4 );if (vcpu->arch.gs_enabled)
arch/s390/kvm/kvm-s390.c static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) · line 4178
Quoted 5 lines
arch/s390/kvm/kvm-s390.c +1
-1@@ -4172,8 +4175,10 @@ static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)/* Restore will be done lazily at return */ current->thread.fpu.fpc = vcpu->arch.host_fpregs.fpc; current->thread.fpu.regs = vcpu->arch.host_fpregs.regs; -if (likely (!kvm_s390_pv_is_protected (vcpu->kvm))) +if (likely (!kvm_s390_pv_handle_cpu (vcpu)))
Why change the if-statement now? Should this maybe rather be squashed into the patch that introduced the if-statement?
arch/s390/kvm/kvm-s390.c static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) · line 4184
Quoted 7 lines
arch/s390/kvm/kvm-s390.c +2
-0store_regs_fmt2 (vcpu, kvm_run); + else + kvm_run->s.regs.pv = 1; }int kvm_arch_vcpu_ioctl_run (struct kvm_vcpu *vcpu,struct kvm_run *kvm_run)
Thomas
Janosch Frank
maintainerRe: [RFC 29/37] KVM: s390: protvirt: Sync pv state
Quoted 26 lines · Thomas Huth
Quoted 25 lines · Janosch Frank
Indicate via register sync if the VM is in secure mode.
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/uapi/asm/kvm.h | 5 ++++- arch/s390/kvm/kvm-s390.c | 7 ++++++- 2 files changed, 10 insertions(+), 2 deletions(-)arch/s390/include/uapi/asm/kvm.h +3
-1@@ -231,11 +231,13 @@ struct kvm_guest_debug_arch {#define KVM_SYNC_GSCB (1UL << 9)#define KVM_SYNC_BPBC (1UL << 10)#define KVM_SYNC_ETOKEN (1UL << 11) +#define KVM_SYNC_PV (1UL << 12)#define KVM_SYNC_S390_VALID_FIELDS \ (KVM_SYNC_PREFIX | KVM_SYNC_GPRS | KVM_SYNC_ACRS | KVM_SYNC_CRS | \ KVM_SYNC_ARCH0 | KVM_SYNC_PFAULT | KVM_SYNC_VRS | KVM_SYNC_RICCB | \ - KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN) + KVM_SYNC_FPRS | KVM_SYNC_GSCB | KVM_SYNC_BPBC | KVM_SYNC_ETOKEN | \ + KVM_SYNC_PV)
Quoted 10 lines
arch/s390/include/uapi/asm/kvm.h struct kvm_sync_regs { · line 267
Quoted 8 lines
arch/s390/include/uapi/asm/kvm.h +1
-0/* length and alignment of the sdnx as a power of two */ #define SDNXC 8@@ -261,6 +263,7 @@ struct kvm_sync_regs {__u8 reserved[512 ];/* for future vector expansion */ __u32 fpc;/* valid on KVM_SYNC_VRS or KVM_SYNC_FPRS */ __u8 bpbc : 1; /* bp mode */ + __u8 pv : 1; /* pv mode */ __u8 reserved2 : 7;Don't you want to decrease the reserved2 bits to 6 ? ...
Ups
Quoted 28 lines
arch/s390/include/uapi/asm/kvm.h struct kvm_sync_regs { · line 268
Quoted 2 lines
arch/s390/include/uapi/asm/kvm.h +0
-0__u8 padding1[51 ];/* riccb needs to be 64byte aligned */ ... otherwise you might mess up the alignment here!
Quoted 23 lines
arch/s390/include/uapi/asm/kvm.h +0
-0__u8 riccb[64 ];/* runtime instrumentation controls block */ arch/s390/kvm/kvm-s390.c +3
-0@@ -2856,6 +2856,8 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu) vcpu->run->kvm_valid_regs |=KVM_SYNC_GSCB ;if (test_kvm_facility (vcpu->kvm,156 )) vcpu->run->kvm_valid_regs |=KVM_SYNC_ETOKEN ; +if (test_kvm_facility (vcpu->kvm,161 )) + vcpu->run->kvm_valid_regs |=KVM_SYNC_PV ;/* fprs can be synchronized via vrs, even if the guest has no vx. With * MACHINE_HAS_VX, (load|store)_fpu_regs() will work with vrs format. */ @@ -4136,6 +4138,7 @@ static void store_regs_fmt2(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) { kvm_run->s.regs.gbea = vcpu->arch.sie_block->gbea; kvm_run->s.regs.bpbc = (vcpu->arch.sie_block->fpf &FPF_BPBC ) ==FPF_BPBC ; + kvm_run->s.regs.pv =0 ;if (MACHINE_HAS_GS ) {__ctl_set_bit (2 ,4 );if (vcpu->arch.gs_enabled)@@ -4172,8 +4175,10 @@ static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
Quoted 8 lines
arch/s390/kvm/kvm-s390.c static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) · line 4178
Quoted 5 lines
arch/s390/kvm/kvm-s390.c +1
-1/* Restore will be done lazily at return */ current->thread.fpu.fpc = vcpu->arch.host_fpregs.fpc; current->thread.fpu.regs = vcpu->arch.host_fpregs.regs; -if (likely (!kvm_s390_pv_is_protected (vcpu->kvm))) +if (likely (!kvm_s390_pv_handle_cpu (vcpu)))Why change the if-statement now? Should this maybe rather be squashed into the patch that introduced the if-statement?
That was part of a cleanup that should have been done in other patches.
Quoted 11 lines
arch/s390/kvm/kvm-s390.c static void store_regs(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run) · line 4184
Quoted 8 lines
arch/s390/kvm/kvm-s390.c +2
-0store_regs_fmt2 (vcpu, kvm_run); + else + kvm_run->s.regs.pv = 1; }int kvm_arch_vcpu_ioctl_run (struct kvm_vcpu *vcpu,struct kvm_run *kvm_run)Thomas
Janosch Frank
maintainerRFC 30/37 DOCUMENTATION: protvirt: Diag 308 IPL
In reply to Janosch FrankDescription of changes that are necessary to move a KVM VM into Protected Virtualization mode.
--- Documentation/virtual/kvm/s390-pv-boot.txt | 62 ++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 Documentation/virtual/kvm/s390-pv-boot.txt
Documentation/virtual/kvm/s390-pv-boot.txt new file +62 -0
@@ -0,0 +1,62 @@ +Boot/IPL of Protected VMs +======================== + +Summary: + +Protected VMs are encrypted while not running. On IPL a small +plaintext bootloader is started which provides information about the +encrypted components and necessary metadata to KVM to decrypt it. + +Based on this data, KVM will make the PV known to the Ultravisor and +instruct it to secure its memory, decrypt the components and verify +the data and address list hashes, to ensure integrity. Afterwards KVM +can run the PV via SIE which the UV will intercept and execute on +KVM's behalf. + +The switch into PV mode lets us load encrypted guest executables and +data via every available method (network, dasd, scsi, direct kernel, +...) without the need to change the boot process. + + +Diag308: + +This diagnose instruction is the basis vor VM IPL. The VM can set and +retrieve IPL information blocks, that specify the IPL method/devices +and request VM memory and subsystem resets, as well as IPLs. + +For PVs this concept has been continued with new subcodes: + +Subcode 8: Set an IPL Information Block of type 5. +Subcode 9: Store the saved block in guest memory +Subcode 10: Move into Protected Virtualization mode + +The new PV load-device-specific-parameters field specifies all data, +that is necessary to move into PV mode. + +* PV Header origin +* PV Header length +* List of Components composed of: + * AES-XTS Tweak prefix + * Origin + * Size + +The PV header contains the keys and hashes, which the UV will use to +decrypt and verify the PV, as well as control flags and a start PSW. + +The components are for instance an encrypted kernel, kernel cmd and +initrd. The components are decrypted by the UV. + +All non-decrypted data of the non-PV guest instance are zero on first +access of the PV. + + +When running in a protected mode some subcodes will result in +exceptions or return error codes. + +Subcodes 4 and 7 will result in specification exceptions. +When removing a secure VM, the UV will clear all memory, so we can't +have non-clearing IPL subcodes. + +Subcodes 8, 9, 10 will result in specification exceptions. +Re-IPL into a protected mode is only possible via a detour into non +protected mode.
Signature · 1 line
2.20.1
Re: [RFC 30/37] DOCUMENTATION: protvirt: Diag 308 IPL
Quoted 28 lines · On Thu, 24 Oct 2019 07:40:52 -0400 Janosch Frank <[email protected]> wrote:
Description of changes that are necessary to move a KVM VM into Protected Virtualization mode.
Signed-off-by Janosch Frank <[email protected]>--- Documentation/virtual/kvm/s390-pv-boot.txt | 62 ++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 Documentation/virtual/kvm/s390-pv-boot.txtDocumentation/virtual/kvm/s390-pv-boot.txt new file +13
-0@@ -0,0 +1,62 @@ +Boot/IPL of Protected VMs +======================== + +Summary: + +Protected VMs are encrypted while not running. On IPL a small +plaintext bootloader is started which provides information about the +encrypted components and necessary metadata to KVM to decrypt it. + +Based on this data, KVM will make the PV known to the Ultravisor and +instruct it to secure its memory, decrypt the components and verify +the data and address list hashes, to ensure integrity. Afterwards KVM +can run the PV via SIE which the UV will intercept and execute on
Documentation/virtual/kvm/s390-pv-boot.txt line 23
Quoted 10 lines
Documentation/virtual/kvm/s390-pv-boot.txt new file +10
-0+KVM's behalf. + +The switch into PV mode lets us load encrypted guest executables and +data via every available method (network, dasd, scsi, direct kernel, +...) without the need to change the boot process. + + +Diag308: + +This diagnose instruction is the basis vor VM IPL. The VM can set and
s/vor/for/
Quoted 29 lines
Documentation/virtual/kvm/s390-pv-boot.txt new file +29
-0+retrieve IPL information blocks, that specify the IPL method/devices +and request VM memory and subsystem resets, as well as IPLs. + +For PVs this concept has been continued with new subcodes: + +Subcode 8: Set an IPL Information Block of type 5. +Subcode 9: Store the saved block in guest memory +Subcode 10: Move into Protected Virtualization mode + +The new PV load-device-specific-parameters field specifies all data, +that is necessary to move into PV mode. + +* PV Header origin +* PV Header length +* List of Components composed of: + * AES-XTS Tweak prefix + * Origin + * Size + +The PV header contains the keys and hashes, which the UV will use to +decrypt and verify the PV, as well as control flags and a start PSW. + +The components are for instance an encrypted kernel, kernel cmd and +initrd. The components are decrypted by the UV. + +All non-decrypted data of the non-PV guest instance are zero on first +access of the PV. + +
Documentation/virtual/kvm/s390-pv-boot.txt line 62
Quoted 10 lines
Documentation/virtual/kvm/s390-pv-boot.txt new file +10
-0+When running in a protected mode some subcodes will result in +exceptions or return error codes. + +Subcodes 4 and 7 will result in specification exceptions. +When removing a secure VM, the UV will clear all memory, so we can't +have non-clearing IPL subcodes. + +Subcodes 8, 9, 10 will result in specification exceptions. +Re-IPL into a protected mode is only possible via a detour into non +protected mode.
So... what do we IPL from? Is there still a need for the bios?
(Sorry, I'm a bit confused here.)
Janosch Frank
maintainerRe: [RFC 30/37] DOCUMENTATION: protvirt: Diag 308 IPL
Quoted 86 lines · Cornelia Huck
Quoted 28 lines · On Thu, 24 Oct 2019 07:40:52 -0400 Janosch Frank <[email protected]> wrote:
Description of changes that are necessary to move a KVM VM into Protected Virtualization mode.
Signed-off-by Janosch Frank <[email protected]>--- Documentation/virtual/kvm/s390-pv-boot.txt | 62 ++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 Documentation/virtual/kvm/s390-pv-boot.txtDocumentation/virtual/kvm/s390-pv-boot.txt new file +13
-0@@ -0,0 +1,62 @@ +Boot/IPL of Protected VMs +======================== + +Summary: + +Protected VMs are encrypted while not running. On IPL a small +plaintext bootloader is started which provides information about the +encrypted components and necessary metadata to KVM to decrypt it. + +Based on this data, KVM will make the PV known to the Ultravisor and +instruct it to secure its memory, decrypt the components and verify +the data and address list hashes, to ensure integrity. Afterwards KVM +can run the PV via SIE which the UV will intercept and execute onDocumentation/virtual/kvm/s390-pv-boot.txt line 23
Quoted 10 lines
Documentation/virtual/kvm/s390-pv-boot.txt new file +10
-0+KVM's behalf. + +The switch into PV mode lets us load encrypted guest executables and +data via every available method (network, dasd, scsi, direct kernel, +...) without the need to change the boot process. + + +Diag308: + +This diagnose instruction is the basis vor VM IPL. The VM can set ands/vor/for/
Quoted 29 lines
Documentation/virtual/kvm/s390-pv-boot.txt new file +29
-0+retrieve IPL information blocks, that specify the IPL method/devices +and request VM memory and subsystem resets, as well as IPLs. + +For PVs this concept has been continued with new subcodes: + +Subcode 8: Set an IPL Information Block of type 5. +Subcode 9: Store the saved block in guest memory +Subcode 10: Move into Protected Virtualization mode + +The new PV load-device-specific-parameters field specifies all data, +that is necessary to move into PV mode. + +* PV Header origin +* PV Header length +* List of Components composed of: + * AES-XTS Tweak prefix + * Origin + * Size + +The PV header contains the keys and hashes, which the UV will use to +decrypt and verify the PV, as well as control flags and a start PSW. + +The components are for instance an encrypted kernel, kernel cmd and +initrd. The components are decrypted by the UV. + +All non-decrypted data of the non-PV guest instance are zero on first +access of the PV. + +Documentation/virtual/kvm/s390-pv-boot.txt line 62
Quoted 10 lines
Documentation/virtual/kvm/s390-pv-boot.txt new file +10
-0+When running in a protected mode some subcodes will result in +exceptions or return error codes. + +Subcodes 4 and 7 will result in specification exceptions. +When removing a secure VM, the UV will clear all memory, so we can't +have non-clearing IPL subcodes. + +Subcodes 8, 9, 10 will result in specification exceptions. +Re-IPL into a protected mode is only possible via a detour into non +protected mode.So... what do we IPL from? Is there still a need for the bios?
Quoted 2 lines
(Sorry, I'm a bit confused here.)
We load a blob via the bios (all methods are supported) and that blob moves itself into protected mode. I.e. it has a small unprotected stub, the rest is an encrypted kernel.
Re: [RFC 30/37] DOCUMENTATION: protvirt: Diag 308 IPL
Quoted 13 lines · On Wed, 6 Nov 2019 18:05:22 +0100 Janosch Frank <[email protected]> wrote:
Quoted 11 lines · Cornelia Huck
Quoted 8 lines · On Thu, 24 Oct 2019 07:40:52 -0400 Janosch Frank <[email protected]> wrote:
Description of changes that are necessary to move a KVM VM into Protected Virtualization mode.
Signed-off-by Janosch Frank <[email protected]>--- Documentation/virtual/kvm/s390-pv-boot.txt | 62 ++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 Documentation/virtual/kvm/s390-pv-boot.txt
Quoted 9 lines
Quoted 4 lines
So... what do we IPL from? Is there still a need for the bios?
(Sorry, I'm a bit confused here.)
We load a blob via the bios (all methods are supported) and that blob moves itself into protected mode. I.e. it has a small unprotected stub, the rest is an encrypted kernel.
Ok. The magic is in the loaded kernel, and we don't need modifications to the bios?
Janosch Frank
maintainerRe: [RFC 30/37] DOCUMENTATION: protvirt: Diag 308 IPL
Quoted 21 lines · Cornelia Huck
Quoted 18 lines · On Wed, 6 Nov 2019 18:05:22 +0100 Janosch Frank <[email protected]> wrote:
Quoted 16 lines · Cornelia Huck
Quoted 8 lines · On Thu, 24 Oct 2019 07:40:52 -0400 Janosch Frank <[email protected]> wrote:
Description of changes that are necessary to move a KVM VM into Protected Virtualization mode.
Signed-off-by Janosch Frank <[email protected]>--- Documentation/virtual/kvm/s390-pv-boot.txt | 62 ++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 Documentation/virtual/kvm/s390-pv-boot.txtSo... what do we IPL from? Is there still a need for the bios?
(Sorry, I'm a bit confused here.)
Quoted 8 lines
Quoted 4 lines
We load a blob via the bios (all methods are supported) and that blob moves itself into protected mode. I.e. it has a small unprotected stub, the rest is an encrypted kernel.
Ok. The magic is in the loaded kernel, and we don't need modifications to the bios?
Yes.
The order is:
* We load a blob via the bios or direct kernel boot.
* That blob consists of a small stub, a header and an encrypted blob glued together
* The small stub does the diag 308 subcode 8 and 10.
* Subcode 8 basically passes the header that describes the encrypted blob to the Ultravisor (well rather registers it with qemu to pass on later)
* Subcode 10 tells QEMU to move the VM into protected mode
* A lot of APIs in KVM and the Ultravisor are called
* The protected VM starts
* A memory mover copies the now unencrypted, but protected kernel to its intended place and jumps into the entry function
* Linux boots and detects, that it is protected and needs to use bounce buffers
Re: [RFC 30/37] DOCUMENTATION: protvirt: Diag 308 IPL
Quoted 33 lines · On Wed, 6 Nov 2019 22:02:41 +0100 Janosch Frank <[email protected]> wrote:
Quoted 21 lines · Cornelia Huck
Quoted 18 lines · On Wed, 6 Nov 2019 18:05:22 +0100 Janosch Frank <[email protected]> wrote:
Quoted 16 lines · Cornelia Huck
Quoted 8 lines · On Thu, 24 Oct 2019 07:40:52 -0400 Janosch Frank <[email protected]> wrote:
Description of changes that are necessary to move a KVM VM into Protected Virtualization mode.
Signed-off-by Janosch Frank <[email protected]>--- Documentation/virtual/kvm/s390-pv-boot.txt | 62 ++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 Documentation/virtual/kvm/s390-pv-boot.txtSo... what do we IPL from? Is there still a need for the bios?
(Sorry, I'm a bit confused here.)
Quoted 8 lines
Quoted 4 lines
We load a blob via the bios (all methods are supported) and that blob moves itself into protected mode. I.e. it has a small unprotected stub, the rest is an encrypted kernel.
Ok. The magic is in the loaded kernel, and we don't need modifications to the bios?
Yes.
Quoted 15 lines
The order is:
* We load a blob via the bios or direct kernel boot.
* That blob consists of a small stub, a header and an encrypted blob glued together
* The small stub does the diag 308 subcode 8 and 10.
* Subcode 8 basically passes the header that describes the encrypted blob to the Ultravisor (well rather registers it with qemu to pass on later)
* Subcode 10 tells QEMU to move the VM into protected mode
* A lot of APIs in KVM and the Ultravisor are called
* The protected VM starts
* A memory mover copies the now unencrypted, but protected kernel to its intended place and jumps into the entry function
* Linux boots and detects, that it is protected and needs to use bounce buffers
Thanks, this explanation makes things much clearer.
Janosch Frank
maintainerRe: [RFC 30/37] DOCUMENTATION: protvirt: Diag 308 IPL
Quoted 52 lines · Cornelia Huck
Quoted 33 lines · On Wed, 6 Nov 2019 22:02:41 +0100 Janosch Frank <[email protected]> wrote:
Quoted 21 lines · Cornelia Huck
Quoted 18 lines · On Wed, 6 Nov 2019 18:05:22 +0100 Janosch Frank <[email protected]> wrote:
Quoted 16 lines · Cornelia Huck
Quoted 8 lines · On Thu, 24 Oct 2019 07:40:52 -0400 Janosch Frank <[email protected]> wrote:
Description of changes that are necessary to move a KVM VM into Protected Virtualization mode.
Signed-off-by Janosch Frank <[email protected]>--- Documentation/virtual/kvm/s390-pv-boot.txt | 62 ++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 Documentation/virtual/kvm/s390-pv-boot.txtSo... what do we IPL from? Is there still a need for the bios?
(Sorry, I'm a bit confused here.)
Quoted 8 lines
Quoted 4 lines
We load a blob via the bios (all methods are supported) and that blob moves itself into protected mode. I.e. it has a small unprotected stub, the rest is an encrypted kernel.
Ok. The magic is in the loaded kernel, and we don't need modifications to the bios?
Yes.
Quoted 15 lines
The order is:
* We load a blob via the bios or direct kernel boot.
* That blob consists of a small stub, a header and an encrypted blob glued together
* The small stub does the diag 308 subcode 8 and 10.
* Subcode 8 basically passes the header that describes the encrypted blob to the Ultravisor (well rather registers it with qemu to pass on later)
* Subcode 10 tells QEMU to move the VM into protected mode
* A lot of APIs in KVM and the Ultravisor are called
* The protected VM starts
* A memory mover copies the now unencrypted, but protected kernel to its intended place and jumps into the entry function
* Linux boots and detects, that it is protected and needs to use bounce buffers
Quoted 1 line
Thanks, this explanation makes things much clearer.
NP
We seem to assume that all of this is easily understandable, but we are obviously biased :-)
I'll try to improve Documentation by adding Pierre to the discussion, as he wasn't involved in the project yet.
Janosch Frank
maintainerRFC 31/37 KVM: s390: protvirt: Add diag 308 subcode 8 - 10 handling
In reply to Janosch FrankIf the host initialized the Ultravisor, we can set stfle bit 161 (protected virtual IPL enhancements facility), which indicates, that the IPL subcodes 8, 9 and are valid. These subcodes are used by a normal guest to set/retrieve a IPIB of type 5 and transition into protected mode.
Once in protected mode, the VM will loose the facility bit, as each boot into protected mode has to go through non-protected. There is no secure re-ipl with subcode 10 without a previous subcode 3.
In protected mode, there is no subcode 4 available, as the VM has no more access to its memory from non-protected mode. I.e. each IPL clears.
--- arch/s390/kvm/diag.c | 6 ++++++ arch/s390/kvm/kvm-s390.c | 5 +++++ 2 files changed, 11 insertions(+)
arch/s390/kvm/diag.c +6 -0
@@ -197,6 +197,12 @@ static int __diag_ipl_functions(struct kvm_vcpu *vcpu)case 4 : vcpu->run->s390_reset_flags =0 ;break ; +case 8 : +case 9 : +case 10 : +if (!test_kvm_facility (vcpu->kvm,161 )) +return kvm_s390_inject_program_int (vcpu,PGM_SPECIFICATION ); +/* fall through */ default :return -EOPNOTSUPP ; }
arch/s390/kvm/kvm-s390.c +5 -0
@@ -2590,6 +2590,11 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)if (css_general_characteristics.aiv &&test_facility (65 ))set_kvm_facility (kvm->arch.model.fac_mask,65 ); +if (is_prot_virt_host ()) { +set_kvm_facility (kvm->arch.model.fac_mask,161 ); +set_kvm_facility (kvm->arch.model.fac_list,161 ); + } + kvm->arch.model.cpuid =kvm_s390_get_initial_cpuid (); kvm->arch.model.ibc = sclp.ibc &0x0fff ;
Signature · 1 line
2.20.1
Re: [RFC 31/37] KVM: s390: protvirt: Add diag 308 subcode 8 - 10 handling
Quoted 7 lines · Janosch Frank
If the host initialized the Ultravisor, we can set stfle bit 161 (protected virtual IPL enhancements facility), which indicates, that the IPL subcodes 8, 9 and are valid. These subcodes are used by a normal guest to set/retrieve a IPIB of type 5 and transition into protected mode.
Once in protected mode, the VM will loose the facility bit, as each
So should the bit be cleared in the host code again? ... I don't see this happening in this patch?
Thomas
Quoted 47 lines
boot into protected mode has to go through non-protected. There is no secure re-ipl with subcode 10 without a previous subcode 3.
In protected mode, there is no subcode 4 available, as the VM has no more access to its memory from non-protected mode. I.e. each IPL clears.
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/diag.c | 6 ++++++ arch/s390/kvm/kvm-s390.c | 5 +++++ 2 files changed, 11 insertions(+)arch/s390/kvm/diag.c +6
-0@@ -197,6 +197,12 @@ static int __diag_ipl_functions(struct kvm_vcpu *vcpu)case 4 : vcpu->run->s390_reset_flags =0 ;break ; +case 8 : +case 9 : +case 10 : +if (!test_kvm_facility (vcpu->kvm,161 )) +return kvm_s390_inject_program_int (vcpu,PGM_SPECIFICATION ); +/* fall through */ default :return -EOPNOTSUPP ; }arch/s390/kvm/kvm-s390.c +5
-0@@ -2590,6 +2590,11 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)if (css_general_characteristics.aiv &&test_facility (65 ))set_kvm_facility (kvm->arch.model.fac_mask,65 ); +if (is_prot_virt_host ()) { +set_kvm_facility (kvm->arch.model.fac_mask,161 ); +set_kvm_facility (kvm->arch.model.fac_list,161 ); + } + kvm->arch.model.cpuid =kvm_s390_get_initial_cpuid (); kvm->arch.model.ibc = sclp.ibc &0x0fff ;
Janosch Frank
maintainerRe: [RFC 31/37] KVM: s390: protvirt: Add diag 308 subcode 8 - 10 handling
Quoted 7 lines · Thomas Huth
Quoted 6 lines · Janosch Frank
If the host initialized the Ultravisor, we can set stfle bit 161 (protected virtual IPL enhancements facility), which indicates, that the IPL subcodes 8, 9 and are valid. These subcodes are used by a normal guest to set/retrieve a IPIB of type 5 and transition into protected mode.
Quoted 6 lines
Quoted 1 line
Once in protected mode, the VM will loose the facility bit, as each
So should the bit be cleared in the host code again? ... I don't see this happening in this patch?
Thomas
No, KVM doesn't report stfle facilities in protected mode and we would need to add it again in normal mode so just clearing it would be pointless. In protected mode 8-10 do not intercept, so there's nothing we need to do.
Quoted 50 lines
Quoted 49 lines
boot into protected mode has to go through non-protected. There is no secure re-ipl with subcode 10 without a previous subcode 3.
In protected mode, there is no subcode 4 available, as the VM has no more access to its memory from non-protected mode. I.e. each IPL clears.
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/diag.c | 6 ++++++ arch/s390/kvm/kvm-s390.c | 5 +++++ 2 files changed, 11 insertions(+)arch/s390/kvm/diag.c +6
-0@@ -197,6 +197,12 @@ static int __diag_ipl_functions(struct kvm_vcpu *vcpu)case 4 : vcpu->run->s390_reset_flags =0 ;break ; +case 8 : +case 9 : +case 10 : +if (!test_kvm_facility (vcpu->kvm,161 )) +return kvm_s390_inject_program_int (vcpu,PGM_SPECIFICATION ); +/* fall through */ default :return -EOPNOTSUPP ; }arch/s390/kvm/kvm-s390.c +5
-0@@ -2590,6 +2590,11 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)if (css_general_characteristics.aiv &&test_facility (65 ))set_kvm_facility (kvm->arch.model.fac_mask,65 ); +if (is_prot_virt_host ()) { +set_kvm_facility (kvm->arch.model.fac_mask,161 ); +set_kvm_facility (kvm->arch.model.fac_list,161 ); + } + kvm->arch.model.cpuid =kvm_s390_get_initial_cpuid (); kvm->arch.model.ibc = sclp.ibc &0x0fff ;
Re: [RFC 31/37] KVM: s390: protvirt: Add diag 308 subcode 8 - 10 handling
Quoted 13 lines · Janosch Frank
Quoted 12 lines · Thomas Huth
Quoted 7 lines · Janosch Frank
If the host initialized the Ultravisor, we can set stfle bit 161 (protected virtual IPL enhancements facility), which indicates, that the IPL subcodes 8, 9 and are valid. These subcodes are used by a normal guest to set/retrieve a IPIB of type 5 and transition into protected mode.
Once in protected mode, the VM will loose the facility bit, as each
So should the bit be cleared in the host code again? ... I don't see this happening in this patch?
Quoted 6 lines
Quoted 1 line
Thomas
No, KVM doesn't report stfle facilities in protected mode and we would need to add it again in normal mode so just clearing it would be pointless. In protected mode 8-10 do not intercept, so there's nothing we need to do.
Ah, ok, that's what I've missed. Maybe replace "the VM will loose the facility bit" with "the ultravisor will conceal the facility bit" ?
Thomas
Janosch Frank
maintainerRe: [RFC 31/37] KVM: s390: protvirt: Add diag 308 subcode 8 - 10 handling
Quoted 21 lines · Thomas Huth
Quoted 13 lines · Janosch Frank
Quoted 12 lines · Thomas Huth
Quoted 7 lines · Janosch Frank
If the host initialized the Ultravisor, we can set stfle bit 161 (protected virtual IPL enhancements facility), which indicates, that the IPL subcodes 8, 9 and are valid. These subcodes are used by a normal guest to set/retrieve a IPIB of type 5 and transition into protected mode.
Once in protected mode, the VM will loose the facility bit, as each
So should the bit be cleared in the host code again? ... I don't see this happening in this patch?
Quoted 6 lines
Quoted 1 line
Thomas
No, KVM doesn't report stfle facilities in protected mode and we would need to add it again in normal mode so just clearing it would be pointless. In protected mode 8-10 do not intercept, so there's nothing we need to do.
Quoted 5 lines
Ah, ok, that's what I've missed. Maybe replace "the VM will loose the facility bit" with "the ultravisor will conceal the facility bit" ?
Thomas
Sure
Janosch Frank
maintainerRFC 32/37 KVM: s390: protvirt: UV calls diag308 0, 1
In reply to Janosch Frank--- arch/s390/include/asm/uv.h | 25 +++++++++++++++++++++++++ arch/s390/kvm/diag.c | 1 + arch/s390/kvm/kvm-s390.c | 20 ++++++++++++++++++++ arch/s390/kvm/kvm-s390.h | 2 ++ arch/s390/kvm/pv.c | 19 +++++++++++++++++++ include/uapi/linux/kvm.h | 2 ++ 6 files changed, 69 insertions(+)
arch/s390/include/asm/uv.h +25 -0
@@ -35,6 +35,12 @@#define UVC_CMD_SET_SEC_CONF_PARAMS 0x0300#define UVC_CMD_UNPACK_IMG 0x0301#define UVC_CMD_VERIFY_IMG 0x0302 +#define UVC_CMD_CPU_RESET 0x0310 +#define UVC_CMD_CPU_RESET_INITIAL 0x0311 +#define UVC_CMD_PERF_CONF_CLEAR_RESET 0x0320 +#define UVC_CMD_CPU_RESET_CLEAR 0x0321 +#define UVC_CMD_CPU_SET_STATE 0x0330 +#define UVC_CMD_SET_UNSHARED_ALL 0x0340#define UVC_CMD_SET_SHARED_ACCESS 0x1000#define UVC_CMD_REMOVE_SHARED_ACCESS 0x1001
@@ -53,6 +59,12 @@ enum uv_cmds_inst {BIT_UVC_CMD_SET_SEC_PARMS =11 ,BIT_UVC_CMD_UNPACK_IMG =13 ,BIT_UVC_CMD_VERIFY_IMG =14 , +BIT_UVC_CMD_CPU_RESET =15 , +BIT_UVC_CMD_CPU_RESET_INITIAL =16 , +BIT_UVC_CMD_CPU_SET_STATE =17 , +BIT_UVC_CMD_PREPARE_CLEAR_RESET =18 , +BIT_UVC_CMD_CPU_PERFORM_CLEAR_RESET =19 , + BIT_UVC_CMD_REMOVE_SHARED_ACCES = 20, }; struct uv_cb_header {
@@ -148,6 +160,19 @@ struct uv_cb_unp {u64 reserved28[3 ]; } __packed__aligned(8) ; +#define PV_CPU_STATE_OPR 1 +#define PV_CPU_STATE_STP 2 +#define PV_CPU_STATE_CHKSTP 3 + +struct uv_cb_cpu_set_state { +struct uv_cb_header header; +u64 reserved08[2 ]; +u64 cpu_handle; +u8 reserved20[7 ]; +u8 state; +u64 reserved28[5 ]; +}; +/* * A common UV call struct for the following calls: * Destroy cpu/config
arch/s390/kvm/diag.c +1 -0
@@ -13,6 +13,7 @@#include <asm/pgalloc.h> #include <asm/gmap.h> #include <asm/virtio-ccw.h> +#include <asm/uv.h> #include "kvm-s390.h" #include "trace.h" #include "trace-s390.h"
arch/s390/kvm/kvm-s390.c +20 -0
@@ -2256,6 +2256,26 @@ static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd) ret >> 16, ret & 0x0000ffff);break ; } +case KVM_PV_VM_PERF_CLEAR_RESET : { +u32 ret; + + r =uv_cmd_nodata (kvm_s390_pv_handle (kvm), +UVC_CMD_PERF_CONF_CLEAR_RESET , + &ret); +VM_EVENT (kvm,3 ,"PROTVIRT PERF CLEAR: rc %x rrc %x" , + ret >>16 , ret &0x0000ffff ); +break ; + } +case KVM_PV_VM_UNSHARE : { +u32 ret; + + r =uv_cmd_nodata (kvm_s390_pv_handle (kvm), +UVC_CMD_SET_UNSHARED_ALL , + &ret); +VM_EVENT (kvm,3 ,"PROTVIRT UNSHARE: %d rc %x rrc %x" , + r, ret >>16 , ret &0x0000ffff ); +break ; + }default :return -ENOTTY ; }
arch/s390/kvm/kvm-s390.h +2 -0
@@ -209,6 +209,7 @@ int kvm_s390_pv_set_sec_parms(struct kvm *kvm, void *hdr, u64 length);int kvm_s390_pv_unpack (struct kvm *kvm,unsigned long addr,unsigned long size,unsigned long tweak);int kvm_s390_pv_verify (struct kvm *kvm); +int kvm_s390_pv_set_cpu_state (struct kvm_vcpu *vcpu,u8 state);static inline bool kvm_s390_pv_is_protected (struct kvm *kvm) {
@@ -238,6 +239,7 @@ static inline int kvm_s390_pv_unpack(struct kvm *kvm, unsigned long addr, unsigned long size, unsigned long tweak) {return 0 ; }static inline int kvm_s390_pv_verify (struct kvm *kvm) {return 0 ; } +static inline int kvm_s390_pv_set_cpu_state (struct kvm_vcpu *vcpu,u8 state) {return 0 ; }static inline bool kvm_s390_pv_is_protected (struct kvm *kvm) {return 0 ; }static inline u64 kvm_s390_pv_handle (struct kvm *kvm) {return 0 ; }static inline u64 kvm_s390_pv_handle_cpu (struct kvm_vcpu *vcpu) {return 0 ; }
arch/s390/kvm/pv.c +19 -0
@@ -280,3 +280,22 @@ int kvm_s390_pv_unpack(struct kvm *kvm, unsigned long addr, unsigned long size, uvcb.header.rc, uvcb.header.rrc);return rc; } + +int kvm_s390_pv_set_cpu_state (struct kvm_vcpu *vcpu,u8 state) +{ +int rc; +struct uv_cb_cpu_set_state uvcb = { + .header.cmd =UVC_CMD_CPU_SET_STATE , + .header.len =sizeof (uvcb), + .cpu_handle =kvm_s390_pv_handle_cpu (vcpu), + .state = state, + }; + +if (!kvm_s390_pv_handle_cpu (vcpu)) +return -EINVAL ; + + rc =uv_call (0 , (u64)&uvcb); +if (rc) +return -EINVAL ; +return 0 ; +}
include/uapi/linux/kvm.h +2 -0
@@ -1479,6 +1479,8 @@ enum pv_cmd_id {KVM_PV_VM_SET_SEC_PARMS ,KVM_PV_VM_UNPACK ,KVM_PV_VM_VERIFY , +KVM_PV_VM_PERF_CLEAR_RESET , +KVM_PV_VM_UNSHARE ,KVM_PV_VCPU_CREATE , KVM_PV_VCPU_DESTROY, };
Signature · 1 line
2.20.1
Re: [RFC 32/37] KVM: s390: protvirt: UV calls diag308 0, 1
Quoted 9 lines · Janosch Frank
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/asm/uv.h | 25 +++++++++++++++++++++++++ arch/s390/kvm/diag.c | 1 + arch/s390/kvm/kvm-s390.c | 20 ++++++++++++++++++++ arch/s390/kvm/kvm-s390.h | 2 ++ arch/s390/kvm/pv.c | 19 +++++++++++++++++++ include/uapi/linux/kvm.h | 2 ++ 6 files changed, 69 insertions(+)
Add at least a short patch description what this patch is all about?
Thomas
Janosch Frank
maintainerRe: [RFC 32/37] KVM: s390: protvirt: UV calls diag308 0, 1
Quoted 11 lines · Thomas Huth
Quoted 9 lines · Janosch Frank
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/asm/uv.h | 25 +++++++++++++++++++++++++ arch/s390/kvm/diag.c | 1 + arch/s390/kvm/kvm-s390.c | 20 ++++++++++++++++++++ arch/s390/kvm/kvm-s390.h | 2 ++ arch/s390/kvm/pv.c | 19 +++++++++++++++++++ include/uapi/linux/kvm.h | 2 ++ 6 files changed, 69 insertions(+)
Quoted 4 lines
Add at least a short patch description what this patch is all about?
Thomas
I'm thinking about taking out the set cpu state changes and move it into a later patch.
How about:
diag 308 subcode 0 and 1 require KVM and Ultravisor interaction, since the cpus have to be set into multiple reset states.
* All cpus need to be stopped
* The unshare all UVC needs to be executed
* The perform reset UVC needs to be executed
* The cpus need to be reset via the set cpu state UVC
* The issuing cpu needs to set state 5 via set cpu state
Re: [RFC 32/37] KVM: s390: protvirt: UV calls diag308 0, 1
Quoted 25 lines · Janosch Frank
Quoted 11 lines · Thomas Huth
Quoted 9 lines · Janosch Frank
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/asm/uv.h | 25 +++++++++++++++++++++++++ arch/s390/kvm/diag.c | 1 + arch/s390/kvm/kvm-s390.c | 20 ++++++++++++++++++++ arch/s390/kvm/kvm-s390.h | 2 ++ arch/s390/kvm/pv.c | 19 +++++++++++++++++++ include/uapi/linux/kvm.h | 2 ++ 6 files changed, 69 insertions(+)Quoted 4 lines
Add at least a short patch description what this patch is all about?
Thomas
I'm thinking about taking out the set cpu state changes and move it into a later patch.
How about:
diag 308 subcode 0 and 1 require KVM and Ultravisor interaction, since the cpus have to be set into multiple reset states.
Quoted 5 lines
* All cpus need to be stopped
* The unshare all UVC needs to be executed
* The perform reset UVC needs to be executed
* The cpus need to be reset via the set cpu state UVC
* The issuing cpu needs to set state 5 via set cpu state
Could you put the UVC names into quotes? Like:
* The "unshare all" UVC needs to be executed
... I first had to read the sentence three times to really understand it.
Thomas
Janosch Frank
maintainerRe: [RFC 32/37] KVM: s390: protvirt: UV calls diag308 0, 1
Quoted 32 lines · Thomas Huth
Quoted 25 lines · Janosch Frank
Quoted 11 lines · Thomas Huth
Quoted 9 lines · Janosch Frank
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/asm/uv.h | 25 +++++++++++++++++++++++++ arch/s390/kvm/diag.c | 1 + arch/s390/kvm/kvm-s390.c | 20 ++++++++++++++++++++ arch/s390/kvm/kvm-s390.h | 2 ++ arch/s390/kvm/pv.c | 19 +++++++++++++++++++ include/uapi/linux/kvm.h | 2 ++ 6 files changed, 69 insertions(+)Quoted 4 lines
Add at least a short patch description what this patch is all about?
Thomas
I'm thinking about taking out the set cpu state changes and move it into a later patch.
How about:
diag 308 subcode 0 and 1 require KVM and Ultravisor interaction, since the cpus have to be set into multiple reset states.Quoted 5 lines
* All cpus need to be stopped
* The unshare all UVC needs to be executed
* The perform reset UVC needs to be executed
* The cpus need to be reset via the set cpu state UVC
* The issuing cpu needs to set state 5 via set cpu state
Quoted 8 lines
Could you put the UVC names into quotes? Like:
* The "unshare all" UVC needs to be executed
... I first had to read the sentence three times to really understand it.
Thomas
Sure, just did
Janosch Frank
maintainerRFC 33/37 KVM: s390: Introduce VCPU reset IOCTL
In reply to Janosch FrankWith PV we need to do things for all reset types, not only initial...
--- arch/s390/kvm/kvm-s390.c | 53 ++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/kvm.h | 6 +++++ 2 files changed, 59 insertions(+)
arch/s390/kvm/kvm-s390.c +53 -0
@@ -3472,6 +3472,53 @@ static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)return 0 ; } +static int kvm_arch_vcpu_ioctl_reset (struct kvm_vcpu *vcpu, +unsigned long type) +{ +int rc; +u32 ret; + +switch (type) { +case KVM_S390_VCPU_RESET_NORMAL : +/* +* Only very little is reset, userspace handles the +* non-protected case. +*/ + rc =0 ; +if (kvm_s390_pv_handle_cpu (vcpu)) { + rc =uv_cmd_nodata (kvm_s390_pv_handle_cpu (vcpu), +UVC_CMD_CPU_RESET , &ret); +VCPU_EVENT (vcpu,3 ,"PROTVIRT RESET NORMAL VCPU: cpu %d rc %x rrc %x" , + vcpu->vcpu_id, ret >>16 , ret &0x0000ffff ); + } +break ; +case KVM_S390_VCPU_RESET_INITIAL : + rc =kvm_arch_vcpu_ioctl_initial_reset (vcpu); +if (kvm_s390_pv_handle_cpu (vcpu)) { +uv_cmd_nodata (kvm_s390_pv_handle_cpu (vcpu), +UVC_CMD_CPU_RESET_INITIAL , + &ret); +VCPU_EVENT (vcpu,3 ,"PROTVIRT RESET INITIAL VCPU: cpu %d rc %x rrc %x" , + vcpu->vcpu_id, ret >>16 , ret &0x0000ffff ); + } +break ; +case KVM_S390_VCPU_RESET_CLEAR : + rc =kvm_arch_vcpu_ioctl_initial_reset (vcpu); +if (kvm_s390_pv_handle_cpu (vcpu)) { + rc =uv_cmd_nodata (kvm_s390_pv_handle_cpu (vcpu), +UVC_CMD_CPU_RESET_CLEAR , &ret); +VCPU_EVENT (vcpu,3 ,"PROTVIRT RESET CLEAR VCPU: cpu %d rc %x rrc %x" , + vcpu->vcpu_id, ret >>16 , ret &0x0000ffff ); + } +break ; +default : + rc = -EINVAL ; +break ; + } +return rc; +} + +int kvm_arch_vcpu_ioctl_set_regs (struct kvm_vcpu *vcpu,struct kvm_regs *regs) {vcpu_load (vcpu);
@@ -4633,8 +4680,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,break ; }case KVM_S390_INITIAL_RESET : + r = -EINVAL ; +if (kvm_s390_pv_is_protected (vcpu->kvm)) +break ; r =kvm_arch_vcpu_ioctl_initial_reset (vcpu);break ; +case KVM_S390_VCPU_RESET : + r =kvm_arch_vcpu_ioctl_reset (vcpu, arg); +break ;case KVM_SET_ONE_REG :case KVM_GET_ONE_REG : {struct kvm_one_reg reg;
include/uapi/linux/kvm.h +6 -0
@@ -1496,6 +1496,12 @@ struct kvm_pv_cmd {#define KVM_S390_PV_COMMAND _IOW(KVMIO, 0xc3, struct kvm_pv_cmd)#define KVM_S390_PV_COMMAND_VCPU _IOW(KVMIO, 0xc4, struct kvm_pv_cmd) +#define KVM_S390_VCPU_RESET_NORMAL 0 +#define KVM_S390_VCPU_RESET_INITIAL 1 +#define KVM_S390_VCPU_RESET_CLEAR 2 + +#define KVM_S390_VCPU_RESET _IO(KVMIO, 0xd0) +/* Secure Encrypted Virtualization command */ enum sev_cmd_id {/* Guest initialization commands */
Signature · 1 line
2.20.1
Re: [RFC 33/37] KVM: s390: Introduce VCPU reset IOCTL
Quoted 48 lines · Janosch Frank
With PV we need to do things for all reset types, not only initial...
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/kvm-s390.c | 53 ++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/kvm.h | 6 +++++ 2 files changed, 59 insertions(+)arch/s390/kvm/kvm-s390.c +32
-0@@ -3472,6 +3472,53 @@ static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)return 0 ; } +static int kvm_arch_vcpu_ioctl_reset (struct kvm_vcpu *vcpu, +unsigned long type) +{ +int rc; +u32 ret; + +switch (type) { +case KVM_S390_VCPU_RESET_NORMAL : +/* +* Only very little is reset, userspace handles the +* non-protected case. +*/ + rc =0 ; +if (kvm_s390_pv_handle_cpu (vcpu)) { + rc =uv_cmd_nodata (kvm_s390_pv_handle_cpu (vcpu), +UVC_CMD_CPU_RESET , &ret); +VCPU_EVENT (vcpu,3 ,"PROTVIRT RESET NORMAL VCPU: cpu %d rc %x rrc %x" , + vcpu->vcpu_id, ret >>16 , ret &0x0000ffff ); + } +break ; +case KVM_S390_VCPU_RESET_INITIAL : + rc =kvm_arch_vcpu_ioctl_initial_reset (vcpu); +if (kvm_s390_pv_handle_cpu (vcpu)) { +uv_cmd_nodata (kvm_s390_pv_handle_cpu (vcpu), +UVC_CMD_CPU_RESET_INITIAL , + &ret); +VCPU_EVENT (vcpu,3 ,"PROTVIRT RESET INITIAL VCPU: cpu %d rc %x rrc %x" , + vcpu->vcpu_id, ret >>16 , ret &0x0000ffff ); + } +break ; +case KVM_S390_VCPU_RESET_CLEAR : + rc =kvm_arch_vcpu_ioctl_initial_reset (vcpu);
arch/s390/kvm/kvm-s390.c static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu) · line 3516
Quoted 10 lines
arch/s390/kvm/kvm-s390.c +10
-0+if (kvm_s390_pv_handle_cpu (vcpu)) { + rc =uv_cmd_nodata (kvm_s390_pv_handle_cpu (vcpu), +UVC_CMD_CPU_RESET_CLEAR , &ret); +VCPU_EVENT (vcpu,3 ,"PROTVIRT RESET CLEAR VCPU: cpu %d rc %x rrc %x" , + vcpu->vcpu_id, ret >>16 , ret &0x0000ffff ); + } +break ; +default : + rc = -EINVAL ; +break ;
(nit: you could drop the "break;" here)
Quoted 9 lines
arch/s390/kvm/kvm-s390.c +5
-0+ } +return rc; +} + +int kvm_arch_vcpu_ioctl_set_regs (struct kvm_vcpu *vcpu,struct kvm_regs *regs) {vcpu_load (vcpu);
arch/s390/kvm/kvm-s390.c long kvm_arch_vcpu_ioctl(struct file *filp, · line 4685
Quoted 6 lines
arch/s390/kvm/kvm-s390.c +3
-0@@ -4633,8 +4680,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,break ; }case KVM_S390_INITIAL_RESET : + r = -EINVAL ; +if (kvm_s390_pv_is_protected (vcpu->kvm)) +break ;
Wouldn't it be nicer to call
kvm_arch_vcpu_ioctl_reset(vcpu, KVM_S390_VCPU_RESET_INITIAL)
in this case instead?
Quoted 13 lines
arch/s390/kvm/kvm-s390.c +3
-0r =kvm_arch_vcpu_ioctl_initial_reset (vcpu);break ; +case KVM_S390_VCPU_RESET : + r =kvm_arch_vcpu_ioctl_reset (vcpu, arg); +break ;case KVM_SET_ONE_REG :case KVM_GET_ONE_REG : {struct kvm_one_reg reg;include/uapi/linux/kvm.h +0
-0
include/uapi/linux/kvm.h struct kvm_pv_cmd { · line 1503
Quoted 8 lines
include/uapi/linux/kvm.h +5
-0@@ -1496,6 +1496,12 @@ struct kvm_pv_cmd {#define KVM_S390_PV_COMMAND _IOW(KVMIO, 0xc3, struct kvm_pv_cmd)#define KVM_S390_PV_COMMAND_VCPU _IOW(KVMIO, 0xc4, struct kvm_pv_cmd) +#define KVM_S390_VCPU_RESET_NORMAL 0 +#define KVM_S390_VCPU_RESET_INITIAL 1 +#define KVM_S390_VCPU_RESET_CLEAR 2 + +#define KVM_S390_VCPU_RESET _IO(KVMIO, 0xd0)
Why not 0xc5 ?
Thomas
Janosch Frank
maintainerRe: [RFC 33/37] KVM: s390: Introduce VCPU reset IOCTL
Quoted 80 lines · Thomas Huth
Quoted 48 lines · Janosch Frank
With PV we need to do things for all reset types, not only initial...
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/kvm-s390.c | 53 ++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/kvm.h | 6 +++++ 2 files changed, 59 insertions(+)arch/s390/kvm/kvm-s390.c +32
-0@@ -3472,6 +3472,53 @@ static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)return 0 ; } +static int kvm_arch_vcpu_ioctl_reset (struct kvm_vcpu *vcpu, +unsigned long type) +{ +int rc; +u32 ret; + +switch (type) { +case KVM_S390_VCPU_RESET_NORMAL : +/* +* Only very little is reset, userspace handles the +* non-protected case. +*/ + rc =0 ; +if (kvm_s390_pv_handle_cpu (vcpu)) { + rc =uv_cmd_nodata (kvm_s390_pv_handle_cpu (vcpu), +UVC_CMD_CPU_RESET , &ret); +VCPU_EVENT (vcpu,3 ,"PROTVIRT RESET NORMAL VCPU: cpu %d rc %x rrc %x" , + vcpu->vcpu_id, ret >>16 , ret &0x0000ffff ); + } +break ; +case KVM_S390_VCPU_RESET_INITIAL : + rc =kvm_arch_vcpu_ioctl_initial_reset (vcpu); +if (kvm_s390_pv_handle_cpu (vcpu)) { +uv_cmd_nodata (kvm_s390_pv_handle_cpu (vcpu), +UVC_CMD_CPU_RESET_INITIAL , + &ret); +VCPU_EVENT (vcpu,3 ,"PROTVIRT RESET INITIAL VCPU: cpu %d rc %x rrc %x" , + vcpu->vcpu_id, ret >>16 , ret &0x0000ffff ); + } +break ; +case KVM_S390_VCPU_RESET_CLEAR : + rc =kvm_arch_vcpu_ioctl_initial_reset (vcpu);arch/s390/kvm/kvm-s390.c static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu) · line 3516
Quoted 10 lines
arch/s390/kvm/kvm-s390.c +10
-0+if (kvm_s390_pv_handle_cpu (vcpu)) { + rc =uv_cmd_nodata (kvm_s390_pv_handle_cpu (vcpu), +UVC_CMD_CPU_RESET_CLEAR , &ret); +VCPU_EVENT (vcpu,3 ,"PROTVIRT RESET CLEAR VCPU: cpu %d rc %x rrc %x" , + vcpu->vcpu_id, ret >>16 , ret &0x0000ffff ); + } +break ; +default : + rc = -EINVAL ; +break ;(nit: you could drop the "break;" here)
Quoted 9 lines
arch/s390/kvm/kvm-s390.c +5
-0+ } +return rc; +} + +int kvm_arch_vcpu_ioctl_set_regs (struct kvm_vcpu *vcpu,struct kvm_regs *regs) {vcpu_load (vcpu);arch/s390/kvm/kvm-s390.c long kvm_arch_vcpu_ioctl(struct file *filp, · line 4685
Quoted 6 lines
arch/s390/kvm/kvm-s390.c +3
-0@@ -4633,8 +4680,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,break ; }case KVM_S390_INITIAL_RESET : + r = -EINVAL ; +if (kvm_s390_pv_is_protected (vcpu->kvm)) +break ;Wouldn't it be nicer to call
Quoted 3 lines
kvm_arch_vcpu_ioctl_reset(vcpu, KVM_S390_VCPU_RESET_INITIAL)in this case instead?
How about:
case KVM_S390_INITIAL_RESET:
arg = KVM_S390_VCPU_RESET_INITIAL;
case KVM_S390_VCPU_RESET:
r = kvm_arch_vcpu_ioctl_reset(vcpu, arg);
break;Quoted 17 lines
Quoted 17 lines
arch/s390/kvm/kvm-s390.c +3
-0r =kvm_arch_vcpu_ioctl_initial_reset (vcpu);break ; +case KVM_S390_VCPU_RESET : + r =kvm_arch_vcpu_ioctl_reset (vcpu, arg); +break ;case KVM_SET_ONE_REG :case KVM_GET_ONE_REG : {struct kvm_one_reg reg;include/uapi/linux/kvm.h +0
-0@@ -1496,6 +1496,12 @@ struct kvm_pv_cmd {#define KVM_S390_PV_COMMAND _IOW(KVMIO, 0xc3, struct kvm_pv_cmd)#define KVM_S390_PV_COMMAND_VCPU _IOW(KVMIO, 0xc4, struct kvm_pv_cmd)
Quoted 7 lines
include/uapi/linux/kvm.h struct kvm_pv_cmd { · line 1503
Quoted 5 lines
include/uapi/linux/kvm.h +5
-0+#define KVM_S390_VCPU_RESET_NORMAL 0 +#define KVM_S390_VCPU_RESET_INITIAL 1 +#define KVM_S390_VCPU_RESET_CLEAR 2 + +#define KVM_S390_VCPU_RESET _IO(KVMIO, 0xd0)Why not
0xc5?
Fixed
Quoted 3 lines
Thomas
Re: [RFC 33/37] KVM: s390: Introduce VCPU reset IOCTL
Quoted 83 lines · Janosch Frank
Quoted 82 lines · Thomas Huth
Quoted 48 lines · Janosch Frank
With PV we need to do things for all reset types, not only initial...
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/kvm-s390.c | 53 ++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/kvm.h | 6 +++++ 2 files changed, 59 insertions(+)arch/s390/kvm/kvm-s390.c +32
-0@@ -3472,6 +3472,53 @@ static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)return 0 ; } +static int kvm_arch_vcpu_ioctl_reset (struct kvm_vcpu *vcpu, +unsigned long type) +{ +int rc; +u32 ret; + +switch (type) { +case KVM_S390_VCPU_RESET_NORMAL : +/* +* Only very little is reset, userspace handles the +* non-protected case. +*/ + rc =0 ; +if (kvm_s390_pv_handle_cpu (vcpu)) { + rc =uv_cmd_nodata (kvm_s390_pv_handle_cpu (vcpu), +UVC_CMD_CPU_RESET , &ret); +VCPU_EVENT (vcpu,3 ,"PROTVIRT RESET NORMAL VCPU: cpu %d rc %x rrc %x" , + vcpu->vcpu_id, ret >>16 , ret &0x0000ffff ); + } +break ; +case KVM_S390_VCPU_RESET_INITIAL : + rc =kvm_arch_vcpu_ioctl_initial_reset (vcpu); +if (kvm_s390_pv_handle_cpu (vcpu)) { +uv_cmd_nodata (kvm_s390_pv_handle_cpu (vcpu), +UVC_CMD_CPU_RESET_INITIAL , + &ret); +VCPU_EVENT (vcpu,3 ,"PROTVIRT RESET INITIAL VCPU: cpu %d rc %x rrc %x" , + vcpu->vcpu_id, ret >>16 , ret &0x0000ffff ); + } +break ; +case KVM_S390_VCPU_RESET_CLEAR : + rc =kvm_arch_vcpu_ioctl_initial_reset (vcpu);arch/s390/kvm/kvm-s390.c static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu) · line 3516
Quoted 10 lines
arch/s390/kvm/kvm-s390.c +10
-0+if (kvm_s390_pv_handle_cpu (vcpu)) { + rc =uv_cmd_nodata (kvm_s390_pv_handle_cpu (vcpu), +UVC_CMD_CPU_RESET_CLEAR , &ret); +VCPU_EVENT (vcpu,3 ,"PROTVIRT RESET CLEAR VCPU: cpu %d rc %x rrc %x" , + vcpu->vcpu_id, ret >>16 , ret &0x0000ffff ); + } +break ; +default : + rc = -EINVAL ; +break ;(nit: you could drop the "break;" here)
Quoted 9 lines
arch/s390/kvm/kvm-s390.c +5
-0+ } +return rc; +} + +int kvm_arch_vcpu_ioctl_set_regs (struct kvm_vcpu *vcpu,struct kvm_regs *regs) {vcpu_load (vcpu);arch/s390/kvm/kvm-s390.c long kvm_arch_vcpu_ioctl(struct file *filp, · line 4685
Quoted 6 lines
arch/s390/kvm/kvm-s390.c +3
-0@@ -4633,8 +4680,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,break ; }case KVM_S390_INITIAL_RESET : + r = -EINVAL ; +if (kvm_s390_pv_is_protected (vcpu->kvm)) +break ;Wouldn't it be nicer to call
kvm_arch_vcpu_ioctl_reset(vcpu, KVM_S390_VCPU_RESET_INITIAL)
Quoted 8 lines
Quoted 1 line
in this case instead?
How about:
case KVM_S390_INITIAL_RESET: arg = KVM_S390_VCPU_RESET_INITIAL;
Add a "/* fallthrough */" comment here...
Quoted 7 lines
case KVM_S390_VCPU_RESET:
r = kvm_arch_vcpu_ioctl_reset(vcpu, arg);
break;... then this looks good, yes!
Thomas
Janosch Frank
maintainerRFC 34/37 KVM: s390: protvirt: Report CPU state to Ultravisor
In reply to Janosch FrankVCPU states have to be reported to the ultravisor for SIGP interpretation.
--- arch/s390/kvm/kvm-s390.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
arch/s390/kvm/kvm-s390.c +4 -1
@@ -4439,7 +4439,8 @@ void kvm_s390_vcpu_start(struct kvm_vcpu *vcpu)*/ __disable_ibs_on_all_vcpus (vcpu->kvm); } - +/* Let's tell the UV that we want to start again */ +kvm_s390_pv_set_cpu_state (vcpu,PV_CPU_STATE_OPR );kvm_s390_clear_cpuflags (vcpu,CPUSTAT_STOPPED );/* * Another VCPU might have used IBS while we were offline.
@@ -4467,6 +4468,8 @@ void kvm_s390_vcpu_stop(struct kvm_vcpu *vcpu)kvm_s390_clear_stop_irq (vcpu);kvm_s390_set_cpuflags (vcpu,CPUSTAT_STOPPED ); +/* Let's tell the UV that we successfully stopped the vcpu */ +kvm_s390_pv_set_cpu_state (vcpu,PV_CPU_STATE_STP );__disable_ibs_on_vcpu (vcpu);for (i =0 ; i < online_vcpus; i++) {
Signature · 1 line
2.20.1
Janosch Frank
maintainerRFC 35/37 KVM: s390: Fix cpu reset local IRQ clearing
In reply to Janosch FrankThe architecture states that we need to reset local IRQs for all CPU resets. Because the old reset interface did not support the normal CPU reset we never did that.
Now that we have a new interface, let's properly clear out local IRQs and let this commit be a reminder.
--- arch/s390/kvm/kvm-s390.c | 2 ++ 1 file changed, 2 insertions(+)
arch/s390/kvm/kvm-s390.c +2 -0
@@ -3485,6 +3485,8 @@ static int kvm_arch_vcpu_ioctl_reset(struct kvm_vcpu *vcpu,* non-protected case. */ rc =0 ; +kvm_clear_async_pf_completion_queue (vcpu); +kvm_s390_clear_local_irqs (vcpu);if (kvm_s390_pv_handle_cpu (vcpu)) { rc =uv_cmd_nodata (kvm_s390_pv_handle_cpu (vcpu),UVC_CMD_CPU_RESET , &ret);
Signature · 1 line
2.20.1
Re: [RFC 35/37] KVM: s390: Fix cpu reset local IRQ clearing
Quoted 18 lines · Janosch Frank
The architecture states that we need to reset local IRQs for all CPU resets. Because the old reset interface did not support the normal CPU reset we never did that.
Now that we have a new interface, let's properly clear out local IRQs and let this commit be a reminder.
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/kvm-s390.c | 2 ++ 1 file changed, 2 insertions(+)arch/s390/kvm/kvm-s390.c +0
-0
arch/s390/kvm/kvm-s390.c static int kvm_arch_vcpu_ioctl_reset(struct kvm_vcpu *vcpu, · line 3492
Quoted 8 lines
arch/s390/kvm/kvm-s390.c +2
-0@@ -3485,6 +3485,8 @@ static int kvm_arch_vcpu_ioctl_reset(struct kvm_vcpu *vcpu,* non-protected case. */ rc =0 ; +kvm_clear_async_pf_completion_queue (vcpu); +kvm_s390_clear_local_irqs (vcpu);if (kvm_s390_pv_handle_cpu (vcpu)) { rc =uv_cmd_nodata (kvm_s390_pv_handle_cpu (vcpu),UVC_CMD_CPU_RESET , &ret);
I think you could squash this into patch 33/37 where you've introduced the RESET_NORMAL (and adjust the patch description there).
Thomas
Janosch Frank
maintainerRe: [RFC 35/37] KVM: s390: Fix cpu reset local IRQ clearing
Quoted 31 lines · Thomas Huth
Quoted 18 lines · Janosch Frank
The architecture states that we need to reset local IRQs for all CPU resets. Because the old reset interface did not support the normal CPU reset we never did that.
Now that we have a new interface, let's properly clear out local IRQs and let this commit be a reminder.
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/kvm/kvm-s390.c | 2 ++ 1 file changed, 2 insertions(+)arch/s390/kvm/kvm-s390.c +0
-0arch/s390/kvm/kvm-s390.c static int kvm_arch_vcpu_ioctl_reset(struct kvm_vcpu *vcpu, · line 3492
Quoted 8 lines
arch/s390/kvm/kvm-s390.c +2
-0@@ -3485,6 +3485,8 @@ static int kvm_arch_vcpu_ioctl_reset(struct kvm_vcpu *vcpu,* non-protected case. */ rc =0 ; +kvm_clear_async_pf_completion_queue (vcpu); +kvm_s390_clear_local_irqs (vcpu);if (kvm_s390_pv_handle_cpu (vcpu)) { rc =uv_cmd_nodata (kvm_s390_pv_handle_cpu (vcpu),UVC_CMD_CPU_RESET , &ret);I think you could squash this into patch 33/37 where you've introduced the
RESET_NORMAL(and adjust the patch description there).
Quoted 2 lines
Thomas
Yes, that hunk was singled out to have an item to discuss internally. Since we now established, that it is needed, I can squash it.
Janosch Frank
maintainerRFC 36/37 KVM: s390: protvirt: Support cmd 5 operation state
In reply to Janosch FrankCode 5 for the set cpu state UV call tells the UV to load a PSW from the SE header (first IPL) or from guest location 0x0 (diag 308 subcode 0/1). Also it sets the cpu into operating state afterwards, so we can start it.
--- arch/s390/include/asm/uv.h | 1 + arch/s390/kvm/kvm-s390.c | 4 ++++ include/uapi/linux/kvm.h | 1 + 3 files changed, 6 insertions(+)
arch/s390/include/asm/uv.h +1 -0
@@ -163,6 +163,7 @@ struct uv_cb_unp {#define PV_CPU_STATE_OPR 1#define PV_CPU_STATE_STP 2#define PV_CPU_STATE_CHKSTP 3 +#define PV_CPU_STATE_OPR_LOAD 5struct uv_cb_cpu_set_state {struct uv_cb_header header;
arch/s390/kvm/kvm-s390.c +4 -0
@@ -4652,6 +4652,10 @@ static int kvm_s390_handle_pv_vcpu(struct kvm_vcpu *vcpu, r =kvm_s390_pv_destroy_cpu (vcpu);break ; } +case KVM_PV_VCPU_SET_IPL_PSW : { + r =kvm_s390_pv_set_cpu_state (vcpu,PV_CPU_STATE_OPR_LOAD ); +break ; + }default : r = -ENOTTY ; }
include/uapi/linux/kvm.h +1 -0
@@ -1483,6 +1483,7 @@ enum pv_cmd_id {KVM_PV_VM_UNSHARE ,KVM_PV_VCPU_CREATE ,KVM_PV_VCPU_DESTROY , + KVM_PV_VCPU_SET_IPL_PSW, }; struct kvm_pv_cmd {
Signature · 1 line
2.20.1
Re: [RFC 36/37] KVM: s390: protvirt: Support cmd 5 operation state
Quoted 29 lines · Janosch Frank
Code 5 for the set cpu state UV call tells the UV to load a PSW from the SE header (first IPL) or from guest location
0x0(diag 308 subcode 0/1). Also it sets the cpu into operating state afterwards, so we can start it.Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/asm/uv.h | 1 + arch/s390/kvm/kvm-s390.c | 4 ++++ include/uapi/linux/kvm.h | 1 + 3 files changed, 6 insertions(+)arch/s390/include/asm/uv.h +1
-0@@ -163,6 +163,7 @@ struct uv_cb_unp {#define PV_CPU_STATE_OPR 1#define PV_CPU_STATE_STP 2#define PV_CPU_STATE_CHKSTP 3 +#define PV_CPU_STATE_OPR_LOAD 5struct uv_cb_cpu_set_state {struct uv_cb_header header;arch/s390/kvm/kvm-s390.c +0
-0
arch/s390/kvm/kvm-s390.c static int kvm_s390_handle_pv_vcpu(struct kvm_vcpu *vcpu, · line 4658
Quoted 7 lines
arch/s390/kvm/kvm-s390.c +4
-0@@ -4652,6 +4652,10 @@ static int kvm_s390_handle_pv_vcpu(struct kvm_vcpu *vcpu, r =kvm_s390_pv_destroy_cpu (vcpu);break ; } +case KVM_PV_VCPU_SET_IPL_PSW : { + r =kvm_s390_pv_set_cpu_state (vcpu,PV_CPU_STATE_OPR_LOAD ); +break ; + }
Nit: No need for the curly braces here.
Thomas
Re: [RFC 36/37] KVM: s390: protvirt: Support cmd 5 operation state
In reply to Janosch FrankQuoted 4 lines · On Thu, 24 Oct 2019 07:40:58 -0400 Janosch Frank <[email protected]> wrote:
Code 5 for the set cpu state UV call tells the UV to load a PSW from the SE header (first IPL) or from guest location
0x0(diag 308 subcode 0/1). Also it sets the cpu into operating state afterwards, so we can start it.
I'm a bit confused by the patch description: Does this mean that the UV does the transition to operating state? Does the hypervisor get a notification for that?
Quoted 47 lines
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/asm/uv.h | 1 + arch/s390/kvm/kvm-s390.c | 4 ++++ include/uapi/linux/kvm.h | 1 + 3 files changed, 6 insertions(+)arch/s390/include/asm/uv.h +1
-0@@ -163,6 +163,7 @@ struct uv_cb_unp {#define PV_CPU_STATE_OPR 1#define PV_CPU_STATE_STP 2#define PV_CPU_STATE_CHKSTP 3 +#define PV_CPU_STATE_OPR_LOAD 5struct uv_cb_cpu_set_state {struct uv_cb_header header;arch/s390/kvm/kvm-s390.c +4
-0@@ -4652,6 +4652,10 @@ static int kvm_s390_handle_pv_vcpu(struct kvm_vcpu *vcpu, r =kvm_s390_pv_destroy_cpu (vcpu);break ; } +case KVM_PV_VCPU_SET_IPL_PSW : { + r =kvm_s390_pv_set_cpu_state (vcpu,PV_CPU_STATE_OPR_LOAD ); +break ; + }default : r = -ENOTTY ; }include/uapi/linux/kvm.h +1
-0@@ -1483,6 +1483,7 @@ enum pv_cmd_id {KVM_PV_VM_UNSHARE ,KVM_PV_VCPU_CREATE ,KVM_PV_VCPU_DESTROY , + KVM_PV_VCPU_SET_IPL_PSW, }; struct kvm_pv_cmd {
Janosch Frank
maintainerRe: [RFC 36/37] KVM: s390: protvirt: Support cmd 5 operation state
Quoted 11 lines · Cornelia Huck
Quoted 4 lines · On Thu, 24 Oct 2019 07:40:58 -0400 Janosch Frank <[email protected]> wrote:
Code 5 for the set cpu state UV call tells the UV to load a PSW from the SE header (first IPL) or from guest location
0x0(diag 308 subcode 0/1). Also it sets the cpu into operating state afterwards, so we can start it.I'm a bit confused by the patch description: Does this mean that the UV does the transition to operating state? Does the hypervisor get a notification for that?
CMD 5 is defined as "load psw and set to operating". Currently QEMU will still go out and do a "set to operating" after the cmd 5 because our current infrastructure does it and it's basically a nop, so I didn't want to put in the effort to remove it.
Quoted 49 lines
Quoted 48 lines
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/asm/uv.h | 1 + arch/s390/kvm/kvm-s390.c | 4 ++++ include/uapi/linux/kvm.h | 1 + 3 files changed, 6 insertions(+)arch/s390/include/asm/uv.h +1
-0@@ -163,6 +163,7 @@ struct uv_cb_unp {#define PV_CPU_STATE_OPR 1#define PV_CPU_STATE_STP 2#define PV_CPU_STATE_CHKSTP 3 +#define PV_CPU_STATE_OPR_LOAD 5struct uv_cb_cpu_set_state {struct uv_cb_header header;arch/s390/kvm/kvm-s390.c +4
-0@@ -4652,6 +4652,10 @@ static int kvm_s390_handle_pv_vcpu(struct kvm_vcpu *vcpu, r =kvm_s390_pv_destroy_cpu (vcpu);break ; } +case KVM_PV_VCPU_SET_IPL_PSW : { + r =kvm_s390_pv_set_cpu_state (vcpu,PV_CPU_STATE_OPR_LOAD ); +break ; + }default : r = -ENOTTY ; }include/uapi/linux/kvm.h +1
-0@@ -1483,6 +1483,7 @@ enum pv_cmd_id {KVM_PV_VM_UNSHARE ,KVM_PV_VCPU_CREATE ,KVM_PV_VCPU_DESTROY , + KVM_PV_VCPU_SET_IPL_PSW, }; struct kvm_pv_cmd {
Re: [RFC 36/37] KVM: s390: protvirt: Support cmd 5 operation state
Quoted 9 lines · On Tue, 19 Nov 2019 09:13:11 +0100 Janosch Frank <[email protected]> wrote:
Quoted 8 lines · Cornelia Huck
Quoted 4 lines · On Thu, 24 Oct 2019 07:40:58 -0400 Janosch Frank <[email protected]> wrote:
Code 5 for the set cpu state UV call tells the UV to load a PSW from the SE header (first IPL) or from guest location
0x0(diag 308 subcode 0/1). Also it sets the cpu into operating state afterwards, so we can start it.
Quoted 8 lines
Quoted 3 lines
I'm a bit confused by the patch description: Does this mean that the UV does the transition to operating state? Does the hypervisor get a notification for that?
CMD 5 is defined as "load psw and set to operating". Currently QEMU will still go out and do a "set to operating" after the cmd 5 because our current infrastructure does it and it's basically a nop, so I didn't want to put in the effort to remove it.
So, the "it" setting the cpu into operating state is QEMU, via the mpstate interface, which triggers that call? Or is that implicit, but it does not hurt to do it again (which would make more sense to me)?
Assuming the latter, what about the following description:
"KVM: s390: protvirt: support setting cpu state 5
Setting code 5 ("load psw and set to operating") in the set cpu state UV call tells the UV to load a PSW either from the SE header (first IPL) or from guest location 0x0 (diag 308 subcode 0/1). Subsequently, the cpu is set into operating state by the UV.
Note that we can still instruct the UV to set the cpu into operating state explicitly afterwards."
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/asm/uv.h | 1 + arch/s390/kvm/kvm-s390.c | 4 ++++ include/uapi/linux/kvm.h | 1 + 3 files changed, 6 insertions(+)arch/s390/include/asm/uv.h +1
-0@@ -163,6 +163,7 @@ struct uv_cb_unp {#define PV_CPU_STATE_OPR 1#define PV_CPU_STATE_STP 2#define PV_CPU_STATE_CHKSTP 3 +#define PV_CPU_STATE_OPR_LOAD 5
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
arch/s390/include/asm/uv.h +0
-0struct uv_cb_cpu_set_state {struct uv_cb_header header;arch/s390/kvm/kvm-s390.c +0
-0Quoted 8 lines
arch/s390/kvm/kvm-s390.c static int kvm_s390_handle_pv_vcpu(struct kvm_vcpu *vcpu, · line 4656
Quoted 8 lines
arch/s390/kvm/kvm-s390.c +2
-0@@ -4652,6 +4652,10 @@ static int kvm_s390_handle_pv_vcpu(struct kvm_vcpu *vcpu, r =kvm_s390_pv_destroy_cpu (vcpu);break ; } +case KVM_PV_VCPU_SET_IPL_PSW : { + r =kvm_s390_pv_set_cpu_state (vcpu,PV_CPU_STATE_OPR_LOAD );
Also maybe add a comment here that setting into oper state (again) can be done separately?
Quoted 20 lines
Quoted 18 lines
Quoted 17 lines
arch/s390/kvm/kvm-s390.c +2
-0+break ; + }default : r = -ENOTTY ; }include/uapi/linux/kvm.h +1
-0@@ -1483,6 +1483,7 @@ enum pv_cmd_id {KVM_PV_VM_UNSHARE ,KVM_PV_VCPU_CREATE ,KVM_PV_VCPU_DESTROY , + KVM_PV_VCPU_SET_IPL_PSW, }; struct kvm_pv_cmd {
Janosch Frank
maintainerRe: [RFC 36/37] KVM: s390: protvirt: Support cmd 5 operation state
Quoted 16 lines · Cornelia Huck
Quoted 13 lines · On Tue, 19 Nov 2019 09:13:11 +0100 Janosch Frank <[email protected]> wrote:
Quoted 11 lines · Cornelia Huck
Quoted 4 lines · On Thu, 24 Oct 2019 07:40:58 -0400 Janosch Frank <[email protected]> wrote:
Code 5 for the set cpu state UV call tells the UV to load a PSW from the SE header (first IPL) or from guest location
0x0(diag 308 subcode 0/1). Also it sets the cpu into operating state afterwards, so we can start it.I'm a bit confused by the patch description: Does this mean that the UV does the transition to operating state? Does the hypervisor get a notification for that?
Quoted 8 lines
Quoted 4 lines
CMD 5 is defined as "load psw and set to operating". Currently QEMU will still go out and do a "set to operating" after the cmd 5 because our current infrastructure does it and it's basically a nop, so I didn't want to put in the effort to remove it.
So, the "it" setting the cpu into operating state is QEMU, via the mpstate interface, which triggers that call? Or is that implicit, but it does not hurt to do it again (which would make more sense to me)?
Qemu sets operating state via mpstate.
I could fence that via env->pv checks but that would also mean more code and setting operating twice is just a nop.
Quoted 5 lines
Assuming the latter, what about the following description:
"KVM: s390: protvirt: support setting cpu state 5
Quoted 7 lines
Setting code 5 ("load psw and set to operating") in the set cpu state UV call tells the UV to load a PSW either from the SE header (first IPL) or from guest location
0x0(diag 308 subcode 0/1). Subsequently, the cpu is set into operating state by the UV.Note that we can still instruct the UV to set the cpu into operating state explicitly afterwards."
Sounds good
Quoted 58 lines
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
Signed-off-by Janosch Frank <[email protected]>--- arch/s390/include/asm/uv.h | 1 + arch/s390/kvm/kvm-s390.c | 4 ++++ include/uapi/linux/kvm.h | 1 + 3 files changed, 6 insertions(+)arch/s390/include/asm/uv.h +1
-0@@ -163,6 +163,7 @@ struct uv_cb_unp {#define PV_CPU_STATE_OPR 1#define PV_CPU_STATE_STP 2#define PV_CPU_STATE_CHKSTP 3 +#define PV_CPU_STATE_OPR_LOAD 5Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
arch/s390/include/asm/uv.h +0
-0struct uv_cb_cpu_set_state {struct uv_cb_header header;arch/s390/kvm/kvm-s390.c +0
-0Quoted 8 lines
arch/s390/kvm/kvm-s390.c static int kvm_s390_handle_pv_vcpu(struct kvm_vcpu *vcpu, · line 4656
Quoted 8 lines
arch/s390/kvm/kvm-s390.c +2
-0@@ -4652,6 +4652,10 @@ static int kvm_s390_handle_pv_vcpu(struct kvm_vcpu *vcpu, r =kvm_s390_pv_destroy_cpu (vcpu);break ; } +case KVM_PV_VCPU_SET_IPL_PSW : { + r =kvm_s390_pv_set_cpu_state (vcpu,PV_CPU_STATE_OPR_LOAD );Also maybe add a comment here that setting into oper state (again) can be done separately?
Quoted 20 lines
Quoted 18 lines
Quoted 17 lines
arch/s390/kvm/kvm-s390.c +2
-0+break ; + }default : r = -ENOTTY ; }include/uapi/linux/kvm.h +1
-0@@ -1483,6 +1483,7 @@ enum pv_cmd_id {KVM_PV_VM_UNSHARE ,KVM_PV_VCPU_CREATE ,KVM_PV_VCPU_DESTROY , + KVM_PV_VCPU_SET_IPL_PSW, }; struct kvm_pv_cmd {
Janosch Frank
maintainerRFC 37/37 KVM: s390: protvirt: Add UV debug trace
In reply to Janosch FrankLet's have some debug traces which stay around for longer than the guest.
--- arch/s390/kvm/kvm-s390.c | 10 +++++++++- arch/s390/kvm/kvm-s390.h | 9 +++++++++ arch/s390/kvm/pv.c | 16 ++++++++++++++++ 3 files changed, 34 insertions(+), 1 deletion(-)
arch/s390/kvm/kvm-s390.c +9 -1
@@ -221,6 +221,7 @@ static struct kvm_s390_vm_cpu_subfunc kvm_s390_available_subfunc;static struct gmap_notifier gmap_notifier;static struct gmap_notifier vsie_gmap_notifier;debug_info_t *kvm_s390_dbf; +debug_info_t *kvm_s390_dbf_uv;/* Section: not file related */ int kvm_arch_hardware_enable (void )
@@ -462,7 +463,13 @@ int kvm_arch_init(void *opaque)if (!kvm_s390_dbf)return -ENOMEM ; -if (debug_register_view (kvm_s390_dbf, &debug_sprintf_view)) + kvm_s390_dbf_uv =debug_register ("kvm-uv" ,32 ,1 ,7 *sizeof (long )); +if (!kvm_s390_dbf_uv) +return -ENOMEM ; + + +if (debug_register_view (kvm_s390_dbf, &debug_sprintf_view) || +debug_register_view (kvm_s390_dbf_uv, &debug_sprintf_view)) goto out;kvm_s390_cpu_feat_init ();
@@ -489,6 +496,7 @@ void kvm_arch_exit(void) {kvm_s390_gib_destroy ();debug_unregister (kvm_s390_dbf); +debug_unregister (kvm_s390_dbf_uv); }/* Section: device related */
arch/s390/kvm/kvm-s390.h +9 -0
@@ -25,6 +25,15 @@#define IS_ITDB_VALID (vcpu) ((*(char *)vcpu->arch.sie_block->itdba == TDB_FORMAT1))extern debug_info_t *kvm_s390_dbf; +extern debug_info_t *kvm_s390_dbf_uv; + +#define KVM_UV_EVENT(d_kvm, d_loglevel, d_string, d_args...)\ +do { \ + debug_sprintf_event(kvm_s390_dbf_uv, d_loglevel, \ + "%s: " d_string "\n", d_kvm->arch.dbf->name, \ + d_args); \ +} while (0) + #define KVM_EVENT(d_loglevel, d_string, d_args...)\ do { \ debug_sprintf_event(kvm_s390_dbf, d_loglevel, d_string "\n", \
arch/s390/kvm/pv.c +16 -0
@@ -100,6 +100,8 @@ int kvm_s390_pv_destroy_vm(struct kvm *kvm) UVC_CMD_DESTROY_SEC_CONF, &ret);VM_EVENT (kvm,3 ,"PROTVIRT DESTROY VM: rc %x rrc %x" , ret >>16 , ret &0x0000ffff ); +KVM_UV_EVENT (kvm,3 ,"PROTVIRT DESTROY VM: rc %x rrc %x" , + ret >>16 , ret &0x0000ffff );return rc; }
@@ -115,6 +117,8 @@ int kvm_s390_pv_destroy_cpu(struct kvm_vcpu *vcpu)VCPU_EVENT (vcpu,3 ,"PROTVIRT DESTROY VCPU: cpu %d rc %x rrc %x" , vcpu->vcpu_id, ret >>16 , ret &0x0000ffff ); +KVM_UV_EVENT (vcpu->kvm,3 ,"PROTVIRT DESTROY VCPU: cpu %d rc %x rrc %x" , + vcpu->vcpu_id, ret >>16 , ret &0x0000ffff ); } free_pages(vcpu->arch.pv.stor_base,
@@ -163,6 +167,10 @@ int kvm_s390_pv_create_cpu(struct kvm_vcpu *vcpu)VCPU_EVENT (vcpu,3 ,"PROTVIRT CREATE VCPU: cpu %d handle %llx rc %x rrc %x" , vcpu->vcpu_id, uvcb.cpu_handle, uvcb.header.rc, uvcb.header.rrc); +KVM_UV_EVENT (vcpu->kvm,3 , +"PROTVIRT CREATE VCPU: cpu %d handle %llx rc %x rrc %x" , + vcpu->vcpu_id, uvcb.cpu_handle, uvcb.header.rc, + uvcb.header.rrc);/* Output */ vcpu->arch.pv.handle = uvcb.cpu_handle;
@@ -200,6 +208,10 @@ int kvm_s390_pv_create_vm(struct kvm *kvm) uvcb.guest_handle, uvcb.guest_stor_len, uvcb.header.rc, uvcb.header.rrc); +KVM_UV_EVENT (kvm,3 ,"PROTVIRT CREATE VM: handle %llx len %llx rc %x rrc %x" , + uvcb.guest_handle, uvcb.guest_stor_len, uvcb.header.rc, + uvcb.header.rrc); +/* Outputs */ kvm->arch.pv.handle = uvcb.guest_handle;
@@ -230,6 +242,8 @@ int kvm_s390_pv_set_sec_parms(struct kvm *kvm, rc =uv_call (0 , (u64)&uvcb);VM_EVENT (kvm,3 ,"PROTVIRT VM SET PARMS: rc %x rrc %x" , uvcb.header.rc, uvcb.header.rrc); +KVM_UV_EVENT (kvm,3 ,"PROTVIRT VM SET PARMS: rc %x rrc %x" , + uvcb.header.rc, uvcb.header.rrc);if (rc)return -EINVAL ;return 0 ;
@@ -278,6 +292,8 @@ int kvm_s390_pv_unpack(struct kvm *kvm, unsigned long addr, unsigned long size, }VM_EVENT (kvm,3 ,"PROTVIRT VM UNPACK: finished with rc %x rrc %x" , uvcb.header.rc, uvcb.header.rrc); +KVM_UV_EVENT (kvm,3 ,"PROTVIRT VM UNPACK: finished with rc %x rrc %x" , + uvcb.header.rc, uvcb.header.rrc);return rc; }
Signature · 1 line
2.20.1