Xen project Mailing List

On Wed, Sep 24, 2014 at 5:02 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 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

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