Xen project Mailing List

On Wed, Apr 8, 2015 at 5:26 PM, Julien Grall <julien.grall@xxxxxxxxxx> 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

Re: [Xen-devel] [PATCH V14 4/7] xen/arm: Data abort exception (R/W) mem_access events