Re: [PATCH for-4.5 v8 15/19] xen/arm: Data abort exception (R/W) mem_events.

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

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

On Wed, Sep 24, 2014 at 5:02 PM, Ian Campbell <Ian.Campbell@citrix.com>
wrote:

> On Tue, 2014-09-23 at 15:14 +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. A custom boolean,
> access_in_use,
> > specifies if the tree is in use to avoid uneccessary lookups on an empty
> tree.
>
> "unnecessary".
>

Ack.


>
> > @@ -414,12 +417,40 @@ static int p2m_create_table(struct domain *d,
> lpae_t *entry,
> >      return 0;
> >  }
> >
> > +static long p2m_mem_access_radix_set(struct p2m_domain *p2m, unsigned
> long pfn,
> > +                                     p2m_access_t a)
> > +{
> > +    long rc;
> > +    if ( p2m_access_rwx == a )
>
> I think I mentioned this before, but please write your conditionals the
> more conventional way around.
>

I guess my argument for this style was too much tin-foil-hat so Ack.


>
> > +    {
> > +        if ( p2m->access_in_use )
> > +            radix_tree_delete(&p2m->mem_access_settings, pfn);
> > +
> > +        return 0;
> > +    }
> > +
> > +    rc = radix_tree_insert(&p2m->mem_access_settings, pfn,
> > +                           radix_tree_int_to_ptr(a));
> > +    if ( -EEXIST == rc )
> > +    {
> > +        /* If a setting existed already, change it to the new one */
> > +        radix_tree_replace_slot(
> > +            radix_tree_lookup_slot(
> > +                &p2m->mem_access_settings, pfn),
> > +            radix_tree_int_to_ptr(a));
>
> What a shame that the API doesn't allow you to do an insert or replace
> in one go!
>

Could be extended I guess in the future.


>
> > @@ -591,9 +631,14 @@ static int apply_one_level(struct domain *d,
> >
> >      case INSERT:
> >          if ( is_mapping_aligned(*addr, end_gpaddr, *maddr, level_size)
> &&
> > -           /* We do not handle replacing an existing table with a
> superpage */
> > -             (level == 3 || !p2m_table(orig_pte)) )
> > +           /* We do not handle replacing an existing table with a
> superpage
> > +            * or when mem_access is in use. */
> > +             (level == 3 || (!p2m_table(orig_pte) &&
> !p2m->access_in_use)) )
> >          {
> > +            rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a);
> > +            if ( rc < 0 )
> > +                return rc;
>
> I expect the answer is yes, but just to check: It is desirable to be
> able to insert with a specific non-defualt access perms, as opposed to
> INSERT with defaults then MEMACCESS?
>

I would say yes for performance reasons. No need to go through the pages
once for INSERT then once for MEMACCESS.


>
> > @@ -753,6 +799,49 @@ static int apply_one_level(struct domain *d,
> >              *addr += PAGE_SIZE;
> >              return P2M_ONE_PROGRESS_NOP;
> >          }
> > +
> > +    case MEMACCESS:
> > +        if ( level < 3 )
> > +        {
> > +            if ( !p2m_valid(orig_pte) )
> > +            {
> > +                *addr += level_size;
> > +                return P2M_ONE_PROGRESS_NOP;
> > +            }
> > +
> > +            /* Shatter large pages as we descend */
> > +            if ( p2m_mapping(orig_pte) )
> > +            {
> > +                rc = p2m_shatter_page(d, entry, level, flush_cache);
> > +
> > +                if ( rc < 0 )
> > +                    return rc;
>
> I don't know if that matter for this specific sub op but what is
> supposed to happen on error? Should partial work be undone?
>

IMHO if a page cannot be shattered something must be pretty screwed already
(Xen running out of memory?) in which case un-doing the already shattered
pages is the least of our concern..


>
> > @@ -776,6 +865,8 @@ static int apply_p2m_changes(struct domain *d,
> >      unsigned int cur_root_table = ~0;
> >      unsigned int cur_offset[4] = { ~0, };
> >      unsigned int count = 0;
> > +    unsigned long start_gpfn = paddr_to_pfn(start_gpaddr),
> > +                  end_gpfn = paddr_to_pfn(end_gpaddr);
>
> You don't need to update end_gpaddr up, do you?
>

No, these two values are provided by the user and we only keep track of the
progress made in updating the PTEs that fall in-range.


>
> >      bool_t flush = false;
> >      bool_t flush_pt;
> >
> > @@ -821,6 +912,21 @@ static int apply_p2m_changes(struct domain *d,
> >              count = 0;
> >          }
> >
> > +        /*
> > +         * Preempt setting mem_access permissions as required by XSA-89,
> > +         * if it's not the last iteration.
> > +         */
> > +        if ( op == MEMACCESS && count )
> > +        {
> > +            int progress = paddr_to_pfn(addr) - start_gpfn + 1;
> > +            if ( (end_gpfn-start_gpfn) > progress && !(progress & mask)
>
> This differs from the x86 equivalent in that it is not "++progress".
> Doesn't that mean that we can fail to make any progress at all?
>

It is technically ++progress, just the style is different. Note the +1 when
declaring progress. Furthermore, the && count ensures that at least one
iteration is always done when calling with MEMACCESS.


>
> > +int p2m_get_mem_access(struct domain *d, unsigned long gpfn,
> > +                       xenmem_access_t *access)
> > +{
> > +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +    void *i;
> > +    unsigned int index;
> > +
> > +    static const xenmem_access_t memaccess[] = {
> > +#define ACCESS(ac) [p2m_access_##ac] = XENMEM_access_##ac
> > +            ACCESS(n),
> > +            ACCESS(r),
> > +            ACCESS(w),
> > +            ACCESS(rw),
> > +            ACCESS(x),
> > +            ACCESS(rx),
> > +            ACCESS(wx),
> > +            ACCESS(rwx),
> > +            ACCESS(rx2rw),
> > +            ACCESS(n2rwx),
> > +#undef ACCESS
>
> Unlike x86 I think you have pretty much complete freedom to define
> p2m_access_*? Why not just make them 1:1?
>
> (one for a future cleanup if you prefer)
>

I moved p2m_access_t into common in this version of the series..


>
> > +        /* Seting was found in the Radix tree. */
>
> "Setting"
>

Ack.

Thanks,
Tamas

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

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

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