HCPTR cp15 writes need isb?

HCPTR cp15 writes need isb?

[Vikram Sethi]

Hi Marc, Christoffer, Catalin, Will,

I'm seeing an issue with KVM HCPTR (cp15) writes on guest entry/exit on one of Qualcomm's CPU cores in AArch32 host and AArch32 guest mode. Our CPU architects believe that HCPTR cp15 writes are context changing and require an isb.
With an isb in set_hcptr macro in arch/arm/kvm/interrupts_head.S I am able to boot the Aarch32 guest, but without it, I see strange crashes to hyp_undef or hyp_pabt.

What is your opinion on HCPTR cp15 writes being context changing and needing isb? I can submit a one line patch that adds the isb at the end of set_hcptr macro.

I did find some examples in ARMv7 ARM with an isb after cp15 write and an ARM blog [1] that states ISB "is used to ensure any previously executed context changing operations (including cp15 operations) will have completed by the time the ISB completed."

I also found this text from an older lkml thread [2] which seems to quote from an ARM document (although I cannot find the same text in ARMv7 ARM) "Any change to CP15 registers is guaranteed to be visible to subsequent
instructions only after one of isb, exception, return from exception"

1 http://community.arm.com/groups/processors/blog/2011/10/19/memory-access-ordering-part-3--memory-access-ordering-in-the-arm-architecture
2 http://lkml.iu.edu/hypermail/linux/kernel/1105.1/00475.html

Thanks,
Vikram

-- 
Vikram Sethi
Qualcomm Technologies Inc, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: HCPTR cp15 writes need isb?

[Marc Zyngier]

On Tue, 16 Jun 2015 02:34:23 +0100
Vikram Sethi <vikrams@codeaurora.org> wrote:

Hi Vikram,

> Hi Marc, Christoffer, Catalin, Will,
> 
> I'm seeing an issue with KVM HCPTR (cp15) writes on guest entry/exit
> on one of Qualcomm's CPU cores in AArch32 host and AArch32 guest
> mode. Our CPU architects believe that HCPTR cp15 writes are context
> changing and require an isb. With an isb in set_hcptr macro in
> arch/arm/kvm/interrupts_head.S I am able to boot the Aarch32 guest,
> but without it, I see strange crashes to hyp_undef or hyp_pabt.

[...]

Can you look at the following patch (queued for 4.2)?

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330955.html

Please let me know if this solves the issue you are seeing.

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.

Re: HCPTR cp15 writes need isb?

[Vikram Sethi]

On 06/16/15 01:46, Marc Zyngier wrote:
> On Tue, 16 Jun 2015 02:34:23 +0100
> Vikram Sethi <vikrams@codeaurora.org> wrote:
>
> Hi Vikram,
>
>> Hi Marc, Christoffer, Catalin, Will,
>>
>> I'm seeing an issue with KVM HCPTR (cp15) writes on guest entry/exit
>> on one of Qualcomm's CPU cores in AArch32 host and AArch32 guest
>> mode. Our CPU architects believe that HCPTR cp15 writes are context
>> changing and require an isb. With an isb in set_hcptr macro in
>> arch/arm/kvm/interrupts_head.S I am able to boot the Aarch32 guest,
>> but without it, I see strange crashes to hyp_undef or hyp_pabt.
> [...]
>
> Can you look at the following patch (queued for 4.2)?
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330955.html
>
> Please let me know if this solves the issue you are seeing.
Don't we have the same issue the first time guest touches FP and traps i.e in switch_to_guest_vfp where we turn on floating point access in HCPTR and immediately access FPEXC in store_vfp_state without a isb?

My first attempt at a fix was similar to yours (add isb only in kvm_vcpu_return path after hcptr update) and while that helped the guest boot further, I still got hyp prefetch abort (hyp_pabt) later in the guest boot, until I also added isbs after the other HCPTR writes.

>
> Thanks,
>
> 	M.


-- 
Vikram Sethi
Qualcomm Technologies Inc, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: HCPTR cp15 writes need isb?

[Marc Zyngier]

On 16/06/15 13:30, Vikram Sethi wrote:
> On 06/16/15 01:46, Marc Zyngier wrote:
>> On Tue, 16 Jun 2015 02:34:23 +0100
>> Vikram Sethi <vikrams@codeaurora.org> wrote:
>>
>> Hi Vikram,
>>
>>> Hi Marc, Christoffer, Catalin, Will,
>>>
>>> I'm seeing an issue with KVM HCPTR (cp15) writes on guest entry/exit
>>> on one of Qualcomm's CPU cores in AArch32 host and AArch32 guest
>>> mode. Our CPU architects believe that HCPTR cp15 writes are context
>>> changing and require an isb. With an isb in set_hcptr macro in
>>> arch/arm/kvm/interrupts_head.S I am able to boot the Aarch32 guest,
>>> but without it, I see strange crashes to hyp_undef or hyp_pabt.
>> [...]
>>
>> Can you look at the following patch (queued for 4.2)?
>>
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330955.html
>>
>> Please let me know if this solves the issue you are seeing.
> Don't we have the same issue the first time guest touches FP and
> traps i.e in switch_to_guest_vfp where we turn on floating point
> access in HCPTR and immediately access FPEXC in store_vfp_state
> without a isb?

Good point, looks like my initial fix is incomplete. I'll repost a more
complete fix but in the meantime, does adding the following work for you?

diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
index 3ac7aca..5b30047 100644
--- a/arch/arm/kvm/interrupts.S
+++ b/arch/arm/kvm/interrupts.S
@@ -487,6 +487,7 @@ switch_to_guest_vfp:

 	@ NEON/VFP used.  Turn on VFP access.
 	set_hcptr vmexit, (HCPTR_TCP(10) | HCPTR_TCP(11))
+	isb	@ Force execution of HCPTR as we've just reenabled VFP access

 	@ Switch VFP/NEON hardware state to the guest's
 	add	r7, r0, #VCPU_VFP_HOST

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: HCPTR cp15 writes need isb?

[Vikram Sethi]

On 06/16/15 07:47, Marc Zyngier wrote:
> On 16/06/15 13:30, Vikram Sethi wrote:
>> On 06/16/15 01:46, Marc Zyngier wrote:
>>> On Tue, 16 Jun 2015 02:34:23 +0100
>>> Vikram Sethi <vikrams@codeaurora.org> wrote:
>>>
>>> Hi Vikram,
>>>
>>>> Hi Marc, Christoffer, Catalin, Will,
>>>>
>>>> I'm seeing an issue with KVM HCPTR (cp15) writes on guest entry/exit
>>>> on one of Qualcomm's CPU cores in AArch32 host and AArch32 guest
>>>> mode. Our CPU architects believe that HCPTR cp15 writes are context
>>>> changing and require an isb. With an isb in set_hcptr macro in
>>>> arch/arm/kvm/interrupts_head.S I am able to boot the Aarch32 guest,
>>>> but without it, I see strange crashes to hyp_undef or hyp_pabt.
>>> [...]
>>>
>>> Can you look at the following patch (queued for 4.2)?
>>>
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330955.html
>>>
>>> Please let me know if this solves the issue you are seeing.
>> Don't we have the same issue the first time guest touches FP and
>> traps i.e in switch_to_guest_vfp where we turn on floating point
>> access in HCPTR and immediately access FPEXC in store_vfp_state
>> without a isb?
> Good point, looks like my initial fix is incomplete. I'll repost a more
> complete fix but in the meantime, does adding the following work for you?
Yes, the additional isb in switch_to_guest_vfp along with your original patch works for me.
When you refactor the original patch will it be cleaner to handle the isb in the set_hcptr macro whenever it is changed to not trap VFP access?

>
> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
> index 3ac7aca..5b30047 100644
> --- a/arch/arm/kvm/interrupts.S
> +++ b/arch/arm/kvm/interrupts.S
> @@ -487,6 +487,7 @@ switch_to_guest_vfp:
>
>  	@ NEON/VFP used.  Turn on VFP access.
>  	set_hcptr vmexit, (HCPTR_TCP(10) | HCPTR_TCP(11))
> +	isb	@ Force execution of HCPTR as we've just reenabled VFP access
>
>  	@ Switch VFP/NEON hardware state to the guest's
>  	add	r7, r0, #VCPU_VFP_HOST
>
> Thanks,
>
> 	M.


-- 
Vikram Sethi
Qualcomm Technologies Inc, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: HCPTR cp15 writes need isb?

[Marc Zyngier]

On 16/06/15 18:51, Vikram Sethi wrote:
> On 06/16/15 07:47, Marc Zyngier wrote:
>> On 16/06/15 13:30, Vikram Sethi wrote:
>>> On 06/16/15 01:46, Marc Zyngier wrote:
>>>> On Tue, 16 Jun 2015 02:34:23 +0100
>>>> Vikram Sethi <vikrams@codeaurora.org> wrote:
>>>>
>>>> Hi Vikram,
>>>>
>>>>> Hi Marc, Christoffer, Catalin, Will,
>>>>>
>>>>> I'm seeing an issue with KVM HCPTR (cp15) writes on guest entry/exit
>>>>> on one of Qualcomm's CPU cores in AArch32 host and AArch32 guest
>>>>> mode. Our CPU architects believe that HCPTR cp15 writes are context
>>>>> changing and require an isb. With an isb in set_hcptr macro in
>>>>> arch/arm/kvm/interrupts_head.S I am able to boot the Aarch32 guest,
>>>>> but without it, I see strange crashes to hyp_undef or hyp_pabt.
>>>> [...]
>>>>
>>>> Can you look at the following patch (queued for 4.2)?
>>>>
>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/330955.html
>>>>
>>>> Please let me know if this solves the issue you are seeing.
>>> Don't we have the same issue the first time guest touches FP and
>>> traps i.e in switch_to_guest_vfp where we turn on floating point
>>> access in HCPTR and immediately access FPEXC in store_vfp_state
>>> without a isb?
>> Good point, looks like my initial fix is incomplete. I'll repost a more
>> complete fix but in the meantime, does adding the following work for you?
> Yes, the additional isb in switch_to_guest_vfp along with your original patch works for me.
> When you refactor the original patch will it be cleaner to handle the
> isb in the set_hcptr macro whenever it is changed to not trap VFP
> access?

That's what I have done, but the result is a bit awkward, so I'm in two
minds about it. I'll post it in a minute, please check that it still
works for you (though I've checked that the generated code is the same).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...