[PATCH] KVM: s390: force bp isolation for VSIE

[PATCH] KVM: s390: force bp isolation for VSIE

[Christian Borntraeger]

If the guest runs with bp isolation when doing a SIE instruction,
we must also run the nested guest with bp isolation when emulating
that SIE instruction.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/vsie.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index ec772700ff96..b8e7660d7207 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -821,6 +821,7 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 {
 	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
 	struct kvm_s390_sie_block *scb_o = vsie_page->scb_o;
+	int guest_bp_isolation;
 	int rc;
 
 	handle_last_fault(vcpu, vsie_page);
@@ -831,6 +832,15 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 		s390_handle_mcck();
 
 	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
+
+	/* save current guest state of bp isolation override */
+	guest_bp_isolation = test_thread_flag(TIF_ISOLATE_BP_GUEST);
+
+	/* if guest runs with bp isolation force it on nested guest */
+	if (test_kvm_facility(vcpu->kvm, 82) &&
+	    vcpu->arch.sie_block->fpf & FPF_BPBC)
+		set_thread_flag(TIF_ISOLATE_BP_GUEST);
+
 	local_irq_disable();
 	guest_enter_irqoff();
 	local_irq_enable();
@@ -840,6 +850,11 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
 	local_irq_disable();
 	guest_exit_irqoff();
 	local_irq_enable();
+
+	/* restore guest state for bp isolation override */
+	if (!guest_bp_isolation)
+		clear_thread_flag(TIF_ISOLATE_BP_GUEST);
+
 	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
 
 	if (rc == -EINTR) {
-- 
2.14.3

Re: [PATCH] KVM: s390: force bp isolation for VSIE

[David Hildenbrand]

On 14.02.2018 09:34, Christian Borntraeger wrote:
> If the guest runs with bp isolation when doing a SIE instruction,
> we must also run the nested guest with bp isolation when emulating
> that SIE instruction.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/kvm/vsie.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index ec772700ff96..b8e7660d7207 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -821,6 +821,7 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  {
>  	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
>  	struct kvm_s390_sie_block *scb_o = vsie_page->scb_o;
> +	int guest_bp_isolation;
>  	int rc;
>  
>  	handle_last_fault(vcpu, vsie_page);
> @@ -831,6 +832,15 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  		s390_handle_mcck();
>  
>  	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
> +
> +	/* save current guest state of bp isolation override */
> +	guest_bp_isolation = test_thread_flag(TIF_ISOLATE_BP_GUEST);

If I am not wrong, this is not "guest state". The guest state is
vcpu->arch.sie_block->fpf . This is host state of a thread.

> +
> +	/* if guest runs with bp isolation force it on nested guest */
> +	if (test_kvm_facility(vcpu->kvm, 82) &&
> +	    vcpu->arch.sie_block->fpf & FPF_BPBC)
> +		set_thread_flag(TIF_ISOLATE_BP_GUEST);
> +
>  	local_irq_disable();
>  	guest_enter_irqoff();
>  	local_irq_enable();
> @@ -840,6 +850,11 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>  	local_irq_disable();
>  	guest_exit_irqoff();
>  	local_irq_enable();
> +
> +	/* restore guest state for bp isolation override */
> +	if (!guest_bp_isolation)
> +		clear_thread_flag(TIF_ISOLATE_BP_GUEST);
> +
>  	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
>  
>  	if (rc == -EINTR) {
> 

You are trying to optimize the following case here:

1. TIF_ISOLATE_BP_GUEST is not set
2. The guest has facility 82 and enabled FPF_BPBC

As the vSIE guest can change its FPF_BPBC, there is basically no
guarantee to that. So, when entering/leaving the nested guest, you act
like the hardware would be doing FPF_BPBC - as it could be disabled for
the nested guest / the nested guest can change the state itself.

However I wonder what the semantics of FPF_BPBC should be. Shouldn't it
be the case that if the guest has enabled FPF_BPBC, that it is forced on
for the nested guest? (HW is missing a control to force it on).

Unfortunately, I don't have access to documentation, can you clarify?

-- 

Thanks,

David / dhildenb

Re: [PATCH] KVM: s390: force bp isolation for VSIE

[Christian Borntraeger]

On 02/14/2018 11:06 AM, David Hildenbrand wrote:
> On 14.02.2018 09:34, Christian Borntraeger wrote:
>> If the guest runs with bp isolation when doing a SIE instruction,
>> we must also run the nested guest with bp isolation when emulating
>> that SIE instruction.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  arch/s390/kvm/vsie.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index ec772700ff96..b8e7660d7207 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -821,6 +821,7 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>  {
>>  	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
>>  	struct kvm_s390_sie_block *scb_o = vsie_page->scb_o;
>> +	int guest_bp_isolation;
>>  	int rc;
>>  
>>  	handle_last_fault(vcpu, vsie_page);
>> @@ -831,6 +832,15 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>  		s390_handle_mcck();
>>  
>>  	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
>> +
>> +	/* save current guest state of bp isolation override */
>> +	guest_bp_isolation = test_thread_flag(TIF_ISOLATE_BP_GUEST);
> 
> If I am not wrong, this is not "guest state". The guest state is
> vcpu->arch.sie_block->fpf . This is host state of a thread.

Yes, this is the host thread that is going to "emulate" the vsie instruction
by calling sie64a.

> 
>> +
>> +	/* if guest runs with bp isolation force it on nested guest */
>> +	if (test_kvm_facility(vcpu->kvm, 82) &&
>> +	    vcpu->arch.sie_block->fpf & FPF_BPBC)
>> +		set_thread_flag(TIF_ISOLATE_BP_GUEST);
>> +
>>  	local_irq_disable();
>>  	guest_enter_irqoff();
>>  	local_irq_enable();
>> @@ -840,6 +850,11 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>  	local_irq_disable();
>>  	guest_exit_irqoff();
>>  	local_irq_enable();
>> +
>> +	/* restore guest state for bp isolation override */
>> +	if (!guest_bp_isolation)
>> +		clear_thread_flag(TIF_ISOLATE_BP_GUEST);
>> +
>>  	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
>>  
>>  	if (rc == -EINTR) {
>>
> 
> You are trying to optimize the following case here:

I am trying to fix a case where vsie would allow to disable branch prediction blocking.
> 
> 1. TIF_ISOLATE_BP_GUEST is not set
> 2. The guest has facility 82 and enabled FPF_BPBC


> As the vSIE guest can change its FPF_BPBC, there is basically no
> guarantee to that. So, when entering/leaving the nested guest, you act
> like the hardware would be doing FPF_BPBC - as it could be disabled for
> the nested guest / the nested guest can change the state itself.

The BPBC is an effective control, so if you enter SIE with bp blocking,
then the guest will have bp blocking "forced" on.

> 
> However I wonder what the semantics of FPF_BPBC should be. Shouldn't it
> be the case that if the guest has enabled FPF_BPBC, that it is forced on
> for the nested guest? (HW is missing a control to force it on).

the forcing happens by being an effective control. Imagine it like setting
the TIF bit will basically turn on FPF_BPBC on the LPAR level before going
into SIE and the  effective value for guest3 running via vsie as guest2 
will be that this guest3 can do ppa12/13 as it likes, it will always run
with bp blocking.

So this covers the case where guest2 runs a guest3 with bp blocking. 
> Unfortunately, I don't have access to documentation, can you clarify?
> 

Re: [PATCH] KVM: s390: force bp isolation for VSIE

[Christian Borntraeger]

On 02/14/2018 11:14 AM, Christian Borntraeger wrote:
> 
> 
> On 02/14/2018 11:06 AM, David Hildenbrand wrote:
>> On 14.02.2018 09:34, Christian Borntraeger wrote:
>>> If the guest runs with bp isolation when doing a SIE instruction,
>>> we must also run the nested guest with bp isolation when emulating
>>> that SIE instruction.
>>>
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> ---
>>>  arch/s390/kvm/vsie.c | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>> index ec772700ff96..b8e7660d7207 100644
>>> --- a/arch/s390/kvm/vsie.c
>>> +++ b/arch/s390/kvm/vsie.c
>>> @@ -821,6 +821,7 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>  {
>>>  	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
>>>  	struct kvm_s390_sie_block *scb_o = vsie_page->scb_o;
>>> +	int guest_bp_isolation;
>>>  	int rc;
>>>  
>>>  	handle_last_fault(vcpu, vsie_page);
>>> @@ -831,6 +832,15 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>  		s390_handle_mcck();
>>>  
>>>  	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
>>> +
>>> +	/* save current guest state of bp isolation override */
>>> +	guest_bp_isolation = test_thread_flag(TIF_ISOLATE_BP_GUEST);
>>
>> If I am not wrong, this is not "guest state". The guest state is
>> vcpu->arch.sie_block->fpf . This is host state of a thread.
> 
> Yes, this is the host thread that is going to "emulate" the vsie instruction
> by calling sie64a.
> 
>>
>>> +
>>> +	/* if guest runs with bp isolation force it on nested guest */
>>> +	if (test_kvm_facility(vcpu->kvm, 82) &&
>>> +	    vcpu->arch.sie_block->fpf & FPF_BPBC)
>>> +		set_thread_flag(TIF_ISOLATE_BP_GUEST);
>>> +
>>>  	local_irq_disable();
>>>  	guest_enter_irqoff();
>>>  	local_irq_enable();
>>> @@ -840,6 +850,11 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>  	local_irq_disable();
>>>  	guest_exit_irqoff();
>>>  	local_irq_enable();
>>> +
>>> +	/* restore guest state for bp isolation override */
>>> +	if (!guest_bp_isolation)
>>> +		clear_thread_flag(TIF_ISOLATE_BP_GUEST);
>>> +
>>>  	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
>>>  
>>>  	if (rc == -EINTR) {
>>>
>>
>> You are trying to optimize the following case here:
> 
> I am trying to fix a case where vsie would allow to disable branch prediction blocking.
>>
>> 1. TIF_ISOLATE_BP_GUEST is not set
>> 2. The guest has facility 82 and enabled FPF_BPBC
> 
> 
>> As the vSIE guest can change its FPF_BPBC, there is basically no
>> guarantee to that. So, when entering/leaving the nested guest, you act
>> like the hardware would be doing FPF_BPBC - as it could be disabled for
>> the nested guest / the nested guest can change the state itself.
> 
> The BPBC is an effective control, so if you enter SIE with bp blocking,
> then the guest will have bp blocking "forced" on.
> 
>>
>> However I wonder what the semantics of FPF_BPBC should be. Shouldn't it
>> be the case that if the guest has enabled FPF_BPBC, that it is forced on
>> for the nested guest? (HW is missing a control to force it on).
> 
> the forcing happens by being an effective control. Imagine it like setting
> the TIF bit will basically turn on FPF_BPBC on the LPAR level before going
> into SIE and the  effective value for guest3 running via vsie as guest2 

FWIW, check 

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/s390/kernel/entry.S?id=6b73044b2b0081ee3dd1cd6eaab7dee552601efb

How the TIF bit will make the host kernel call ppa12 before calling SIE.

Re: [PATCH] KVM: s390: force bp isolation for VSIE

[David Hildenbrand]

On 14.02.2018 11:14, Christian Borntraeger wrote:
> 
> 
> On 02/14/2018 11:06 AM, David Hildenbrand wrote:
>> On 14.02.2018 09:34, Christian Borntraeger wrote:
>>> If the guest runs with bp isolation when doing a SIE instruction,
>>> we must also run the nested guest with bp isolation when emulating
>>> that SIE instruction.
>>>
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> ---
>>>  arch/s390/kvm/vsie.c | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>> index ec772700ff96..b8e7660d7207 100644
>>> --- a/arch/s390/kvm/vsie.c
>>> +++ b/arch/s390/kvm/vsie.c
>>> @@ -821,6 +821,7 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>  {
>>>  	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
>>>  	struct kvm_s390_sie_block *scb_o = vsie_page->scb_o;
>>> +	int guest_bp_isolation;
>>>  	int rc;
>>>  
>>>  	handle_last_fault(vcpu, vsie_page);
>>> @@ -831,6 +832,15 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>  		s390_handle_mcck();
>>>  
>>>  	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
>>> +
>>> +	/* save current guest state of bp isolation override */
>>> +	guest_bp_isolation = test_thread_flag(TIF_ISOLATE_BP_GUEST);
>>
>> If I am not wrong, this is not "guest state". The guest state is
>> vcpu->arch.sie_block->fpf . This is host state of a thread.
> 
> Yes, this is the host thread that is going to "emulate" the vsie instruction
> by calling sie64a.
> 
>>
>>> +
>>> +	/* if guest runs with bp isolation force it on nested guest */
>>> +	if (test_kvm_facility(vcpu->kvm, 82) &&
>>> +	    vcpu->arch.sie_block->fpf & FPF_BPBC)
>>> +		set_thread_flag(TIF_ISOLATE_BP_GUEST);
>>> +
>>>  	local_irq_disable();
>>>  	guest_enter_irqoff();
>>>  	local_irq_enable();
>>> @@ -840,6 +850,11 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>  	local_irq_disable();
>>>  	guest_exit_irqoff();
>>>  	local_irq_enable();
>>> +
>>> +	/* restore guest state for bp isolation override */
>>> +	if (!guest_bp_isolation)
>>> +		clear_thread_flag(TIF_ISOLATE_BP_GUEST);
>>> +
>>>  	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
>>>  
>>>  	if (rc == -EINTR) {
>>>
>>
>> You are trying to optimize the following case here:
> 
> I am trying to fix a case where vsie would allow to disable branch prediction blocking.
>>
>> 1. TIF_ISOLATE_BP_GUEST is not set
>> 2. The guest has facility 82 and enabled FPF_BPBC
> 
> 
>> As the vSIE guest can change its FPF_BPBC, there is basically no
>> guarantee to that. So, when entering/leaving the nested guest, you act
>> like the hardware would be doing FPF_BPBC - as it could be disabled for
>> the nested guest / the nested guest can change the state itself.
> 
> The BPBC is an effective control, so if you enter SIE with bp blocking,
> then the guest will have bp blocking "forced" on.

The guest can at least disable BPBC logically. (you can enable the
control in the SCB but the guest can simply turn it off) - that's why we
sync it back in unshadow_scb().

I now understand it like this:

LPAR (BPBC = on) -> Guest BPBC value ignored
LPAR (BPBC = off) -> Guest BPBC value used
LPAR (BPBC = off) -> Guest (BPBC = off) -> Nested guest value used

And you are fixing this case:
LPAR (BPBC = off) -> Guest (BPBC = on) -> Nested guest ignored

And you do this by setting LPAR (BPBC = on) while running the nested guest.

If so, please add a comment

/*
 * The guest is running with BPBC, so we have to force it on for our
 * nested guest. This is done by enabling BPBC globally, so the BPBC
 * control in the SCB (which the nested guest can modify) is simply
 * ignored.
 */
> 
>>
>> However I wonder what the semantics of FPF_BPBC should be. Shouldn't it
>> be the case that if the guest has enabled FPF_BPBC, that it is forced on
>> for the nested guest? (HW is missing a control to force it on).
> 
> the forcing happens by being an effective control. Imagine it like setting
> the TIF bit will basically turn on FPF_BPBC on the LPAR level before going
> into SIE and the  effective value for guest3 running via vsie as guest2 
> will be that this guest3 can do ppa12/13 as it likes, it will always run
> with bp blocking.
> 



-- 

Thanks,

David / dhildenb

Re: [PATCH] KVM: s390: force bp isolation for VSIE

[Christian Borntraeger]

On 02/14/2018 11:37 AM, David Hildenbrand wrote:
> On 14.02.2018 11:14, Christian Borntraeger wrote:
>>
>>
>> On 02/14/2018 11:06 AM, David Hildenbrand wrote:
>>> On 14.02.2018 09:34, Christian Borntraeger wrote:
>>>> If the guest runs with bp isolation when doing a SIE instruction,
>>>> we must also run the nested guest with bp isolation when emulating
>>>> that SIE instruction.
>>>>
>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> ---
>>>>  arch/s390/kvm/vsie.c | 15 +++++++++++++++
>>>>  1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>>> index ec772700ff96..b8e7660d7207 100644
>>>> --- a/arch/s390/kvm/vsie.c
>>>> +++ b/arch/s390/kvm/vsie.c
>>>> @@ -821,6 +821,7 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>  {
>>>>  	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
>>>>  	struct kvm_s390_sie_block *scb_o = vsie_page->scb_o;
>>>> +	int guest_bp_isolation;
>>>>  	int rc;
>>>>  
>>>>  	handle_last_fault(vcpu, vsie_page);
>>>> @@ -831,6 +832,15 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>  		s390_handle_mcck();
>>>>  
>>>>  	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
>>>> +
>>>> +	/* save current guest state of bp isolation override */
>>>> +	guest_bp_isolation = test_thread_flag(TIF_ISOLATE_BP_GUEST);
>>>
>>> If I am not wrong, this is not "guest state". The guest state is
>>> vcpu->arch.sie_block->fpf . This is host state of a thread.
>>
>> Yes, this is the host thread that is going to "emulate" the vsie instruction
>> by calling sie64a.
>>
>>>
>>>> +
>>>> +	/* if guest runs with bp isolation force it on nested guest */
>>>> +	if (test_kvm_facility(vcpu->kvm, 82) &&
>>>> +	    vcpu->arch.sie_block->fpf & FPF_BPBC)
>>>> +		set_thread_flag(TIF_ISOLATE_BP_GUEST);
>>>> +
>>>>  	local_irq_disable();
>>>>  	guest_enter_irqoff();
>>>>  	local_irq_enable();
>>>> @@ -840,6 +850,11 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>  	local_irq_disable();
>>>>  	guest_exit_irqoff();
>>>>  	local_irq_enable();
>>>> +
>>>> +	/* restore guest state for bp isolation override */
>>>> +	if (!guest_bp_isolation)
>>>> +		clear_thread_flag(TIF_ISOLATE_BP_GUEST);
>>>> +
>>>>  	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
>>>>  
>>>>  	if (rc == -EINTR) {
>>>>
>>>
>>> You are trying to optimize the following case here:
>>
>> I am trying to fix a case where vsie would allow to disable branch prediction blocking.
>>>
>>> 1. TIF_ISOLATE_BP_GUEST is not set
>>> 2. The guest has facility 82 and enabled FPF_BPBC
>>
>>
>>> As the vSIE guest can change its FPF_BPBC, there is basically no
>>> guarantee to that. So, when entering/leaving the nested guest, you act
>>> like the hardware would be doing FPF_BPBC - as it could be disabled for
>>> the nested guest / the nested guest can change the state itself.
>>
>> The BPBC is an effective control, so if you enter SIE with bp blocking,
>> then the guest will have bp blocking "forced" on.
> 
> The guest can at least disable BPBC logically. (you can enable the
> control in the SCB but the guest can simply turn it off) - that's why we
> sync it back in unshadow_scb().
> 
> I now understand it like this:
> 
> LPAR (BPBC = on) -> Guest BPBC value ignored
> LPAR (BPBC = off) -> Guest BPBC value used
> LPAR (BPBC = off) -> Guest (BPBC = off) -> Nested guest value used
> 

For full correctness:
s/ignored/not relevant as the effective value is the logical OR/

but ignored is certainly good enough and shorter.

> And you are fixing this case:
> LPAR (BPBC = off) -> Guest (BPBC = on) -> Nested guest ignored


which would run the nested guest with BPBC off. 

> 
> And you do this by setting LPAR (BPBC = on) while running the nested guest.

yes.

> 
> If so, please add a comment
> 
> /*
>  * The guest is running with BPBC, so we have to force it on for our
>  * nested guest. This is done by enabling BPBC globally, so the BPBC
>  * control in the SCB (which the nested guest can modify) is simply
>  * ignored.
>  */

I will replace the
/* if guest runs with bp isolation force it on nested guest */
with your comment.
>>

>>>
>>> However I wonder what the semantics of FPF_BPBC should be. Shouldn't it
>>> be the case that if the guest has enabled FPF_BPBC, that it is forced on
>>> for the nested guest? (HW is missing a control to force it on).
>>
>> the forcing happens by being an effective control. Imagine it like setting
>> the TIF bit will basically turn on FPF_BPBC on the LPAR level before going
>> into SIE and the  effective value for guest3 running via vsie as guest2 
>> will be that this guest3 can do ppa12/13 as it likes, it will always run
>> with bp blocking.
>>
> 
> 
> 

Re: [PATCH] KVM: s390: force bp isolation for VSIE

[David Hildenbrand]

On 14.02.2018 12:05, Christian Borntraeger wrote:
> 
> 
> On 02/14/2018 11:37 AM, David Hildenbrand wrote:
>> On 14.02.2018 11:14, Christian Borntraeger wrote:
>>>
>>>
>>> On 02/14/2018 11:06 AM, David Hildenbrand wrote:
>>>> On 14.02.2018 09:34, Christian Borntraeger wrote:
>>>>> If the guest runs with bp isolation when doing a SIE instruction,
>>>>> we must also run the nested guest with bp isolation when emulating
>>>>> that SIE instruction.
>>>>>
>>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>> ---
>>>>>  arch/s390/kvm/vsie.c | 15 +++++++++++++++
>>>>>  1 file changed, 15 insertions(+)
>>>>>
>>>>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>>>>> index ec772700ff96..b8e7660d7207 100644
>>>>> --- a/arch/s390/kvm/vsie.c
>>>>> +++ b/arch/s390/kvm/vsie.c
>>>>> @@ -821,6 +821,7 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>>  {
>>>>>  	struct kvm_s390_sie_block *scb_s = &vsie_page->scb_s;
>>>>>  	struct kvm_s390_sie_block *scb_o = vsie_page->scb_o;
>>>>> +	int guest_bp_isolation;
>>>>>  	int rc;
>>>>>  
>>>>>  	handle_last_fault(vcpu, vsie_page);
>>>>> @@ -831,6 +832,15 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>>  		s390_handle_mcck();
>>>>>  
>>>>>  	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
>>>>> +
>>>>> +	/* save current guest state of bp isolation override */
>>>>> +	guest_bp_isolation = test_thread_flag(TIF_ISOLATE_BP_GUEST);
>>>>
>>>> If I am not wrong, this is not "guest state". The guest state is
>>>> vcpu->arch.sie_block->fpf . This is host state of a thread.
>>>
>>> Yes, this is the host thread that is going to "emulate" the vsie instruction
>>> by calling sie64a.
>>>
>>>>
>>>>> +
>>>>> +	/* if guest runs with bp isolation force it on nested guest */
>>>>> +	if (test_kvm_facility(vcpu->kvm, 82) &&
>>>>> +	    vcpu->arch.sie_block->fpf & FPF_BPBC)
>>>>> +		set_thread_flag(TIF_ISOLATE_BP_GUEST);
>>>>> +
>>>>>  	local_irq_disable();
>>>>>  	guest_enter_irqoff();
>>>>>  	local_irq_enable();
>>>>> @@ -840,6 +850,11 @@ static int do_vsie_run(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>>>>>  	local_irq_disable();
>>>>>  	guest_exit_irqoff();
>>>>>  	local_irq_enable();
>>>>> +
>>>>> +	/* restore guest state for bp isolation override */
>>>>> +	if (!guest_bp_isolation)
>>>>> +		clear_thread_flag(TIF_ISOLATE_BP_GUEST);
>>>>> +
>>>>>  	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
>>>>>  
>>>>>  	if (rc == -EINTR) {
>>>>>
>>>>
>>>> You are trying to optimize the following case here:
>>>
>>> I am trying to fix a case where vsie would allow to disable branch prediction blocking.
>>>>
>>>> 1. TIF_ISOLATE_BP_GUEST is not set
>>>> 2. The guest has facility 82 and enabled FPF_BPBC
>>>
>>>
>>>> As the vSIE guest can change its FPF_BPBC, there is basically no
>>>> guarantee to that. So, when entering/leaving the nested guest, you act
>>>> like the hardware would be doing FPF_BPBC - as it could be disabled for
>>>> the nested guest / the nested guest can change the state itself.
>>>
>>> The BPBC is an effective control, so if you enter SIE with bp blocking,
>>> then the guest will have bp blocking "forced" on.
>>
>> The guest can at least disable BPBC logically. (you can enable the
>> control in the SCB but the guest can simply turn it off) - that's why we
>> sync it back in unshadow_scb().
>>
>> I now understand it like this:
>>
>> LPAR (BPBC = on) -> Guest BPBC value ignored
>> LPAR (BPBC = off) -> Guest BPBC value used
>> LPAR (BPBC = off) -> Guest (BPBC = off) -> Nested guest value used
>>
> 
> For full correctness:
> s/ignored/not relevant as the effective value is the logical OR/
> 
> but ignored is certainly good enough and shorter.
> 
>> And you are fixing this case:
>> LPAR (BPBC = off) -> Guest (BPBC = on) -> Nested guest ignored
> 
> 
> which would run the nested guest with BPBC off. 
> 
>>
>> And you do this by setting LPAR (BPBC = on) while running the nested guest.
> 
> yes.
> 
>>
>> If so, please add a comment
>>
>> /*
>>  * The guest is running with BPBC, so we have to force it on for our
>>  * nested guest. This is done by enabling BPBC globally, so the BPBC
>>  * control in the SCB (which the nested guest can modify) is simply
>>  * ignored.
>>  */
> 
> I will replace the
> /* if guest runs with bp isolation force it on nested guest */
> with your comment.

With that

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 

Thanks,

David / dhildenb

Re: [PATCH] KVM: s390: force bp isolation for VSIE

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 705 bytes --]

On 14.02.2018 12:05, Christian Borntraeger wrote:
> 
> 
> On 02/14/2018 11:37 AM, David Hildenbrand wrote:
>> On 14.02.2018 11:14, Christian Borntraeger wrote:
>>>
>>>
>>> On 02/14/2018 11:06 AM, David Hildenbrand wrote:
>>>> On 14.02.2018 09:34, Christian Borntraeger wrote:
>>>>> If the guest runs with bp isolation when doing a SIE instruction,
>>>>> we must also run the nested guest with bp isolation when emulating
>>>>> that SIE instruction.

This is done by activating BPBC in the lpar, which acts as an override
for lower level guests.

Reviewed-by: Janosch Frank <frankja@linux.vnet.ibm.com>

Maybe also add a short comment, that the fpf bit is a logical or in
shadow_scb?


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]