Re: [Xen-devel] [PATCH for-4.5 v7 14/21] xen/arm: p2m changes for mem_access support

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Mon, Sep 22, 2014 at 2:20 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:

On Wed, 2014-09-17 at 22:51 +0200, Tamas K Lengyel wrote:
> Add p2m_access_t to struct page_info and add necessary changes for
> page table construction routines to pass the default access information.
> We store the p2m_access_t info in page_info as the PTE lacks enough
> software programmable bits.
>
> Signed-off-by: Tamas K Lengyel <tklengyel@xxxxxxxxxxxxx>
> ---
> v7: - Remove radix tree init/destroy and move p2m_access_t store to page_info.

I'm afraid I don't think this will work, since a single page can be
mapped into multiple guests (e.g. granted), possibly multiple times per
guest (less likely I admit) and AIUI the access type is at least per
guest if not per p2m mapping.

Ah, good point. I'll bring back the radix tree for now and we might revisit this approach later. I really liked the fast hardware-based lookup so some solution may still be viable in this general area.

 

The radix tree would have been tolerable if there was some sort of quick
to check flag in p2m->foo to indicate if it is in use for a given
domain. (I previously suggested a sentinal p2m_access_t value for
example, but a bool etc would work too).

I don't especially like trusting radix_lookup to be cheap for an empty
tree in such a hotpatch as p2m lookup since a) I couldn't trivially
determine that it was cheap in such cases by inspection and b) we then
become dependent on the radix tree implementation.

Ack, the boolean value should be acceptable to indicate if the tree is empty or not.

 

>     - Add p2m_gpfn_lock/unlock functions.
>     - Add bool_t lock input to p2m_lookup and apply_p2m_changes so the caller
>       can specify if locking should be performed. This is needed in order to
>       support mem_access_check from common.

Based on the review of the patch which added mem_access_check resulting
in it not being refactored I expect most of this patch will be changing
again, so I won't look at it further unless you ask me to. (except to
note for future reference that if you have a bool_t parameter you should
genherally pass true/false/bool-expr not 0/1)

Ian.

Thanks, yes, this patch will change soon, expecting to send new revision tomorrow.

Tamas

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