On Wed, Apr 8, 2015 at 5:26 PM, Julien Grall <julien.gr...@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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
