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

[Tamas K Lengyel <tamas.lengyel@zentific.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3737 bytes --]

Dropped xen-devel by accident on my previous msg.

On Tue, Sep 16, 2014 at 12:53 AM, Ian Campbell <ian.campbell@citrix.com>
> 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
>

[-- Attachment #1.2: Type: text/html, Size: 5506 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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