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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Aug 1, 2016 at 9:40 AM, Julien Grall <julien.grall@xxxxxxx> 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.
>
> 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.
>
> 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.
>
> The former will be cleaner given than stage-2 permission fault can only
> happen because of memaccess for now. Any opinions?
>

IMHO we should just pause the domain while mem_access permissions are
being changed. On x86 it is not necessary as the mem_access
bookkeeping is kept in the ept pte entries but since on ARM the two
things are disjoint it is required. Even on x86 though - while it's
not strictly necessary - I think it would be a good idea to just pause
the domain as from a user perspective it would be more intuitive then
keeping the vCPUs running.

Tamas

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