On Wed, Apr 8, 2015 at 5:26 PM, Julien Grall <julien.grall@citrix.com>
wrote:

> Hi Tamas,
>
> The code looks good. See few typoes and coding style issue below.
>
> On 26/03/15 22:05, Tamas K Lengyel wrote:
> > +static int p2m_mem_access_radix_set(struct p2m_domain *p2m, unsigned
> long pfn,
> > +                                    p2m_access_t a)
> > +{
> > +    int rc;
> > +
> > +    if ( !p2m->mem_access_enabled )
> > +        return 0;
> > +
> > +    if ( p2m_access_rwx == a )
> > +    {
> > +        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 ( rc == -EEXIST )
> > +    {
> > +        /* If a setting existed already, change it to the new one */
>
> s/existed already/already exists/?
>

Ack.


>
> > +        radix_tree_replace_slot(
> > +            radix_tree_lookup_slot(
> > +                &p2m->mem_access_settings, pfn),
> > +            radix_tree_int_to_ptr(a));
> > +        rc = 0;
> > +    }
> > +
> > +    return rc;
> > +}
> > +
> >  enum p2m_operation {
> >      INSERT,
> >      ALLOCATE,
> >      REMOVE,
> >      RELINQUISH,
> >      CACHEFLUSH,
> > +    MEMACCESS,
> >  };
> >
> >  /* Put any references on the single 4K page referenced by pte.  TODO:
> > @@ -560,13 +593,22 @@ static int apply_one_level(struct domain *d,
> >          if ( p2m_valid(orig_pte) )
> >              return P2M_ONE_DESCEND;
> >
> > -        if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) )
> > +        if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) &&
> > +           /* We only create superpages when mem_access is not in use.
> */
> > +             (level == 3 || (level < 3 && !p2m->mem_access_enabled)) )
>
> I'm wondering if it would be better to check "a != p2m_access_rwx"
> rather than "p2m->mem_access_enabled" in order to avoid split when it's
> unnecessary.
>
> Although given the status of this series. I won't bother you to ask you
> this change now :).
>

I think that's application specific if the performance gain would apply.
Normal use (that I can think of) would warrant a default setting of
!p2m_access_rwx with mem_access.


>
> >          {
> >              struct page_info *page;
> >
> >              page = alloc_domheap_pages(d, level_shift - PAGE_SHIFT, 0);
> >              if ( page )
> >              {
> > +                rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr),
> a);
> > +                if ( rc < 0 )
> > +                {
> > +                    free_domheap_page(page);
> > +                    return rc;
> > +                }
> > +
> >                  pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t, a);
> >                  if ( level < 3 )
> >                      pte.p2m.table = 0;
> > @@ -587,8 +629,8 @@ static int apply_one_level(struct domain *d,
> >          /*
> >           * If we get here then we failed to allocate a sufficiently
> >           * large contiguous region for this level (which can't be
> > -         * L3). Create a page table and continue to descend so we try
> > -         * smaller allocations.
> > +         * L3) or mem_access is in use. Create a page table and
> > +         * continue to descend so we try smaller allocations.
> >           */
> >          rc = p2m_create_table(d, entry, 0, flush_cache);
> >          if ( rc < 0 )
> > @@ -598,9 +640,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. */
>
> Coding style:
> /*
>  * blah blah
>  */
>
> [..]
>
> > +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct
> npfec npfec)
>
> [..]
>
> > +    /* Otherwise, check if there is a memory event listener, and send
> the message along */
> > +    if ( !mem_event_check_ring(&v->domain->mem_event->access) )
> > +    {
> > +        /* No listener */
> > +        if ( p2m->access_required )
> > +        {
> > +            gdprintk(XENLOG_INFO, "Memory access permissions failure, "
> > +                                  "no mem_event listener VCPU %d, dom
> %d\n",
> > +                                  v->vcpu_id, v->domain->domain_id);
>
> You may want to use gprintk as gdprintk call will be dropped on
> non-debug build.
>

gdprink is used on the x86 as well for this condition so for now I think it
makes sense to keep things consistent.


>
> [..]
>
> > +/* Set access type for a region of pfns.
> > + * If start_pfn == -1ul, sets the default access type */
>
> Coding style:
>
> /*
>  * Blah blah
>  */
>
> > +long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t
> nr,
> > +                        uint32_t start, uint32_t mask, xenmem_access_t
> access)
>
> [..]
>
> > +int p2m_get_mem_access(struct domain *d, unsigned long gpfn,
> > +                       xenmem_access_t *access)
>
>
> [..]
>
> > +    /* If request to get default access */
> > +    if ( gpfn == ~0ull )
>
> gpfn is an unsigned long. You may want to use ~0ul here.
>
> [..]
>
> > diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h
> > index 29f3628..56d1afe 100644
> > --- a/xen/include/xen/p2m-common.h
> > +++ b/xen/include/xen/p2m-common.h
> > @@ -44,4 +44,14 @@ int unmap_mmio_regions(struct domain *d,
> >                         unsigned long nr,
> >                         unsigned long mfn);
> >
> > +/* Set access type for a region of pfns.
> > + * If start_pfn == -1ul, sets the default access type */
> > +long p2m_set_mem_access(struct domain *d, unsigned long start_pfn,
> uint32_t nr,
> > +                        uint32_t start, uint32_t mask, xenmem_access_t
> access);
> > +
>
> Coding style:
>
> /*
>  * Blah blah
>  */
>
> > +/* Get access type for a pfn
> > + * If pfn == -1ul, gets the default access type */
> > +int p2m_get_mem_access(struct domain *d, unsigned long pfn,
> > +                       xenmem_access_t *access);
> > +
>
> Ditto
>
>
> >  #endif /* _XEN_P2M_COMMON_H */
> >
>
> Regards,
>
> --
> Julien Grall
>

Thanks, style issues fixed.

Cheers,
Tamas