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

[Christian Borntraeger <borntraeger@de.ibm.com>]

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.