diff mbox series

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

Message ID 20191024114059.102802-20-frankja@linux.ibm.com
State New, archived
Headers show
Series KVM: s390: Add support for protected VMs | expand

Commit Message

Janosch Frank Oct. 24, 2019, 11:40 a.m. UTC
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(-)

Comments

David Hildenbrand Nov. 4, 2019, 11:25 a.m. UTC | #1
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.
Christian Borntraeger Nov. 5, 2019, 12:01 p.m. UTC | #2
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.
Janosch Frank Nov. 5, 2019, 12:39 p.m. UTC | #3
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 :-)
David Hildenbrand Nov. 5, 2019, 1:55 p.m. UTC | #4
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. ;)
Janosch Frank Nov. 5, 2019, 2:11 p.m. UTC | #5
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?
David Hildenbrand Nov. 5, 2019, 2:18 p.m. UTC | #6
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 :)
Thomas Huth Nov. 14, 2019, 2:44 p.m. UTC | #7
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
Thomas Huth Nov. 14, 2019, 2:46 p.m. UTC | #8
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
Janosch Frank Nov. 14, 2019, 3:56 p.m. UTC | #9
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 mbox series

Patch

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();