Re: [RFC 19/37] KVM: s390: protvirt: Add new gprs location handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux