[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.