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

[Julien Grall <julien.grall@xxxxxxx>]

On 01/08/16 17:34, Mark Rutland wrote:
> Hi Julien,
Hi Mark,

> 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@xxxxxxx>
> > > > Cc: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> > > > Cc: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> > > >
> > > > ---
> > > >    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?
I meant TLBI instruction sorry for the confusion.

> > 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.
Agreed, however we only need one TLBI instruction (assuming there is no 
superpage shattering) per-batch rather than one per-entry in this case.
> > 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?
I agree that in general TLBI instruction does not require the full BBM 
sequence. If we ever need the TLB invalidation per entry, it will be 
better to keep BBM to keep the code streamlined.
However, in this case I think it will be better to return to the guest 
and replay the instruction if a data/instruction abort has been received.
Regards,

--
Julien Grall

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