On 2016-10-03 20:04, Mark Rutland wrote:
> Hi,
>
> On Mon, Oct 03, 2016 at 07:09:59PM +0200, Jan Kiszka wrote:
>> I could use a good explanation (for a commit log) why I need this
>>
>> diff --git a/hypervisor/arch/arm64/include/asm/paging.h
>> b/hypervisor/arch/arm64/include/asm/paging.h
>> index 2d17acd..76804e0 100644
>> --- a/hypervisor/arch/arm64/include/asm/paging.h
>> +++ b/hypervisor/arch/arm64/include/asm/paging.h
>> @@ -258,8 +258,11 @@ static inline void arm_paging_vcpu_flush_tlbs(void)
>> /* Only executed on hypervisor paging struct changes */
>> static inline void arch_paging_flush_page_tlbs(unsigned long page_addr)
>> {
>> - asm volatile("tlbi vae2, %0\n"
>> - : : "r" (page_addr >> PAGE_SHIFT));
>> + asm volatile(
>> + "dsb ishst\n\t"
>> + "tlbi vae2, %0\n\t"
>> + "dsb ish\n\t"
>> + : : "r" (page_addr >> PAGE_SHIFT));
>> }
>
> Apologies if the below isn't all that relevant; I haven't dug through
> the jailhouse code, and you may already have the necessary barriers and
> maintenance elsewhere.
If at all, then probably only by chance.
>
> Taking this in a few steps (assuming an SMP system for the use of ISH):
>
> (a) In general, to ensure that any write is visible any page table
> walker (AKA PTW) that fills a TLB in the system, a DSB ISHST is
> required. That ensures that it's left any store buffer, and is
> visible to coherent observers including the PTWs.
>
> (b) If there was previously a valid entry for that VA in the translation
> tables, and there has been no TLB maintenance affecting that VA
> since the translation table was modified, TLB maintenance is
> necessary for that VA. This maintenance requires completion with a
> DSB ISH.
>
> If you never had any valid entries for this VA, TLB maintenance is
> necessary (and neither is the related DSB ISH).
>
> (c) After completion of a DSB for either (a) or (a+b), an ISB (or other
> context synchronising event) is necessary to synchronise context on
> the local PE.
>
> This ISB appears to be missing above.
>
>> /* Used to clean the PAGE_MAP_COHERENT page table changes */
>>
>> on the Hi6220 to use the hypervisor mappings reliably. Without it, the
>> access to the first device mapping (GICD) fails. It's not needed on the
>> AMD Opteron, though.
>
> In any case, (a) and (c) above should be necessary.
>
>> And we don't have such barriers on 32-bit ARM.
>
> I believe that you needed them. The ARM ARM makes this fairly clear in
> the appendix on TLB maintenance requirements:
>
> * In the latest ARMv7-A manual (ARM DDI 0406C.c), see D7.5.3 "TLB
> maintenance operations and barriers".
>
> * In the latest ARMv8-A manual, ARM DDI 0487A.j, see K10.5.3, "TLB
> maintenance instructions and barriers".
>
Issue j - I was searching an older version without that chapter...
Thanks, a lot, that's valuable input! I'll update the commit with the
isb and expand it to ARM.
Jan
--
You received this message because you are subscribed to the Google Groups
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
