Xen project Mailing List

Dropped xen-devel by accident on my previous msg.

On Tue, Sep 16, 2014 at 12:53 AM, Ian Campbell <ian.campbell@xxxxxxxxxx> wrote:
On Mon, 2014-09-15 at 16:02 +0200, Tamas K Lengyel wrote:
> This patch enables to store, set, check and deliver LPAE R/W mem_events.
> As the LPAE PTE's lack enough available software programmable bits,
> we store the permissions in a Radix tree, where the key is the pfn
> of a 4k page. Only settings other than p2m_access_rwx are saved
> in the Radix tree.

It is here that it is absolutely essential to consider the overhead for
the non-xenaccess use case, and I think it should be written here in the
commit message.

I'm worried because I'm not seeing the obvious "xenaccess is disabled,
skip all this stuff" path I was hoping for.

I think the overhead of p2m modifications and on fault needs separate
consideration.

For cases where xenaccess isn't enabled IMHO we need to be talking about
overhead in the ballpark of a pointer deref, compare, conditional jump.
Certainly not any kind of radix tree lookup etc.

In the trap handlers only permission faults are checked against the mem_access radix tree, so that already cuts down overhead to a conditional. In my experiments I haven't seen a single instance of a permission fault happening which I didn't cause myself. In p2m modifications as long as the default mem_access is rwx, the code path is the same as before for large pages. For 4k pages when adding them with rwx permission it does have an extra radix tree lookup to clean any potential setting in the radix tree for that page, but that is really just a safety check on my part and if overhead with it is a problem it can be removed. IMHO in the default case the radix tree is empty, so that lookup is essentially just another conditional.

Do you see any other paths that are a cause for concern?

> @@ -149,6 +152,74 @@ static lpae_t *p2m_map_first(struct p2m_domain *p2m, paddr_t addr)
> return __map_domain_page(page);
> }
>
> +static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a)
> +{

It would have been good to refactor this, and probably p2m_shatter_page
in precursor patches to reduce the size of this aleady pretty big patch.

Ack, that would make sense.

> @@ -1159,6 +1329,236 @@ err:
> return page;
> }
>
> +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)

Can a bunch of this function not be common code? i.e. the inject an
event into a ring bits? Or even the automatic conversion of rx2rw etc.

I'll look into it, some bits might be.

While the code logic is in theory the same, unfortunately there are significant differences between handling locks, which makes it not possible to have this code in common. There are also some style differences, like ARM doesn't have set_entry/get_entry pointers in the p2m_domain, as on ARM we don't need to dynamically support different types for those functions (no need to abstract them). The parts that could be in common are only a couple lines here and there which don't really justify having them in common as separate functions.

> @@ -248,6 +249,23 @@ static inline bool_t p2m_mem_event_sanity_check(struct domain *d)
> return 1;
> }
>
> +/* Send mem event based on the access (gla is -1ull if not available). Boolean

Is a "gla" a "guest linear address"? I don't think we use that term on
ARM.

Yea, that's copy-pasta and will clean it up.. on ARM we always have the faulting vaddr it seems to me anyway.

Ian

Thanks,
Tamas

Re: [Xen-devel] [PATCH for-4.5 v6 13/17] xen/arm: Data abort exception (R/W) mem_events.