On Fri, Mar 27, 2015 at 12:21 AM, Julien Grall <julien.grall@linaro.org>
wrote:

> Hi Tamas,
>
> On 26/03/2015 22:05, Tamas K Lengyel wrote:
>
>> Add missing structure definition for iabt and update the trap handling
>> mechanism to only inject the exception if the mem_access checker
>> decides to do so.
>>
>> Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>> Reviewed-by:  Julien Grall <julien.grall@linaro.org>
>>
>
> IHMO, the Acked-by and Reviewed-by should not have been carried in this
> patch because of the change you made in v14 (mainly the TLB).
>

Yea are probably right.


>
> [..]
>
>  +        /*
>> +         * Flush the TLB to make sure the DTLB is clear before
>> +         * doing GVA->IPA translation. If we got here because of
>> +         * an entry only present in the ITLB, this translation may
>> +         * still be inaccurate.
>> +         */
>> +        flush_tlb_domain(current->domain);
>>
>
> flush TLB domain is very expensive, it flushes TLBs on every CPU. While
> you may only need a flush on the current CPU.
>

Ack, there just isn't a function at the moment to do tlbflush only for a
given cpu.  While I understand the argument behind the performance impact
of the flush, any user of the mem_access system would IMHO prefer accuracy
over performance. As I said before, this path seldom ever triggers without
mem_access triggering it.


>
> Although, on ARMv8, there is no possibility to flush only DTLB or ITLB for
> aarch64. You have to do both at the same time. So the problem you are
> describing can't happen. After reading the ID_MMFR2_EL1, I understand that
> Unified TLB is strongly advice on ARMv8 so any DTLB/ITLB flush would flush
> the unified TLB for aarch32 guest.
>

The problem I'm describing doesn't depend on having separate DTLB/ITLB
flush operations available, albeit those making life a lot for a potential
malicious in-guest kernel. There were no separate flush operations on x86
either and split-TLB poisoning was still a thing. The introduction of the
sTLB is what made it less usable for malicious purposes.


>
> This is different for ARMv7, it may be possible to have a split-TLB. The
> register ID_MMFR2 indicates if the platform implement unified TLB or
> harvard TLB. In the case of the former, any DTLB/ITLB flush will be perform
> on the unified TLB (see B4.2.2 in DDI406C.b).
>
> So it would be possible to avoid the flush in most of the case.
>

Ack, if there is no separate ITLB/DTLB on the hardware and we can tell than
this problem doesn't apply to those devices.


>
> Regards,
>
> --
> Julien Grall
>

Thanks,
Tamas