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@xxxxxxxxxxxxx>
>>>>>>> ---
>>>>>>> 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
