Re: [RFC 21/22] xen/arm: p2m: Re-implement p2m_set_mem_access using p2m_{set, get}_entry

[Mark Rutland <mark.rutland@arm.com>]

Hi Julien, 

On Mon, Aug 01, 2016 at 04:40:50PM +0100, Julien Grall wrote:
> Hi Razvan,
> 
> On 28/07/16 16:04, Razvan Cojocaru wrote:
> >On 07/28/2016 05:51 PM, Julien Grall wrote:
> >>The function p2m_set_mem_access can be re-implemented using the generic
> >>functions p2m_get_entry and __p2m_set_entry.
> >>
> >>Note that because of the implementation of p2m_get_entry, a TLB
> >>invalidation instruction will be issued for each 4KB page. Therefore the
> >>performance of memaccess will be impacted, however the function is now
> >>safe on all the processors.
> >>
> >>Also the function apply_p2m_changes is dropped completely as it is not
> >>unused anymore.
> >>
> >>Signed-off-by: Julien Grall <julien.grall@arm.com>
> >>Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
> >>Cc: Tamas K Lengyel <tamas@tklengyel.com>
> >>
> >>---
> >>    I have not ran any performance test with memaccess for now, but I
> >>    expect an important and unavoidable impact because of how memaccess
> >>    has been designed to workaround hardware limitation. Note that might
> >>    be possible to re-work memaccess work on superpage but this should
> >>    be done in a separate patch.
> >>---
> >> xen/arch/arm/p2m.c | 329 +++++++----------------------------------------------
> >> 1 file changed, 38 insertions(+), 291 deletions(-)
> >
> >Thanks for the CC!
> >
> >This seems to only impact ARM, are there any planned changes for x86
> >along these lines as well?
> 
> Actually, it might be possible to remove the TLB for each 4KB entry
> in the memaccess case.

Could you clarify what you mean by "remove the TLB"? Do you mean a TLBI
instruction?

> After I read again multiple time the ARM ARM (D4-1732 in ARM DDI
> 0487A.i) and spoke with various ARM folks, changing the permission
> (i.e read, write, execute) does not require the break-before-make
> sequence.

Regardless of whether Break-Before-Make (BBM) is required, TLB
invalidation is still required to ensure the new permissions have taken
effect after *any* modification to page tables, unless:

* The prior entry was not permitted to be held in a TLB, and:
* No TLB held an entry for the address range.

> However, I noticed a latent bug in the memaccess code when the
> permission restriction are removed/changed.
> 
> In the current implementation (i.e without this series), the TLB
> invalidation is deferred until the p2m is released. Until that time,
> a vCPU may still run with the previous permission and trap into the
> hypervisor the permission fault. However, as the permission as
> changed, p2m_memaccess_check may fail and we will inject an abort to
> the guest.
> 
> The problem is very similar to [1]. This will still be true with
> this series applied if I relaxed the use of the break-before-make
> sequence. The two ways I can see to fix this are either try again
> the instruction (we would trap again if the permission was not
> correct) or keep the break-before-make.

Why can you not have TLB invalidation without the full BBM sequence?

Thanks,
Mark.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel