| Message ID | 20191024114059.102802-20-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: Add support for protected VMs | expand |
On 24.10.19 13:40, Janosch Frank wrote: > Guest registers for protected guests are stored at offset 0x380. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > arch/s390/include/asm/kvm_host.h | 4 +++- > arch/s390/kvm/kvm-s390.c | 11 +++++++++++ > 2 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index 0ab309b7bf4c..5deabf9734d9 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -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 */ > }; > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 490fde080107..97d3a81e5074 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -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, > + 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. I would have assume that this is not required for prot virt, because the HW has direct access via the sie block? 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. 2. Avoid this copying here. We have to store the state to vcpu->run->s.regs.gprs when returning to user space and restore the state when coming from user space. Also, we access the GPRS from interception handlers, there we might use wrappers like kvm_s390_set_gprs() kvm_s390_get_gprs() to route to the right location. There are multiple options to optimize this.
On 04.11.19 12:25, David Hildenbrand wrote: > On 24.10.19 13:40, Janosch Frank wrote: >> Guest registers for protected guests are stored at offset 0x380. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> arch/s390/include/asm/kvm_host.h | 4 +++- >> arch/s390/kvm/kvm-s390.c | 11 +++++++++++ >> 2 files changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >> index 0ab309b7bf4c..5deabf9734d9 100644 >> --- a/arch/s390/include/asm/kvm_host.h >> +++ b/arch/s390/include/asm/kvm_host.h >> @@ -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 */ >> }; >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index 490fde080107..97d3a81e5074 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -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, >> + 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. > > 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. > > > 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. > > 2. Avoid this copying here. We have to store the state to vcpu->run->s.regs.gprs when returning to user space and restore the state when coming from user space. I like this proposal better than the first one and > > 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? > > to route to the right location. There are multiple options to optimize this.
On 11/5/19 1:01 PM, Christian Borntraeger wrote: > > > On 04.11.19 12:25, David Hildenbrand wrote: >> On 24.10.19 13:40, Janosch Frank wrote: >>> Guest registers for protected guests are stored at offset 0x380. >>> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>> --- >>> arch/s390/include/asm/kvm_host.h | 4 +++- >>> arch/s390/kvm/kvm-s390.c | 11 +++++++++++ >>> 2 files changed, 14 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >>> index 0ab309b7bf4c..5deabf9734d9 100644 >>> --- a/arch/s390/include/asm/kvm_host.h >>> +++ b/arch/s390/include/asm/kvm_host.h >>> @@ -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 */ >>> }; >>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>> index 490fde080107..97d3a81e5074 100644 >>> --- a/arch/s390/kvm/kvm-s390.c >>> +++ b/arch/s390/kvm/kvm-s390.c >>> @@ -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, >>> + 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. >> >> 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. > >> >> >> 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. > >> >> 2. Avoid this copying here. We have to store the state to vcpu->run->s.regs.gprs when returning to user space and restore the state when coming from user space. > > I like this proposal better than the first one and >> >> 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? I second all of that :-)
On 05.11.19 13:39, Janosch Frank wrote: > On 11/5/19 1:01 PM, Christian Borntraeger wrote: >> >> >> On 04.11.19 12:25, David Hildenbrand wrote: >>> On 24.10.19 13:40, Janosch Frank wrote: >>>> Guest registers for protected guests are stored at offset 0x380. >>>> >>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>> --- >>>> arch/s390/include/asm/kvm_host.h | 4 +++- >>>> arch/s390/kvm/kvm-s390.c | 11 +++++++++++ >>>> 2 files changed, 14 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >>>> index 0ab309b7bf4c..5deabf9734d9 100644 >>>> --- a/arch/s390/include/asm/kvm_host.h >>>> +++ b/arch/s390/include/asm/kvm_host.h >>>> @@ -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 */ >>>> }; >>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>>> index 490fde080107..97d3a81e5074 100644 >>>> --- a/arch/s390/kvm/kvm-s390.c >>>> +++ b/arch/s390/kvm/kvm-s390.c >>>> @@ -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, >>>> + 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. >>> >>> 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. >> >>> >>> >>> 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. >> >>> >>> 2. Avoid this copying here. We have to store the state to vcpu->run->s.regs.gprs when 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 >>> >>> 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? 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. ;)
On 11/5/19 2:55 PM, David Hildenbrand wrote: > On 05.11.19 13:39, Janosch Frank wrote: >> On 11/5/19 1:01 PM, Christian Borntraeger wrote: >>> >>> >>> On 04.11.19 12:25, David Hildenbrand wrote: >>>> On 24.10.19 13:40, Janosch Frank wrote: >>>>> Guest registers for protected guests are stored at offset 0x380. >>>>> >>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>>> --- >>>>> arch/s390/include/asm/kvm_host.h | 4 +++- >>>>> arch/s390/kvm/kvm-s390.c | 11 +++++++++++ >>>>> 2 files changed, 14 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >>>>> index 0ab309b7bf4c..5deabf9734d9 100644 >>>>> --- a/arch/s390/include/asm/kvm_host.h >>>>> +++ b/arch/s390/include/asm/kvm_host.h >>>>> @@ -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 */ >>>>> }; >>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>>>> index 490fde080107..97d3a81e5074 100644 >>>>> --- a/arch/s390/kvm/kvm-s390.c >>>>> +++ b/arch/s390/kvm/kvm-s390.c >>>>> @@ -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, >>>>> + 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. >>>> >>>> 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. >>> >>>> >>>> >>>> 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. >>> >>>> >>>> 2. Avoid this copying here. We have to store the state to vcpu->run->s.regs.gprs when 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 > >>>> >>>> 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? > > 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?
On 05.11.19 15:11, Janosch Frank wrote: > On 11/5/19 2:55 PM, David Hildenbrand wrote: >> On 05.11.19 13:39, Janosch Frank wrote: >>> On 11/5/19 1:01 PM, Christian Borntraeger wrote: >>>> >>>> >>>> On 04.11.19 12:25, David Hildenbrand wrote: >>>>> On 24.10.19 13:40, Janosch Frank wrote: >>>>>> Guest registers for protected guests are stored at offset 0x380. >>>>>> >>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>>>> --- >>>>>> arch/s390/include/asm/kvm_host.h | 4 +++- >>>>>> arch/s390/kvm/kvm-s390.c | 11 +++++++++++ >>>>>> 2 files changed, 14 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >>>>>> index 0ab309b7bf4c..5deabf9734d9 100644 >>>>>> --- a/arch/s390/include/asm/kvm_host.h >>>>>> +++ b/arch/s390/include/asm/kvm_host.h >>>>>> @@ -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 */ >>>>>> }; >>>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>>>>> index 490fde080107..97d3a81e5074 100644 >>>>>> --- a/arch/s390/kvm/kvm-s390.c >>>>>> +++ b/arch/s390/kvm/kvm-s390.c >>>>>> @@ -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, >>>>>> + 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. >>>>> >>>>> 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. >>>> >>>>> >>>>> >>>>> 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. >>>> >>>>> >>>>> 2. Avoid this copying here. We have to store the state to vcpu->run->s.regs.gprs when 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 >> >>>>> >>>>> 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? >> >> 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? 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 :)
On 24/10/2019 13.40, Janosch Frank wrote: > Guest registers for protected guests are stored at offset 0x380. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > arch/s390/include/asm/kvm_host.h | 4 +++- > arch/s390/kvm/kvm-s390.c | 11 +++++++++++ > 2 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index 0ab309b7bf4c..5deabf9734d9 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -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? > struct kvm_s390_itdb itdb; /* 0x0600 */ > __u8 reserved700[2304]; /* 0x0700 */ > }; Thomas
On 05/11/2019 15.18, David Hildenbrand wrote: > On 05.11.19 15:11, Janosch Frank wrote: >> On 11/5/19 2:55 PM, David Hildenbrand wrote: >>> On 05.11.19 13:39, Janosch Frank wrote: >>>> On 11/5/19 1:01 PM, Christian Borntraeger wrote: >>>>> >>>>> >>>>> On 04.11.19 12:25, David Hildenbrand wrote: >>>>>> On 24.10.19 13:40, Janosch Frank wrote: >>>>>>> Guest registers for protected guests are stored at offset 0x380. >>>>>>> >>>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>>>>> --- >>>>>>> arch/s390/include/asm/kvm_host.h | 4 +++- >>>>>>> arch/s390/kvm/kvm-s390.c | 11 +++++++++++ >>>>>>> 2 files changed, 14 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/arch/s390/include/asm/kvm_host.h >>>>>>> b/arch/s390/include/asm/kvm_host.h >>>>>>> index 0ab309b7bf4c..5deabf9734d9 100644 >>>>>>> --- a/arch/s390/include/asm/kvm_host.h >>>>>>> +++ b/arch/s390/include/asm/kvm_host.h >>>>>>> @@ -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 */ >>>>>>> }; >>>>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>>>>>> index 490fde080107..97d3a81e5074 100644 >>>>>>> --- a/arch/s390/kvm/kvm-s390.c >>>>>>> +++ b/arch/s390/kvm/kvm-s390.c >>>>>>> @@ -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, >>>>>>> + 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. >>>>>> >>>>>> 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. >>>>> >>>>>> >>>>>> >>>>>> 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. >>>>> >>>>>> >>>>>> 2. Avoid this copying here. We have to store the state to >>>>>> vcpu->run->s.regs.gprs when 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 >>> >>>>>> >>>>>> 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? >>> >>> 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? > > 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
On 11/14/19 3:44 PM, Thomas Huth wrote: > On 24/10/2019 13.40, Janosch Frank wrote: >> Guest registers for protected guests are stored at offset 0x380. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> arch/s390/include/asm/kvm_host.h | 4 +++- >> arch/s390/kvm/kvm-s390.c | 11 +++++++++++ >> 2 files changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >> index 0ab309b7bf4c..5deabf9734d9 100644 >> --- a/arch/s390/include/asm/kvm_host.h >> +++ b/arch/s390/include/asm/kvm_host.h >> @@ -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 > >> struct kvm_s390_itdb itdb; /* 0x0600 */ >> __u8 reserved700[2304]; /* 0x0700 */ >> }; > > Thomas >
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h index 0ab309b7bf4c..5deabf9734d9 100644 --- a/arch/s390/include/asm/kvm_host.h +++ b/arch/s390/include/asm/kvm_host.h @@ -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 */ }; diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index 490fde080107..97d3a81e5074 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -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, + 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)); + } local_irq_disable(); __enable_cpu_timer_accounting(vcpu); guest_exit_irqoff();
Guest registers for protected guests are stored at offset 0x380. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- arch/s390/include/asm/kvm_host.h | 4 +++- arch/s390/kvm/kvm-s390.c | 11 +++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-)