Re: [Xen-devel] [PATCH] p2m: split mem_access into separate files

[Julien Grall <julien.grall@xxxxxxx>]

Hi Tamas,

On 08/12/16 22:57, Tamas K Lengyel wrote:
> This patch relocates mem_access components that are currently mixed with p2m
> code into separate files. This better aligns the code with similar subsystems,
> such as mem_sharing and mem_paging, which are already in separate files. There
> are no code-changes introduced, the patch is mechanical code movement.
Whilst I agree this is a good move in general, the ARM (both 32bits and 
64bits) code deserves more attention to make it work in *all* the case. 
It would have been nice to show that by addressing my concerns when you 
first suggested this move (see [1]).
> On ARM we also relocate the static inline gfn_next_boundary function to p2m.h
> as it is a function the mem_access code needs access to.
>
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
[...]

> +/*
> + * Lookup the MFN corresponding to a domain's GFN.
> + * Lookup mem access in the ratrix tree.
> + * The entries associated to the GFN is considered valid.
> + */
> +p2m_access_t p2m_mem_access_radix_get(struct p2m_domain *p2m, gfn_t gfn)
[...]

> +int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn,
> +                             p2m_access_t a)
[...]

The radix tree functions should stay in p2m.c as they are tie to stage-2 
page table handling.
> +
> +/*
> + * If mem_access is in use it might have been the reason why get_page_from_gva
> + * failed to fetch the page, as it uses the MMU for the permission checking.
> + * Only in these cases we do a software-based type check and fetch the page if
> + * we indeed found a conflicting mem_access setting.
> + */
> +struct page_info*
> +p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag)
This interface should take a vCPU in parameter and not rely on current.

Also, p2m_mem_access_check_get_page is using the hardware to translate a 
VA to an IPA. This only works if the memory where the stage-1 page 
tables reside is not protected. The upcoming support of altp2m will make 
things trickier.
[1] 
https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg00037.html
--
Julien Grall

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