* [PATCH v8 1/8] xen: introduce gnttab_max_frames and gnttab_max_maptrack_frames command line options
2014-10-20 9:47 [PATCH v8 0/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
@ 2014-10-20 9:48 ` Stefano Stabellini
2014-10-20 10:34 ` Jan Beulich
2014-10-20 9:48 ` [PATCH v8 2/8] xen/arm: rename *_xen_dcache_* operations to *_dcache_* Stefano Stabellini
` (7 subsequent siblings)
8 siblings, 1 reply; 31+ messages in thread
From: Stefano Stabellini @ 2014-10-20 9:48 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, Ian.Campbell, JBeulich, Stefano Stabellini
Introduce gnttab_max_maptrack_frames: a new Xen command line option to
specify the max number of maptrack frames per domain.
Deprecate the old gnttab_max_nr_frames and introduce gnttab_max_frames
instead, that doesn't affect the maptrack. Keep gnttab_max_nr_frames for
compatibility.
Rename internally max_nr_grant_frames to max_grant_frames to avoid
confusions.
Introduce DEFAULT_MAX_MAPTRACK_FRAMES, that is completely independent
from max_nr_grant_frames.
Remove MAX_MAPTRACK_TO_GRANTS_RATIO that is now only used in one place
for compatibility.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v8:
- remove unused #include;
- set max_nr_grant_frames as __initdata;
- set max_grant_frames and max_maptrack_frames as __read_mostly;
- respent coding style for comments;
- remove MAX_MAPTRACK_TO_GRANTS_RATIO.
Changes in v7:
- remove 0 initializers;
- make max_nr_grant_frames static;
- remove preprocessors check on max_nr_grant_frames;
- remove double black line;
- introduce DEFAULT_MAX_MAPTRACK_FRAMES;
- no long lines;
- set the new variables independently if the old one is passed as
argument.
Changes in v6:
- change ordering of options in xen-command-line.markdown;
- initialize all options to 0, then set the defaults on an initcall
depending on what options are used.
Changes in v5:
- /max_nr_maptrack_frames/max_maptrack_frames/g;
- deprecate gnttab_max_nr_frames;
- introduce gnttab_max_frames;
- rename the max_nr_grant_frames variable to max_grant_frames.
---
docs/misc/xen-command-line.markdown | 17 +++++++++-
xen/arch/arm/domain.c | 2 +-
xen/arch/arm/mm.c | 2 +-
xen/arch/x86/mm.c | 2 +-
xen/common/compat/grant_table.c | 12 ++++----
xen/common/grant_table.c | 58 +++++++++++++++++++++++------------
xen/include/asm-arm/grant_table.h | 2 +-
xen/include/xen/grant_table.h | 4 +--
8 files changed, 66 insertions(+), 33 deletions(-)
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 28bbaaf..00416af 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -608,11 +608,26 @@ does not provide VM\_ENTRY\_LOAD\_GUEST\_PAT.
Specify the serial parameters for the GDB stub.
-### gnttab\_max\_nr\_frames
+### gnttab\_max\_frames
> `= <integer>`
Specify the maximum number of frames per grant table operation.
+### gnttab\_max\_maptrack\_frames
+> `= <integer>`
+
+Specify the maximum number of maptrack frames domain.
+The default value is 8 times gnttab_max_frames.
+
+### gnttab\_max\_nr\_frames
+> `= <integer>`
+
+*Deprecated*
+Use gnttab\_max\_frames and gnttab\_max\_maptrack\_frames instead.
+
+Specify the maximum number of frames per grant table operation and the
+maximum number of maptrack frames domain.
+
### guest\_loglvl
> `= <level>[/<rate-limited level>]` where level is `none | error | warning | info | debug | all`
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 2b53931..57f09c3 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -409,7 +409,7 @@ struct domain *alloc_domain_struct(void)
return NULL;
clear_page(d);
- d->arch.grant_table_gpfn = xzalloc_array(xen_pfn_t, max_nr_grant_frames);
+ d->arch.grant_table_gpfn = xzalloc_array(xen_pfn_t, max_grant_frames);
return d;
}
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 97e5bc39..dd70d81 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1054,7 +1054,7 @@ int xenmem_add_to_physmap_one(
else
{
if ( (idx >= nr_grant_frames(d->grant_table)) &&
- (idx < max_nr_grant_frames) )
+ (idx < max_grant_frames) )
gnttab_grow_table(d, idx + 1);
if ( idx < nr_grant_frames(d->grant_table) )
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 6cd7f45..edd1923 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4564,7 +4564,7 @@ int xenmem_add_to_physmap_one(
else
{
if ( (idx >= nr_grant_frames(d->grant_table)) &&
- (idx < max_nr_grant_frames) )
+ (idx < max_grant_frames) )
gnttab_grow_table(d, idx + 1);
if ( idx < nr_grant_frames(d->grant_table) )
diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index 7ebbbc1..2dc1e44 100644
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -146,11 +146,11 @@ int compat_grant_table_op(unsigned int cmd,
unsigned int max_frame_list_size_in_page =
(COMPAT_ARG_XLAT_SIZE - sizeof(*nat.setup)) /
sizeof(*nat.setup->frame_list.p);
- if ( max_frame_list_size_in_page < max_nr_grant_frames )
+ if ( max_frame_list_size_in_page < max_grant_frames )
{
gdprintk(XENLOG_WARNING,
- "max_nr_grant_frames is too large (%u,%u)\n",
- max_nr_grant_frames, max_frame_list_size_in_page);
+ "max_grant_frames is too large (%u,%u)\n",
+ max_grant_frames, max_frame_list_size_in_page);
rc = -EINVAL;
}
else
@@ -284,11 +284,11 @@ int compat_grant_table_op(unsigned int cmd,
break;
}
if ( max_frame_list_size_in_pages <
- grant_to_status_frames(max_nr_grant_frames) )
+ grant_to_status_frames(max_grant_frames) )
{
gdprintk(XENLOG_WARNING,
- "grant_to_status_frames(max_nr_grant_frames) is too large (%u,%u)\n",
- grant_to_status_frames(max_nr_grant_frames),
+ "grant_to_status_frames(max_grant_frames) is too large (%u,%u)\n",
+ grant_to_status_frames(max_grant_frames),
max_frame_list_size_in_pages);
rc = -EINVAL;
break;
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 23266c3..784bbc6 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -40,16 +40,25 @@
#include <xsm/xsm.h>
#include <asm/flushtlb.h>
-#ifndef max_nr_grant_frames
-unsigned int max_nr_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
+/* this option is deprecated, use gnttab_max_frames and
+ gnttab_max_maptrack_frames instead */
+static unsigned int __initdata max_nr_grant_frames;
integer_param("gnttab_max_nr_frames", max_nr_grant_frames);
-#endif
+
+unsigned int __read_mostly max_grant_frames;
+integer_param("gnttab_max_frames", max_grant_frames);
/* The maximum number of grant mappings is defined as a multiplier of the
* maximum number of grant table entries. This defines the multiplier used.
* Pretty arbitrary. [POLICY]
+ * As gnttab_max_nr_frames has been deprecated, this multiplier is deprecated too.
+ * New options allow to set max_maptrack_frames and
+ * map_grant_table_frames independently.
*/
-#define MAX_MAPTRACK_TO_GRANTS_RATIO 8
+#define DEFAULT_MAX_MAPTRACK_FRAMES 256
+
+static unsigned int __read_mostly max_maptrack_frames;
+integer_param("gnttab_max_maptrack_frames", max_maptrack_frames);
/*
* The first two members of a grant entry are updated as a combined pair.
@@ -102,11 +111,6 @@ nr_maptrack_frames(struct grant_table *t)
return t->maptrack_limit / MAPTRACK_PER_PAGE;
}
-static unsigned inline int max_nr_maptrack_frames(void)
-{
- return (max_nr_grant_frames * MAX_MAPTRACK_TO_GRANTS_RATIO);
-}
-
#define MAPTRACK_TAIL (~0u)
#define SHGNT_PER_PAGE_V1 (PAGE_SIZE / sizeof(grant_entry_v1_t))
@@ -164,7 +168,7 @@ num_act_frames_from_sha_frames(const unsigned int num)
}
#define max_nr_active_grant_frames \
- num_act_frames_from_sha_frames(max_nr_grant_frames)
+ num_act_frames_from_sha_frames(max_grant_frames)
static inline unsigned int
nr_active_grant_frames(struct grant_table *gt)
@@ -271,7 +275,7 @@ get_maptrack_handle(
while ( unlikely((handle = __get_maptrack_handle(lgt)) == -1) )
{
nr_frames = nr_maptrack_frames(lgt);
- if ( nr_frames >= max_nr_maptrack_frames() )
+ if ( nr_frames >= max_maptrack_frames )
break;
new_mt = alloc_xenheap_page();
@@ -1265,7 +1269,7 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)
struct grant_table *gt = d->grant_table;
unsigned int i;
- ASSERT(req_nr_frames <= max_nr_grant_frames);
+ ASSERT(req_nr_frames <= max_grant_frames);
gdprintk(XENLOG_INFO,
"Expanding dom (%d) grant table from (%d) to (%d) frames.\n",
@@ -1338,11 +1342,11 @@ gnttab_setup_table(
return -EFAULT;
}
- if ( unlikely(op.nr_frames > max_nr_grant_frames) )
+ if ( unlikely(op.nr_frames > max_grant_frames) )
{
gdprintk(XENLOG_INFO, "Xen only supports up to %d grant-table frames"
" per domain.\n",
- max_nr_grant_frames);
+ max_grant_frames);
op.status = GNTST_general_error;
goto out1;
}
@@ -1377,7 +1381,7 @@ gnttab_setup_table(
{
gdprintk(XENLOG_INFO,
"Expand grant table to %u failed. Current: %u Max: %u\n",
- op.nr_frames, nr_grant_frames(gt), max_nr_grant_frames);
+ op.nr_frames, nr_grant_frames(gt), max_grant_frames);
op.status = GNTST_general_error;
goto out3;
}
@@ -1438,7 +1442,7 @@ gnttab_query_size(
spin_lock(&d->grant_table->lock);
op.nr_frames = nr_grant_frames(d->grant_table);
- op.max_nr_frames = max_nr_grant_frames;
+ op.max_nr_frames = max_grant_frames;
op.status = GNTST_okay;
spin_unlock(&d->grant_table->lock);
@@ -2659,7 +2663,7 @@ grant_table_create(
/* Tracking of mapped foreign frames table */
if ( (t->maptrack = xzalloc_array(struct grant_mapping *,
- max_nr_maptrack_frames())) == NULL )
+ max_maptrack_frames)) == NULL )
goto no_mem_2;
if ( (t->maptrack[0] = alloc_xenheap_page()) == NULL )
goto no_mem_3;
@@ -2670,7 +2674,7 @@ grant_table_create(
t->maptrack[0][i - 1].ref = MAPTRACK_TAIL;
/* Shared grant table. */
- if ( (t->shared_raw = xzalloc_array(void *, max_nr_grant_frames)) == NULL )
+ if ( (t->shared_raw = xzalloc_array(void *, max_grant_frames)) == NULL )
goto no_mem_3;
for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
{
@@ -2681,7 +2685,7 @@ grant_table_create(
/* Status pages for grant table - for version 2 */
t->status = xzalloc_array(grant_status_t *,
- grant_to_status_frames(max_nr_grant_frames));
+ grant_to_status_frames(max_grant_frames));
if ( t->status == NULL )
goto no_mem_4;
@@ -2930,6 +2934,22 @@ static struct keyhandler gnttab_usage_print_all_keyhandler = {
static int __init gnttab_usage_init(void)
{
+ if ( max_nr_grant_frames )
+ {
+ printk(XENLOG_WARNING
+ "gnttab_max_nr_frames is deprecated, use gnttab_max_frames instead\n");
+ if ( !max_grant_frames )
+ max_grant_frames = max_nr_grant_frames;
+ if ( !max_maptrack_frames )
+ max_maptrack_frames = max_nr_grant_frames * 8;
+ }
+
+ if ( !max_grant_frames )
+ max_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
+
+ if ( !max_maptrack_frames )
+ max_maptrack_frames = DEFAULT_MAX_MAPTRACK_FRAMES;
+
register_keyhandler('g', &gnttab_usage_print_all_keyhandler);
return 0;
}
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index 47147ce..e798880 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -31,7 +31,7 @@ static inline int replace_grant_supported(void)
#define gnttab_shared_gmfn(d, t, i) \
( ((i >= nr_grant_frames(d->grant_table)) && \
- (i < max_nr_grant_frames)) ? 0 : (d->arch.grant_table_gpfn[i]))
+ (i < max_grant_frames)) ? 0 : (d->arch.grant_table_gpfn[i]))
#define gnttab_need_iommu_mapping(d) (is_domain_direct_mapped(d))
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 5941191..32f5786 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -49,10 +49,8 @@
/* Default maximum size of a grant table. [POLICY] */
#define DEFAULT_MAX_NR_GRANT_FRAMES 32
#endif
-#ifndef max_nr_grant_frames /* to allow arch to override */
/* The maximum size of a grant table. */
-extern unsigned int max_nr_grant_frames;
-#endif
+extern unsigned int max_grant_frames;
/*
* Tracks a mapping of another domain's grant reference. Each domain has a
--
1.7.10.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v8 1/8] xen: introduce gnttab_max_frames and gnttab_max_maptrack_frames command line options
2014-10-20 9:48 ` [PATCH v8 1/8] xen: introduce gnttab_max_frames and gnttab_max_maptrack_frames command line options Stefano Stabellini
@ 2014-10-20 10:34 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2014-10-20 10:34 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: julien.grall, xen-devel, Ian.Campbell
>>> On 20.10.14 at 11:48, <stefano.stabellini@eu.citrix.com> wrote:
> Changes in v8:
> - remove unused #include;
> - set max_nr_grant_frames as __initdata;
> - set max_grant_frames and max_maptrack_frames as __read_mostly;
> - respent coding style for comments;
Not sure what "respent" here means, but ...
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -40,16 +40,25 @@
> #include <xsm/xsm.h>
> #include <asm/flushtlb.h>
>
> -#ifndef max_nr_grant_frames
> -unsigned int max_nr_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
> +/* this option is deprecated, use gnttab_max_frames and
> + gnttab_max_maptrack_frames instead */
... this comment is still malformed.
> @@ -2930,6 +2934,22 @@ static struct keyhandler
> gnttab_usage_print_all_keyhandler = {
>
> static int __init gnttab_usage_init(void)
> {
> + if ( max_nr_grant_frames )
> + {
> + printk(XENLOG_WARNING
> + "gnttab_max_nr_frames is deprecated, use gnttab_max_frames instead\n");
> + if ( !max_grant_frames )
> + max_grant_frames = max_nr_grant_frames;
> + if ( !max_maptrack_frames )
> + max_maptrack_frames = max_nr_grant_frames * 8;
Did it not occur to you to express the 8 here as
DEFAULT_MAX_MAPTRACK_FRAMES / DEFAULT_MAX_NR_GRANT_FRAMES
(with a BUILD_BUG_ON() making sure the former isn't smaller than
the latter)?
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v8 2/8] xen/arm: rename *_xen_dcache_* operations to *_dcache_*
2014-10-20 9:47 [PATCH v8 0/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
2014-10-20 9:48 ` [PATCH v8 1/8] xen: introduce gnttab_max_frames and gnttab_max_maptrack_frames command line options Stefano Stabellini
@ 2014-10-20 9:48 ` Stefano Stabellini
2014-10-20 14:55 ` Ian Campbell
2014-10-20 9:48 ` [PATCH v8 3/8] xen/arm: return int *_dcache_va_range Stefano Stabellini
` (6 subsequent siblings)
8 siblings, 1 reply; 31+ messages in thread
From: Stefano Stabellini @ 2014-10-20 9:48 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, Ian.Campbell, JBeulich, Stefano Stabellini
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
xen/arch/arm/guestcopy.c | 2 +-
xen/arch/arm/kernel.c | 2 +-
xen/arch/arm/mm.c | 24 ++++++++++++------------
xen/arch/arm/p2m.c | 4 ++--
xen/arch/arm/smpboot.c | 2 +-
xen/include/asm-arm/arm32/page.h | 4 ++--
xen/include/asm-arm/arm64/page.h | 4 ++--
xen/include/asm-arm/page.h | 20 ++++++++++----------
8 files changed, 31 insertions(+), 31 deletions(-)
diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
index 0173597..7dbaeca 100644
--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -27,7 +27,7 @@ static unsigned long raw_copy_to_guest_helper(void *to, const void *from,
p += offset;
memcpy(p, from, size);
if ( flush_dcache )
- clean_xen_dcache_va_range(p, size);
+ clean_dcache_va_range(p, size);
unmap_domain_page(p - offset);
put_page(page);
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 1b8ac9a..209c3dd 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -57,7 +57,7 @@ void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
set_fixmap(FIXMAP_MISC, p, BUFFERABLE);
memcpy(dst, src + s, l);
- clean_xen_dcache_va_range(dst, l);
+ clean_dcache_va_range(dst, l);
paddr += l;
dst += l;
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index dd70d81..e43499a 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -388,7 +388,7 @@ void flush_page_to_ram(unsigned long mfn)
{
void *v = map_domain_page(mfn);
- clean_and_invalidate_xen_dcache_va_range(v, PAGE_SIZE);
+ clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
unmap_domain_page(v);
}
@@ -511,17 +511,17 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
/* Clear the copy of the boot pagetables. Each secondary CPU
* rebuilds these itself (see head.S) */
memset(boot_pgtable, 0x0, PAGE_SIZE);
- clean_and_invalidate_xen_dcache(boot_pgtable);
+ clean_and_invalidate_dcache(boot_pgtable);
#ifdef CONFIG_ARM_64
memset(boot_first, 0x0, PAGE_SIZE);
- clean_and_invalidate_xen_dcache(boot_first);
+ clean_and_invalidate_dcache(boot_first);
memset(boot_first_id, 0x0, PAGE_SIZE);
- clean_and_invalidate_xen_dcache(boot_first_id);
+ clean_and_invalidate_dcache(boot_first_id);
#endif
memset(boot_second, 0x0, PAGE_SIZE);
- clean_and_invalidate_xen_dcache(boot_second);
+ clean_and_invalidate_dcache(boot_second);
memset(boot_third, 0x0, PAGE_SIZE);
- clean_and_invalidate_xen_dcache(boot_third);
+ clean_and_invalidate_dcache(boot_third);
/* Break up the Xen mapping into 4k pages and protect them separately. */
for ( i = 0; i < LPAE_ENTRIES; i++ )
@@ -559,7 +559,7 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
/* Make sure it is clear */
memset(this_cpu(xen_dommap), 0, DOMHEAP_SECOND_PAGES*PAGE_SIZE);
- clean_xen_dcache_va_range(this_cpu(xen_dommap),
+ clean_dcache_va_range(this_cpu(xen_dommap),
DOMHEAP_SECOND_PAGES*PAGE_SIZE);
#endif
}
@@ -570,7 +570,7 @@ int init_secondary_pagetables(int cpu)
/* Set init_ttbr for this CPU coming up. All CPus share a single setof
* pagetables, but rewrite it each time for consistency with 32 bit. */
init_ttbr = (uintptr_t) xen_pgtable + phys_offset;
- clean_xen_dcache(init_ttbr);
+ clean_dcache(init_ttbr);
return 0;
}
#else
@@ -605,15 +605,15 @@ int init_secondary_pagetables(int cpu)
write_pte(&first[first_table_offset(DOMHEAP_VIRT_START+i*FIRST_SIZE)], pte);
}
- clean_xen_dcache_va_range(first, PAGE_SIZE);
- clean_xen_dcache_va_range(domheap, DOMHEAP_SECOND_PAGES*PAGE_SIZE);
+ clean_dcache_va_range(first, PAGE_SIZE);
+ clean_dcache_va_range(domheap, DOMHEAP_SECOND_PAGES*PAGE_SIZE);
per_cpu(xen_pgtable, cpu) = first;
per_cpu(xen_dommap, cpu) = domheap;
/* Set init_ttbr for this CPU coming up */
init_ttbr = __pa(first);
- clean_xen_dcache(init_ttbr);
+ clean_dcache(init_ttbr);
return 0;
}
@@ -1287,7 +1287,7 @@ void clear_and_clean_page(struct page_info *page)
void *p = __map_domain_page(page);
clear_page(p);
- clean_xen_dcache_va_range(p, PAGE_SIZE);
+ clean_dcache_va_range(p, PAGE_SIZE);
unmap_domain_page(p);
}
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 1585d35..20bcc9e 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -343,7 +343,7 @@ static inline void p2m_write_pte(lpae_t *p, lpae_t pte, bool_t flush_cache)
{
write_pte(p, pte);
if ( flush_cache )
- clean_xen_dcache(*p);
+ clean_dcache(*p);
}
/*
@@ -403,7 +403,7 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
clear_page(p);
if ( flush_cache )
- clean_xen_dcache_va_range(p, PAGE_SIZE);
+ clean_dcache_va_range(p, PAGE_SIZE);
unmap_domain_page(p);
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index ee395a1..14054ae 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -374,7 +374,7 @@ int __cpu_up(unsigned int cpu)
/* Open the gate for this CPU */
smp_up_cpu = cpu_logical_map(cpu);
- clean_xen_dcache(smp_up_cpu);
+ clean_dcache(smp_up_cpu);
rc = arch_cpu_up(cpu);
diff --git a/xen/include/asm-arm/arm32/page.h b/xen/include/asm-arm/arm32/page.h
index 9740672..20a6a7f 100644
--- a/xen/include/asm-arm/arm32/page.h
+++ b/xen/include/asm-arm/arm32/page.h
@@ -20,11 +20,11 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
}
/* Inline ASM to flush dcache on register R (may be an inline asm operand) */
-#define __clean_xen_dcache_one(R) STORE_CP32(R, DCCMVAC)
+#define __clean_dcache_one(R) STORE_CP32(R, DCCMVAC)
/* Inline ASM to clean and invalidate dcache on register R (may be an
* inline asm operand) */
-#define __clean_and_invalidate_xen_dcache_one(R) STORE_CP32(R, DCCIMVAC)
+#define __clean_and_invalidate_dcache_one(R) STORE_CP32(R, DCCIMVAC)
/*
* Flush all hypervisor mappings from the TLB and branch predictor of
diff --git a/xen/include/asm-arm/arm64/page.h b/xen/include/asm-arm/arm64/page.h
index bb10164..101d7a8 100644
--- a/xen/include/asm-arm/arm64/page.h
+++ b/xen/include/asm-arm/arm64/page.h
@@ -15,11 +15,11 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
}
/* Inline ASM to flush dcache on register R (may be an inline asm operand) */
-#define __clean_xen_dcache_one(R) "dc cvac, %" #R ";"
+#define __clean_dcache_one(R) "dc cvac, %" #R ";"
/* Inline ASM to clean and invalidate dcache on register R (may be an
* inline asm operand) */
-#define __clean_and_invalidate_xen_dcache_one(R) "dc civac, %" #R ";"
+#define __clean_and_invalidate_dcache_one(R) "dc civac, %" #R ";"
/*
* Flush all hypervisor mappings from the TLB of the local processor.
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index d758b61..1327b00 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -268,48 +268,48 @@ extern size_t cacheline_bytes;
/* Functions for flushing medium-sized areas.
* if 'range' is large enough we might want to use model-specific
* full-cache flushes. */
-static inline void clean_xen_dcache_va_range(const void *p, unsigned long size)
+static inline void clean_dcache_va_range(const void *p, unsigned long size)
{
const void *end;
dsb(sy); /* So the CPU issues all writes to the range */
for ( end = p + size; p < end; p += cacheline_bytes )
- asm volatile (__clean_xen_dcache_one(0) : : "r" (p));
+ asm volatile (__clean_dcache_one(0) : : "r" (p));
dsb(sy); /* So we know the flushes happen before continuing */
}
-static inline void clean_and_invalidate_xen_dcache_va_range
+static inline void clean_and_invalidate_dcache_va_range
(const void *p, unsigned long size)
{
const void *end;
dsb(sy); /* So the CPU issues all writes to the range */
for ( end = p + size; p < end; p += cacheline_bytes )
- asm volatile (__clean_and_invalidate_xen_dcache_one(0) : : "r" (p));
+ asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p));
dsb(sy); /* So we know the flushes happen before continuing */
}
/* Macros for flushing a single small item. The predicate is always
* compile-time constant so this will compile down to 3 instructions in
* the common case. */
-#define clean_xen_dcache(x) do { \
+#define clean_dcache(x) do { \
typeof(x) *_p = &(x); \
if ( sizeof(x) > MIN_CACHELINE_BYTES || sizeof(x) > alignof(x) ) \
- clean_xen_dcache_va_range(_p, sizeof(x)); \
+ clean_dcache_va_range(_p, sizeof(x)); \
else \
asm volatile ( \
"dsb sy;" /* Finish all earlier writes */ \
- __clean_xen_dcache_one(0) \
+ __clean_dcache_one(0) \
"dsb sy;" /* Finish flush before continuing */ \
: : "r" (_p), "m" (*_p)); \
} while (0)
-#define clean_and_invalidate_xen_dcache(x) do { \
+#define clean_and_invalidate_dcache(x) do { \
typeof(x) *_p = &(x); \
if ( sizeof(x) > MIN_CACHELINE_BYTES || sizeof(x) > alignof(x) ) \
- clean_and_invalidate_xen_dcache_va_range(_p, sizeof(x)); \
+ clean_and_invalidate_dcache_va_range(_p, sizeof(x)); \
else \
asm volatile ( \
"dsb sy;" /* Finish all earlier writes */ \
- __clean_and_invalidate_xen_dcache_one(0) \
+ __clean_and_invalidate_dcache_one(0) \
"dsb sy;" /* Finish flush before continuing */ \
: : "r" (_p), "m" (*_p)); \
} while (0)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v8 2/8] xen/arm: rename *_xen_dcache_* operations to *_dcache_*
2014-10-20 9:48 ` [PATCH v8 2/8] xen/arm: rename *_xen_dcache_* operations to *_dcache_* Stefano Stabellini
@ 2014-10-20 14:55 ` Ian Campbell
0 siblings, 0 replies; 31+ messages in thread
From: Ian Campbell @ 2014-10-20 14:55 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: julien.grall, xen-devel, JBeulich
On Mon, 2014-10-20 at 10:48 +0100, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Reviewed-by: Julien Grall <julien.grall@linaro.org>
I always prefer the commit log of a change like this to contain at least
the word "because", so I don't have to guess in 6 months time why this
was considered a good idea.
This seems to stand independently from the rest and appears to be purely
mechanical so no release risk, but I've not seen (or looked for) any
comment from Konrad on this series.
> /* Macros for flushing a single small item. The predicate is always
> * compile-time constant so this will compile down to 3 instructions in
> * the common case. */
> -#define clean_xen_dcache(x) do { \
> +#define clean_dcache(x) do { \
> typeof(x) *_p = &(x); \
> if ( sizeof(x) > MIN_CACHELINE_BYTES || sizeof(x) > alignof(x) ) \
> - clean_xen_dcache_va_range(_p, sizeof(x)); \
> + clean_dcache_va_range(_p, sizeof(x)); \
A bit too mechanical here it seems...
Fix up the whitespace and you can add:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> else \
> asm volatile ( \
> "dsb sy;" /* Finish all earlier writes */ \
> - __clean_xen_dcache_one(0) \
> + __clean_dcache_one(0) \
> "dsb sy;" /* Finish flush before continuing */ \
> : : "r" (_p), "m" (*_p)); \
> } while (0)
>
> -#define clean_and_invalidate_xen_dcache(x) do { \
> +#define clean_and_invalidate_dcache(x) do { \
> typeof(x) *_p = &(x); \
> if ( sizeof(x) > MIN_CACHELINE_BYTES || sizeof(x) > alignof(x) ) \
> - clean_and_invalidate_xen_dcache_va_range(_p, sizeof(x)); \
> + clean_and_invalidate_dcache_va_range(_p, sizeof(x)); \
> else \
> asm volatile ( \
> "dsb sy;" /* Finish all earlier writes */ \
> - __clean_and_invalidate_xen_dcache_one(0) \
> + __clean_and_invalidate_dcache_one(0) \
> "dsb sy;" /* Finish flush before continuing */ \
> : : "r" (_p), "m" (*_p)); \
> } while (0)
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v8 3/8] xen/arm: return int *_dcache_va_range
2014-10-20 9:47 [PATCH v8 0/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
2014-10-20 9:48 ` [PATCH v8 1/8] xen: introduce gnttab_max_frames and gnttab_max_maptrack_frames command line options Stefano Stabellini
2014-10-20 9:48 ` [PATCH v8 2/8] xen/arm: rename *_xen_dcache_* operations to *_dcache_* Stefano Stabellini
@ 2014-10-20 9:48 ` Stefano Stabellini
2014-10-20 14:57 ` Ian Campbell
2014-10-20 9:48 ` [PATCH v8 4/8] xen/arm: introduce invalidate_dcache_va_range Stefano Stabellini
` (5 subsequent siblings)
8 siblings, 1 reply; 31+ messages in thread
From: Stefano Stabellini @ 2014-10-20 9:48 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, Ian.Campbell, JBeulich, Stefano Stabellini
These functions cannot really fail on ARM, but their x86 equivalents can
(-EOPNOTSUPP). Change the prototype to return int.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
Changes in v6:
- do not return int from flush_page_to_ram.
---
xen/arch/arm/mm.c | 2 +-
xen/include/asm-arm/page.h | 6 ++++--
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index e43499a..8e989bf 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -388,7 +388,7 @@ void flush_page_to_ram(unsigned long mfn)
{
void *v = map_domain_page(mfn);
- clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
+ ASSERT(clean_and_invalidate_dcache_va_range(v, PAGE_SIZE) == 0);
unmap_domain_page(v);
}
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 1327b00..26c5856 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -268,16 +268,17 @@ extern size_t cacheline_bytes;
/* Functions for flushing medium-sized areas.
* if 'range' is large enough we might want to use model-specific
* full-cache flushes. */
-static inline void clean_dcache_va_range(const void *p, unsigned long size)
+static inline int clean_dcache_va_range(const void *p, unsigned long size)
{
const void *end;
dsb(sy); /* So the CPU issues all writes to the range */
for ( end = p + size; p < end; p += cacheline_bytes )
asm volatile (__clean_dcache_one(0) : : "r" (p));
dsb(sy); /* So we know the flushes happen before continuing */
+ return 0;
}
-static inline void clean_and_invalidate_dcache_va_range
+static inline int clean_and_invalidate_dcache_va_range
(const void *p, unsigned long size)
{
const void *end;
@@ -285,6 +286,7 @@ static inline void clean_and_invalidate_dcache_va_range
for ( end = p + size; p < end; p += cacheline_bytes )
asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p));
dsb(sy); /* So we know the flushes happen before continuing */
+ return 0;
}
/* Macros for flushing a single small item. The predicate is always
--
1.7.10.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v8 3/8] xen/arm: return int *_dcache_va_range
2014-10-20 9:48 ` [PATCH v8 3/8] xen/arm: return int *_dcache_va_range Stefano Stabellini
@ 2014-10-20 14:57 ` Ian Campbell
2014-10-20 15:23 ` Jan Beulich
0 siblings, 1 reply; 31+ messages in thread
From: Ian Campbell @ 2014-10-20 14:57 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: julien.grall, xen-devel, JBeulich
On Mon, 2014-10-20 at 10:48 +0100, Stefano Stabellini wrote:
> These functions cannot really fail on ARM, but their x86 equivalents can
> (-EOPNOTSUPP). Change the prototype to return int.
The subject seems to be missing a word (from?), or is somehow garbled.
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Reviewed-by: Julien Grall <julien.grall@linaro.org>
>
> ---
>
> Changes in v6:
> - do not return int from flush_page_to_ram.
> ---
> xen/arch/arm/mm.c | 2 +-
> xen/include/asm-arm/page.h | 6 ++++--
> 2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index e43499a..8e989bf 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -388,7 +388,7 @@ void flush_page_to_ram(unsigned long mfn)
> {
> void *v = map_domain_page(mfn);
>
> - clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
> + ASSERT(clean_and_invalidate_dcache_va_range(v, PAGE_SIZE) == 0);
Just this one assert? What about all the other callers of
*_dcache_va_range?
> unmap_domain_page(v);
> }
>
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 1327b00..26c5856 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -268,16 +268,17 @@ extern size_t cacheline_bytes;
> /* Functions for flushing medium-sized areas.
> * if 'range' is large enough we might want to use model-specific
> * full-cache flushes. */
> -static inline void clean_dcache_va_range(const void *p, unsigned long size)
> +static inline int clean_dcache_va_range(const void *p, unsigned long size)
> {
> const void *end;
> dsb(sy); /* So the CPU issues all writes to the range */
> for ( end = p + size; p < end; p += cacheline_bytes )
> asm volatile (__clean_dcache_one(0) : : "r" (p));
> dsb(sy); /* So we know the flushes happen before continuing */
> + return 0;
> }
>
> -static inline void clean_and_invalidate_dcache_va_range
> +static inline int clean_and_invalidate_dcache_va_range
> (const void *p, unsigned long size)
> {
> const void *end;
> @@ -285,6 +286,7 @@ static inline void clean_and_invalidate_dcache_va_range
> for ( end = p + size; p < end; p += cacheline_bytes )
> asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p));
> dsb(sy); /* So we know the flushes happen before continuing */
> + return 0;
> }
>
> /* Macros for flushing a single small item. The predicate is always
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v8 3/8] xen/arm: return int *_dcache_va_range
2014-10-20 14:57 ` Ian Campbell
@ 2014-10-20 15:23 ` Jan Beulich
2014-10-20 15:27 ` Ian Campbell
2014-10-20 16:34 ` Stefano Stabellini
0 siblings, 2 replies; 31+ messages in thread
From: Jan Beulich @ 2014-10-20 15:23 UTC (permalink / raw)
To: Ian Campbell, Stefano Stabellini; +Cc: julien.grall, xen-devel
>>> On 20.10.14 at 16:57, <Ian.Campbell@citrix.com> wrote:
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -388,7 +388,7 @@ void flush_page_to_ram(unsigned long mfn)
>> {
>> void *v = map_domain_page(mfn);
>>
>> - clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
>> + ASSERT(clean_and_invalidate_dcache_va_range(v, PAGE_SIZE) == 0);
>
> Just this one assert? What about all the other callers of
> *_dcache_va_range?
I'm glad I looked at this reply (I skipped the original one because of
being ARM-only): Uses of ASSERT() like above won't work in non-
debug builds. You have to latch the result into a variable and assert
on it. The fundamental rule is: Expressions passed to ASSERT() must
never have side effects. That's different from BUG_ON(), but even
there Keir advocates not using expressions with side effects, aiui
both to avoid the above mistake (i.e. getting used to doing it the
right way) and to leave open the option of adding a build option to
eliminate the BUG()s and BUG_ON()s too.
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v8 3/8] xen/arm: return int *_dcache_va_range
2014-10-20 15:23 ` Jan Beulich
@ 2014-10-20 15:27 ` Ian Campbell
2014-10-20 16:34 ` Stefano Stabellini
1 sibling, 0 replies; 31+ messages in thread
From: Ian Campbell @ 2014-10-20 15:27 UTC (permalink / raw)
To: Jan Beulich; +Cc: julien.grall, xen-devel, Stefano Stabellini
On Mon, 2014-10-20 at 16:23 +0100, Jan Beulich wrote:
> >>> On 20.10.14 at 16:57, <Ian.Campbell@citrix.com> wrote:
> >> --- a/xen/arch/arm/mm.c
> >> +++ b/xen/arch/arm/mm.c
> >> @@ -388,7 +388,7 @@ void flush_page_to_ram(unsigned long mfn)
> >> {
> >> void *v = map_domain_page(mfn);
> >>
> >> - clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
> >> + ASSERT(clean_and_invalidate_dcache_va_range(v, PAGE_SIZE) == 0);
> >
> > Just this one assert? What about all the other callers of
> > *_dcache_va_range?
>
> I'm glad I looked at this reply (I skipped the original one because of
> being ARM-only): Uses of ASSERT() like above won't work in non-
> debug builds. You have to latch the result into a variable and assert
> on it. The fundamental rule is: Expressions passed to ASSERT() must
> never have side effects. That's different from BUG_ON(), but even
> there Keir advocates not using expressions with side effects, aiui
> both to avoid the above mistake (i.e. getting used to doing it the
> right way) and to leave open the option of adding a build option to
> eliminate the BUG()s and BUG_ON()s too.
Yes, I don't know how I missed this!
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v8 3/8] xen/arm: return int *_dcache_va_range
2014-10-20 15:23 ` Jan Beulich
2014-10-20 15:27 ` Ian Campbell
@ 2014-10-20 16:34 ` Stefano Stabellini
2014-10-21 9:07 ` Ian Campbell
1 sibling, 1 reply; 31+ messages in thread
From: Stefano Stabellini @ 2014-10-20 16:34 UTC (permalink / raw)
To: Jan Beulich; +Cc: julien.grall, xen-devel, Ian Campbell, Stefano Stabellini
On Mon, 20 Oct 2014, Jan Beulich wrote:
> >>> On 20.10.14 at 16:57, <Ian.Campbell@citrix.com> wrote:
> >> --- a/xen/arch/arm/mm.c
> >> +++ b/xen/arch/arm/mm.c
> >> @@ -388,7 +388,7 @@ void flush_page_to_ram(unsigned long mfn)
> >> {
> >> void *v = map_domain_page(mfn);
> >>
> >> - clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
> >> + ASSERT(clean_and_invalidate_dcache_va_range(v, PAGE_SIZE) == 0);
> >
> > Just this one assert? What about all the other callers of
> > *_dcache_va_range?
There are many many callers under xen/arch/arm, should I really add
ASSERT everywhere?
Maybe I could just add an ASSERT on the return value within
clean_dcache_va_range and clean_and_invalidate_dcache_va_range?
Or should I introduce wrappers with the ASSERT?
> I'm glad I looked at this reply (I skipped the original one because of
> being ARM-only): Uses of ASSERT() like above won't work in non-
> debug builds. You have to latch the result into a variable and assert
> on it. The fundamental rule is: Expressions passed to ASSERT() must
> never have side effects. That's different from BUG_ON(), but even
> there Keir advocates not using expressions with side effects, aiui
> both to avoid the above mistake (i.e. getting used to doing it the
> right way) and to leave open the option of adding a build option to
> eliminate the BUG()s and BUG_ON()s too.
Jan, well spotted, thanks!
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v8 3/8] xen/arm: return int *_dcache_va_range
2014-10-20 16:34 ` Stefano Stabellini
@ 2014-10-21 9:07 ` Ian Campbell
0 siblings, 0 replies; 31+ messages in thread
From: Ian Campbell @ 2014-10-21 9:07 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: julien.grall, xen-devel, Jan Beulich
On Mon, 2014-10-20 at 17:34 +0100, Stefano Stabellini wrote:
> On Mon, 20 Oct 2014, Jan Beulich wrote:
> > >>> On 20.10.14 at 16:57, <Ian.Campbell@citrix.com> wrote:
> > >> --- a/xen/arch/arm/mm.c
> > >> +++ b/xen/arch/arm/mm.c
> > >> @@ -388,7 +388,7 @@ void flush_page_to_ram(unsigned long mfn)
> > >> {
> > >> void *v = map_domain_page(mfn);
> > >>
> > >> - clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
> > >> + ASSERT(clean_and_invalidate_dcache_va_range(v, PAGE_SIZE) == 0);
> > >
> > > Just this one assert? What about all the other callers of
> > > *_dcache_va_range?
>
> There are many many callers under xen/arch/arm, should I really add
> ASSERT everywhere?
I dunno, but you should certainly be consistent, i.e. everywhere or
nowhere.
> Maybe I could just add an ASSERT on the return value within
> clean_dcache_va_range and clean_and_invalidate_dcache_va_range?
I don't think an ASSERT() is necessary, but a comment before the return
0 like:
/*
* These cannot fail and many callers from within the ARM
* architecture code do not check the result.
*/
That's assuming there's no possibility of us needing to propagate
failure in the future. Since we are talking about a h/w instruction with
no possibility of an error return I think we can assert (no pun
intended) that this won't happen.
> Or should I introduce wrappers with the ASSERT?
I don't think so.
Ian.
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v8 4/8] xen/arm: introduce invalidate_dcache_va_range
2014-10-20 9:47 [PATCH v8 0/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
` (2 preceding siblings ...)
2014-10-20 9:48 ` [PATCH v8 3/8] xen/arm: return int *_dcache_va_range Stefano Stabellini
@ 2014-10-20 9:48 ` Stefano Stabellini
2014-10-20 9:48 ` [PATCH v8 5/8] xen/x86: introduce more cache maintenance operations Stefano Stabellini
` (4 subsequent siblings)
8 siblings, 0 replies; 31+ messages in thread
From: Stefano Stabellini @ 2014-10-20 9:48 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, Ian.Campbell, JBeulich, Stefano Stabellini
Take care of handling non-cacheline aligned addresses and sizes.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes in v5:
- return int from invalidate_dcache_va_range.
Changes in v4:
- rename invalidate_xen_dcache_va_range to invalidate_dcache_va_range.
---
xen/include/asm-arm/arm32/page.h | 3 +++
xen/include/asm-arm/arm64/page.h | 3 +++
xen/include/asm-arm/page.h | 32 ++++++++++++++++++++++++++++++++
3 files changed, 38 insertions(+)
diff --git a/xen/include/asm-arm/arm32/page.h b/xen/include/asm-arm/arm32/page.h
index 20a6a7f..a07e217 100644
--- a/xen/include/asm-arm/arm32/page.h
+++ b/xen/include/asm-arm/arm32/page.h
@@ -19,6 +19,9 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
: : "r" (pte.bits), "r" (p) : "memory");
}
+/* Inline ASM to invalidate dcache on register R (may be an inline asm operand) */
+#define __invalidate_dcache_one(R) STORE_CP32(R, DCIMVAC)
+
/* Inline ASM to flush dcache on register R (may be an inline asm operand) */
#define __clean_dcache_one(R) STORE_CP32(R, DCCMVAC)
diff --git a/xen/include/asm-arm/arm64/page.h b/xen/include/asm-arm/arm64/page.h
index 101d7a8..1fd416d 100644
--- a/xen/include/asm-arm/arm64/page.h
+++ b/xen/include/asm-arm/arm64/page.h
@@ -14,6 +14,9 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
: : "r" (pte.bits), "r" (p) : "memory");
}
+/* Inline ASM to invalidate dcache on register R (may be an inline asm operand) */
+#define __invalidate_dcache_one(R) "dc ivac, %" #R ";"
+
/* Inline ASM to flush dcache on register R (may be an inline asm operand) */
#define __clean_dcache_one(R) "dc cvac, %" #R ";"
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 26c5856..fdcc173 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -268,6 +268,38 @@ extern size_t cacheline_bytes;
/* Functions for flushing medium-sized areas.
* if 'range' is large enough we might want to use model-specific
* full-cache flushes. */
+
+static inline int invalidate_dcache_va_range(const void *p, unsigned long size)
+{
+ size_t off;
+ const void *end = p + size;
+
+ dsb(sy); /* So the CPU issues all writes to the range */
+
+ off = (unsigned long)p % cacheline_bytes;
+ if ( off )
+ {
+ p -= off;
+ asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (p));
+ p += cacheline_bytes;
+ size -= cacheline_bytes - off;
+ }
+ off = (unsigned long)end % cacheline_bytes;
+ if ( off )
+ {
+ end -= off;
+ size -= off;
+ asm volatile (__clean_and_invalidate_dcache_one(0) : : "r" (end));
+ }
+
+ for ( ; p < end; p += cacheline_bytes )
+ asm volatile (__invalidate_dcache_one(0) : : "r" (p));
+
+ dsb(sy); /* So we know the flushes happen before continuing */
+
+ return 0;
+}
+
static inline int clean_dcache_va_range(const void *p, unsigned long size)
{
const void *end;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH v8 5/8] xen/x86: introduce more cache maintenance operations
2014-10-20 9:47 [PATCH v8 0/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
` (3 preceding siblings ...)
2014-10-20 9:48 ` [PATCH v8 4/8] xen/arm: introduce invalidate_dcache_va_range Stefano Stabellini
@ 2014-10-20 9:48 ` Stefano Stabellini
2014-10-20 9:48 ` [PATCH v8 6/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
` (3 subsequent siblings)
8 siblings, 0 replies; 31+ messages in thread
From: Stefano Stabellini @ 2014-10-20 9:48 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, Ian.Campbell, JBeulich, Stefano Stabellini
Move the existing flush_page_to_ram flushtlb.h.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v7:
- return the called function's return value from clean_dcache_va_range.
Changes in v5:
- make order an unsigned int;
- add a comment on sub-page granularity support;
- cache operations return error;
- move the functions to xen/include/asm-x86/flushtlb.h.
Changes in v4:
- remove _xen in the function names;
- implement the functions using existing x86 flushing functions.
---
xen/include/asm-x86/flushtlb.h | 15 +++++++++++++++
xen/include/asm-x86/page.h | 3 ---
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/xen/include/asm-x86/flushtlb.h b/xen/include/asm-x86/flushtlb.h
index 7f46632..85e260a 100644
--- a/xen/include/asm-x86/flushtlb.h
+++ b/xen/include/asm-x86/flushtlb.h
@@ -11,6 +11,7 @@
#define __FLUSHTLB_H__
#include <xen/config.h>
+#include <xen/mm.h>
#include <xen/percpu.h>
#include <xen/smp.h>
#include <xen/types.h>
@@ -115,4 +116,18 @@ void flush_area_mask(const cpumask_t *, const void *va, unsigned int flags);
#define flush_tlb_one_all(v) \
flush_tlb_one_mask(&cpu_online_map, v)
+static inline void flush_page_to_ram(unsigned long mfn) {}
+static inline int invalidate_dcache_va_range(const void *p, unsigned long size) { return -EOPNOTSUPP; }
+static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size)
+{
+ unsigned int order = get_order_from_bytes(size);
+ /* sub-page granularity support needs to be added if necessary */
+ flush_area_local(p, FLUSH_CACHE|FLUSH_ORDER(order));
+ return 0;
+}
+static inline int clean_dcache_va_range(const void *p, unsigned long size)
+{
+ return clean_and_invalidate_dcache_va_range(p, size);
+}
+
#endif /* __FLUSHTLB_H__ */
diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index 9aa780e..a8bc999 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -344,9 +344,6 @@ static inline uint32_t cacheattr_to_pte_flags(uint32_t cacheattr)
return ((cacheattr & 4) << 5) | ((cacheattr & 3) << 3);
}
-/* No cache maintenance required on x86 architecture. */
-static inline void flush_page_to_ram(unsigned long mfn) {}
-
/* return true if permission increased */
static inline bool_t
perms_strictly_increased(uint32_t old_flags, uint32_t new_flags)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH v8 6/8] xen/arm: introduce GNTTABOP_cache_flush
2014-10-20 9:47 [PATCH v8 0/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
` (4 preceding siblings ...)
2014-10-20 9:48 ` [PATCH v8 5/8] xen/x86: introduce more cache maintenance operations Stefano Stabellini
@ 2014-10-20 9:48 ` Stefano Stabellini
2014-10-20 10:58 ` Jan Beulich
` (2 more replies)
2014-10-20 9:48 ` [PATCH v8 7/8] Revert "xen/arm: introduce XENFEAT_grant_map_identity" Stefano Stabellini
` (2 subsequent siblings)
8 siblings, 3 replies; 31+ messages in thread
From: Stefano Stabellini @ 2014-10-20 9:48 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, Ian.Campbell, JBeulich, Stefano Stabellini
Introduce a new hypercall to perform cache maintenance operation on
behalf of the guest. The argument is a machine address and a size. The
implementation checks that the memory range is owned by the guest or the
guest has been granted access to it by another domain.
Introduce grant_map_exists: an internal grant table function to check
whether an mfn has been granted to a given domain on a target grant
table. Check hypercall_preempt_check() every 4096 iterations in the
implementation of grant_map_exists.
Use the top 20 bits of the GNTTABOP cmd encoding to save the last ref
across the hypercall continuation.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
Changes in v8:
- avoid security issues, use two separate opaque variables to store the
input argument and the output argument;
- fix return values of grant_map_exists;
- rename CMD_MASK to GNTTABOP_CMD_MASK;
- rename OPAQUE_CONTINUATION_ARG_SHIFT to
GNTTABOP_CONTINUATION_ARG_SHIFT;
- save in the opaque argument the shifted ref_count;
- set GNTTABOP_CONTINUATION_ARG_SHIFT to 20 and
MAX_GRANT_ENTRIES_ITER_SHIFT to 12, to cover the full grant_ref_t value
range;
- move GNTTABOP_CONTINUATION_ARG_SHIFT and GNTTABOP_CMD_MASK to
include/xen/grant_table.h;
- cmd &= GNTTABOP_CMD_MASK in the compat wrapper.
Changes in v7:
- remove warning message;
- prefix second line of the warning with XENLOG_WARNING;
- do not lower DEFAULT_MAX_NR_GRANT_FRAMES;
- no long lines;
- interrupt loops in grant_map_exists with more than 2048 iterations,
create an hypercall continuation if necessary.
Changes in v6:
- set DEFAULT_MAX_NR_GRANT_FRAMES to 10;
- warn if max_grant_frames > 10.
Changes in v5:
- make mfn mfn unsigned long;
- remove unhelpful error message;
- handle errors returned by cache maintenance functions.
Changes in v4:
- ASSERT(spin_is_locked);
- return instead of break in grant_map_exists;
- pass a pointer to __gnttab_cache_flush;
- code style;
- unsigned int iterator in gnttab_cache_flush;
- return ret instead -ret;
- cflush.offset >= PAGE_SIZE return -EINVAL.
Changes in v3:
- reduce the time the grant_table lock is held;
- fix warning message;
- s/EFAULT/EPERM;
- s/llx/PRIx64;
- check offset and size independetly before checking their sum;
- rcu_lock_current_domain cannot fail;
- s/ENOSYS/EOPNOTSUPP;
- use clean_and_invalidate_xen_dcache_va_range to do both operations at
once;
- fold grant_map_exists in this patch;
- support "count" argument;
- make correspondent changes to compat/grant_table.c;
- introduce GNTTAB_CACHE_SOURCE_GREF to select the type of input in the
union;
- rename size field to length;
- make length and offset uint16_t;
- only take spin_lock if d != owner.
Changes in v2:
- do not check for mfn_to_page errors;
- take a reference to the page;
- replace printk with gdprintk;
- split long line;
- remove out label;
- move rcu_lock_current_domain down before the loop.
- move the hypercall to GNTTABOP;
- take a spin_lock before calling grant_map_exists.
---
xen/common/compat/grant_table.c | 15 +++-
xen/common/grant_table.c | 166 +++++++++++++++++++++++++++++++++++++-
xen/include/public/grant_table.h | 21 +++++
xen/include/xen/grant_table.h | 3 +
xen/include/xlat.lst | 1 +
5 files changed, 200 insertions(+), 6 deletions(-)
diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index 2dc1e44..0368289 100644
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -51,16 +51,21 @@ CHECK_gnttab_get_version;
CHECK_gnttab_swap_grant_ref;
#undef xen_gnttab_swap_grant_ref
+#define xen_gnttab_cache_flush gnttab_cache_flush
+CHECK_gnttab_cache_flush;
+#undef xen_gnttab_cache_flush
+
int compat_grant_table_op(unsigned int cmd,
XEN_GUEST_HANDLE_PARAM(void) cmp_uop,
unsigned int count)
{
int rc = 0;
- unsigned int i;
+ unsigned int i, cmd_op;
XEN_GUEST_HANDLE_PARAM(void) cnt_uop;
set_xen_guest_handle(cnt_uop, NULL);
- switch ( cmd )
+ cmd_op = cmd & GNTTABOP_CMD_MASK;
+ switch ( cmd_op )
{
#define CASE(name) \
case GNTTABOP_##name: \
@@ -106,6 +111,10 @@ int compat_grant_table_op(unsigned int cmd,
CASE(swap_grant_ref);
#endif
+#ifndef CHECK_gnttab_cache_flush
+ CASE(cache_flush);
+#endif
+
#undef CASE
default:
return do_grant_table_op(cmd, cmp_uop, count);
@@ -132,7 +141,7 @@ int compat_grant_table_op(unsigned int cmd,
} cmp;
set_xen_guest_handle(nat.uop, COMPAT_ARG_XLAT_VIRT_BASE);
- switch ( cmd )
+ switch ( cmd_op )
{
case GNTTABOP_setup_table:
if ( unlikely(count > 1) )
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 784bbc6..c56ad5e 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -488,6 +488,43 @@ static int _set_status(unsigned gt_version,
return _set_status_v2(domid, readonly, mapflag, shah, act, status);
}
+#define MAX_GRANT_ENTRIES_ITER_SHIFT 12
+static int grant_map_exists(const struct domain *ld,
+ struct grant_table *rgt,
+ unsigned long mfn,
+ grant_ref_t *ref_count)
+{
+ const struct active_grant_entry *act;
+ grant_ref_t ref;
+
+ ASSERT(spin_is_locked(&rgt->lock));
+
+ for ( ref = *ref_count; ref < nr_grant_entries(rgt); ref++ )
+ {
+ if ( (ref % (1 << MAX_GRANT_ENTRIES_ITER_SHIFT)) == 0 &&
+ ref != *ref_count )
+ {
+ *ref_count = ref;
+ return 1;
+ }
+
+ act = &active_entry(rgt, ref);
+
+ if ( !act->pin )
+ continue;
+
+ if ( act->domid != ld->domain_id )
+ continue;
+
+ if ( act->frame != mfn )
+ continue;
+
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
static void mapcount(
struct grant_table *lgt, struct domain *rd, unsigned long mfn,
unsigned int *wrc, unsigned int *rdc)
@@ -2486,16 +2523,119 @@ gnttab_swap_grant_ref(XEN_GUEST_HANDLE_PARAM(gnttab_swap_grant_ref_t) uop,
return 0;
}
+static int __gnttab_cache_flush(gnttab_cache_flush_t *cflush,
+ grant_ref_t *ref_count)
+{
+ struct domain *d, *owner;
+ struct page_info *page;
+ unsigned long mfn;
+ void *v;
+ int ret = 0;
+
+ if ( (cflush->offset >= PAGE_SIZE) ||
+ (cflush->length > PAGE_SIZE) ||
+ (cflush->offset + cflush->length > PAGE_SIZE) )
+ return -EINVAL;
+
+ if ( cflush->length == 0 || cflush->op == 0 )
+ return 0;
+
+ /* currently unimplemented */
+ if ( cflush->op & GNTTAB_CACHE_SOURCE_GREF )
+ return -EOPNOTSUPP;
+
+ d = rcu_lock_current_domain();
+ mfn = cflush->a.dev_bus_addr >> PAGE_SHIFT;
+
+ if ( !mfn_valid(mfn) )
+ {
+ rcu_unlock_domain(d);
+ return -EINVAL;
+ }
+
+ page = mfn_to_page(mfn);
+ owner = page_get_owner_and_reference(page);
+ if ( !owner )
+ {
+ rcu_unlock_domain(d);
+ return -EPERM;
+ }
+
+ if ( d != owner )
+ {
+ spin_lock(&owner->grant_table->lock);
+
+ ret = grant_map_exists(d, owner->grant_table, mfn, ref_count);
+ if ( ret != 0 )
+ {
+ spin_unlock(&owner->grant_table->lock);
+ rcu_unlock_domain(d);
+ put_page(page);
+ return ret;
+ }
+ }
+
+ v = map_domain_page(mfn);
+ v += cflush->offset;
+
+ if ( (cflush->op & GNTTAB_CACHE_INVAL) && (cflush->op & GNTTAB_CACHE_CLEAN) )
+ ret = clean_and_invalidate_dcache_va_range(v, cflush->length);
+ else if ( cflush->op & GNTTAB_CACHE_INVAL )
+ ret = invalidate_dcache_va_range(v, cflush->length);
+ else if ( cflush->op & GNTTAB_CACHE_CLEAN )
+ ret = clean_dcache_va_range(v, cflush->length);
+
+ if ( d != owner )
+ spin_unlock(&owner->grant_table->lock);
+ unmap_domain_page(v);
+ put_page(page);
+
+ return ret;
+}
+
+static long
+gnttab_cache_flush(XEN_GUEST_HANDLE_PARAM(gnttab_cache_flush_t) uop,
+ grant_ref_t *ref_count,
+ unsigned int count)
+{
+ int ret = 0;
+ unsigned int i;
+ gnttab_cache_flush_t op;
+
+ for ( i = 0; i < count; i++ )
+ {
+ if ( i && hypercall_preempt_check() )
+ return i;
+ if ( unlikely(__copy_from_guest(&op, uop, 1)) )
+ return -EFAULT;
+ do {
+ ret = __gnttab_cache_flush(&op, ref_count);
+ if ( ret < 0 )
+ return ret;
+ if ( ret > 0 && hypercall_preempt_check() )
+ return i;
+ } while ( ret > 0 );
+ *ref_count = 0;
+ guest_handle_add_offset(uop, 1);
+ }
+ return 0;
+}
+
+
long
do_grant_table_op(
unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
{
long rc;
+ unsigned int opaque_in = 0, opaque_out = 0;
if ( (int)count < 0 )
return -EINVAL;
rc = -EFAULT;
+
+ opaque_in = cmd >> GNTTABOP_CONTINUATION_ARG_SHIFT;
+ cmd &= GNTTABOP_CMD_MASK;
switch ( cmd )
{
case GNTTABOP_map_grant_ref:
@@ -2615,17 +2755,37 @@ do_grant_table_op(
}
break;
}
+ case GNTTABOP_cache_flush:
+ {
+ grant_ref_t ref_count = opaque_in << MAX_GRANT_ENTRIES_ITER_SHIFT;
+ XEN_GUEST_HANDLE_PARAM(gnttab_cache_flush_t) cflush =
+ guest_handle_cast(uop, gnttab_cache_flush_t);
+ if ( unlikely(!guest_handle_okay(cflush, count)) )
+ goto out;
+ rc = gnttab_cache_flush(cflush, &ref_count, count);
+ if ( rc > 0 )
+ {
+ guest_handle_add_offset(cflush, rc);
+ uop = guest_handle_cast(cflush, void);
+ }
+ opaque_out = ref_count >> MAX_GRANT_ENTRIES_ITER_SHIFT;
+ break;
+ }
default:
rc = -ENOSYS;
break;
}
out:
- if ( rc > 0 )
+ if ( rc > 0 || opaque_out != 0 )
{
ASSERT(rc < count);
- rc = hypercall_create_continuation(__HYPERVISOR_grant_table_op,
- "ihi", cmd, uop, count - rc);
+ ASSERT(opaque_out <
+ (1 << (sizeof(cmd)*8 - GNTTABOP_CONTINUATION_ARG_SHIFT)));
+ rc = hypercall_create_continuation(__HYPERVISOR_grant_table_op, "ihi",
+ (opaque_out <<
+ GNTTABOP_CONTINUATION_ARG_SHIFT) | cmd,
+ uop, count - rc);
}
return rc;
diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h
index b8a3d6c..2dfb5a2 100644
--- a/xen/include/public/grant_table.h
+++ b/xen/include/public/grant_table.h
@@ -309,6 +309,8 @@ typedef uint16_t grant_status_t;
#define GNTTABOP_get_status_frames 9
#define GNTTABOP_get_version 10
#define GNTTABOP_swap_grant_ref 11
+#define GNTTABOP_cache_flush 12
+/* max GNTTABOP is 4095 */
#endif /* __XEN_INTERFACE_VERSION__ */
/* ` } */
@@ -574,6 +576,25 @@ struct gnttab_swap_grant_ref {
typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t;
DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t);
+/*
+ * Issue one or more cache maintenance operations on a portion of a
+ * page granted to the calling domain by a foreign domain.
+ */
+struct gnttab_cache_flush {
+ union {
+ uint64_t dev_bus_addr;
+ grant_ref_t ref;
+ } a;
+ uint16_t offset; /* offset from start of grant */
+ uint16_t length; /* size within the grant */
+#define GNTTAB_CACHE_CLEAN (1<<0)
+#define GNTTAB_CACHE_INVAL (1<<1)
+#define GNTTAB_CACHE_SOURCE_GREF (1<<31)
+ uint32_t op;
+};
+typedef struct gnttab_cache_flush gnttab_cache_flush_t;
+DEFINE_XEN_GUEST_HANDLE(gnttab_cache_flush_t);
+
#endif /* __XEN_INTERFACE_VERSION__ */
/*
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 32f5786..0b80be7 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -127,4 +127,7 @@ static inline unsigned int grant_to_status_frames(int grant_frames)
GRANT_STATUS_PER_PAGE;
}
+#define GNTTABOP_CONTINUATION_ARG_SHIFT 20
+#define GNTTABOP_CMD_MASK ((1<<GNTTABOP_CONTINUATION_ARG_SHIFT)-1)
+
#endif /* __XEN_GRANT_TABLE_H__ */
diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
index 234b668..9ce9fee 100644
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -51,6 +51,7 @@
? grant_entry_header grant_table.h
? grant_entry_v2 grant_table.h
? gnttab_swap_grant_ref grant_table.h
+? gnttab_cache_flush grant_table.h
? kexec_exec kexec.h
! kexec_image kexec.h
! kexec_range kexec.h
--
1.7.10.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v8 6/8] xen/arm: introduce GNTTABOP_cache_flush
2014-10-20 9:48 ` [PATCH v8 6/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
@ 2014-10-20 10:58 ` Jan Beulich
2014-10-20 15:00 ` Ian Campbell
2014-10-20 15:24 ` Ian Campbell
2 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2014-10-20 10:58 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: julien.grall, xen-devel, Ian.Campbell
>>> On 20.10.14 at 11:48, <stefano.stabellini@eu.citrix.com> wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -488,6 +488,43 @@ static int _set_status(unsigned gt_version,
> return _set_status_v2(domid, readonly, mapflag, shah, act, status);
> }
>
> +#define MAX_GRANT_ENTRIES_ITER_SHIFT 12
> +static int grant_map_exists(const struct domain *ld,
> + struct grant_table *rgt,
> + unsigned long mfn,
> + grant_ref_t *ref_count)
> +{
> + const struct active_grant_entry *act;
> + grant_ref_t ref;
> +
> + ASSERT(spin_is_locked(&rgt->lock));
> +
> + for ( ref = *ref_count; ref < nr_grant_entries(rgt); ref++ )
> + {
> + if ( (ref % (1 << MAX_GRANT_ENTRIES_ITER_SHIFT)) == 0 &&
Wouldn't it be cheaper (not needing calculations on each
iteration) to determine the maximum loop exit condition up front
as min(*ref_count + (1 << MAX_GRANT_ENTRIES_ITER_SHIFT),
nr_grant_entries(rgt))?
> long
> do_grant_table_op(
> unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
> {
> long rc;
> + unsigned int opaque_in = 0, opaque_out = 0;
>
> if ( (int)count < 0 )
> return -EINVAL;
>
> rc = -EFAULT;
> +
> + opaque_in = cmd >> GNTTABOP_CONTINUATION_ARG_SHIFT;
This and MAX_GRANT_ENTRIES_ITER_SHIFT need to be identical
(and you really don't need that second manifest constant). With
what you do right now, you shift out 20 bits here, but handle only
12 low bits per iteration (i.e. a total of 24 instead of 32). And
hence you don't need to shift here (only mask), ...
> @@ -2615,17 +2755,37 @@ do_grant_table_op(
> }
> break;
> }
> + case GNTTABOP_cache_flush:
> + {
> + grant_ref_t ref_count = opaque_in << MAX_GRANT_ENTRIES_ITER_SHIFT;
... and neither here ...
> + XEN_GUEST_HANDLE_PARAM(gnttab_cache_flush_t) cflush =
> + guest_handle_cast(uop, gnttab_cache_flush_t);
> + if ( unlikely(!guest_handle_okay(cflush, count)) )
> + goto out;
> + rc = gnttab_cache_flush(cflush, &ref_count, count);
> + if ( rc > 0 )
> + {
> + guest_handle_add_offset(cflush, rc);
> + uop = guest_handle_cast(cflush, void);
> + }
> + opaque_out = ref_count >> MAX_GRANT_ENTRIES_ITER_SHIFT;
... nor here ...
> + break;
> + }
> default:
> rc = -ENOSYS;
> break;
> }
>
> out:
> - if ( rc > 0 )
> + if ( rc > 0 || opaque_out != 0 )
> {
> ASSERT(rc < count);
> - rc = hypercall_create_continuation(__HYPERVISOR_grant_table_op,
> - "ihi", cmd, uop, count - rc);
> + ASSERT(opaque_out <
> + (1 << (sizeof(cmd)*8 - GNTTABOP_CONTINUATION_ARG_SHIFT)));
> + rc = hypercall_create_continuation(__HYPERVISOR_grant_table_op, "ihi",
> + (opaque_out <<
> + GNTTABOP_CONTINUATION_ARG_SHIFT) | cmd,
... nor here (but asserting that the low bits are clear is still desirable).
> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -127,4 +127,7 @@ static inline unsigned int grant_to_status_frames(int grant_frames)
> GRANT_STATUS_PER_PAGE;
> }
>
> +#define GNTTABOP_CONTINUATION_ARG_SHIFT 20
> +#define GNTTABOP_CMD_MASK ((1<<GNTTABOP_CONTINUATION_ARG_SHIFT)-1)
Such internal items should be given as little exposure as possible.
In the case here they can be confined to grant_table.c (since
the compat wrapper code gets included in that file). With "this
needs to be moved" (as said in response to v7) I didn't mean to a
header, I meant only inside the file (to its top), as being not
specific to just some particular function.
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v8 6/8] xen/arm: introduce GNTTABOP_cache_flush
2014-10-20 9:48 ` [PATCH v8 6/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
2014-10-20 10:58 ` Jan Beulich
@ 2014-10-20 15:00 ` Ian Campbell
2014-10-20 15:24 ` Ian Campbell
2 siblings, 0 replies; 31+ messages in thread
From: Ian Campbell @ 2014-10-20 15:00 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: julien.grall, xen-devel, JBeulich
On Mon, 2014-10-20 at 10:48 +0100, Stefano Stabellini wrote:
Despite the title I don't think this is arm specific any more, is it? I
trust Jan's judgement more than my own on this area of code so I intend
to defer to him unless someone suggests I do otherwise.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8 6/8] xen/arm: introduce GNTTABOP_cache_flush
2014-10-20 9:48 ` [PATCH v8 6/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
2014-10-20 10:58 ` Jan Beulich
2014-10-20 15:00 ` Ian Campbell
@ 2014-10-20 15:24 ` Ian Campbell
2014-10-20 15:30 ` Jan Beulich
2 siblings, 1 reply; 31+ messages in thread
From: Ian Campbell @ 2014-10-20 15:24 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: julien.grall, xen-devel, JBeulich
On Mon, 2014-10-20 at 10:48 +0100, Stefano Stabellini wrote:
>
> @@ -574,6 +576,25 @@ struct gnttab_swap_grant_ref {
> typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t;
> DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t);
>
> +/*
> + * Issue one or more cache maintenance operations on a portion of a
> + * page granted to the calling domain by a foreign domain.
> + */
> +struct gnttab_cache_flush {
> + union {
> + uint64_t dev_bus_addr;
> + grant_ref_t ref;
> + } a;
> + uint16_t offset; /* offset from start of grant */
> + uint16_t length; /* size within the grant */
So are these not valid when used with dev_bus_addr? I can see that being
the case for offset, but length too?
> +#define GNTTAB_CACHE_CLEAN (1<<0)
> +#define GNTTAB_CACHE_INVAL (1<<1)
> +#define GNTTAB_CACHE_SOURCE_GREF (1<<31)
> + uint32_t op;
> +};
> +typedef struct gnttab_cache_flush gnttab_cache_flush_t;
> +DEFINE_XEN_GUEST_HANDLE(gnttab_cache_flush_t);
> +
> #endif /* __XEN_INTERFACE_VERSION__ */
>
> /*
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v8 6/8] xen/arm: introduce GNTTABOP_cache_flush
2014-10-20 15:24 ` Ian Campbell
@ 2014-10-20 15:30 ` Jan Beulich
2014-10-20 16:31 ` Stefano Stabellini
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2014-10-20 15:30 UTC (permalink / raw)
To: Ian Campbell, Stefano Stabellini; +Cc: julien.grall, xen-devel
>>> On 20.10.14 at 17:24, <Ian.Campbell@citrix.com> wrote:
> On Mon, 2014-10-20 at 10:48 +0100, Stefano Stabellini wrote:
>>
>> @@ -574,6 +576,25 @@ struct gnttab_swap_grant_ref {
>> typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t;
>> DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t);
>>
>> +/*
>> + * Issue one or more cache maintenance operations on a portion of a
>> + * page granted to the calling domain by a foreign domain.
>> + */
>> +struct gnttab_cache_flush {
>> + union {
>> + uint64_t dev_bus_addr;
>> + grant_ref_t ref;
>> + } a;
>> + uint16_t offset; /* offset from start of grant */
>> + uint16_t length; /* size within the grant */
>
> So are these not valid when used with dev_bus_addr? I can see that being
> the case for offset, but length too?
Both ought to be valid, and dev_bus_addr should be page aligned.
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v8 6/8] xen/arm: introduce GNTTABOP_cache_flush
2014-10-20 15:30 ` Jan Beulich
@ 2014-10-20 16:31 ` Stefano Stabellini
2014-10-21 9:04 ` Ian Campbell
0 siblings, 1 reply; 31+ messages in thread
From: Stefano Stabellini @ 2014-10-20 16:31 UTC (permalink / raw)
To: Jan Beulich; +Cc: julien.grall, xen-devel, Ian Campbell, Stefano Stabellini
On Mon, 20 Oct 2014, Jan Beulich wrote:
> >>> On 20.10.14 at 17:24, <Ian.Campbell@citrix.com> wrote:
> > On Mon, 2014-10-20 at 10:48 +0100, Stefano Stabellini wrote:
> >>
> >> @@ -574,6 +576,25 @@ struct gnttab_swap_grant_ref {
> >> typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t;
> >> DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t);
> >>
> >> +/*
> >> + * Issue one or more cache maintenance operations on a portion of a
> >> + * page granted to the calling domain by a foreign domain.
> >> + */
> >> +struct gnttab_cache_flush {
> >> + union {
> >> + uint64_t dev_bus_addr;
> >> + grant_ref_t ref;
> >> + } a;
> >> + uint16_t offset; /* offset from start of grant */
> >> + uint16_t length; /* size within the grant */
> >
> > So are these not valid when used with dev_bus_addr? I can see that being
> > the case for offset, but length too?
>
> Both ought to be valid, and dev_bus_addr should be page aligned.
That's right.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v8 6/8] xen/arm: introduce GNTTABOP_cache_flush
2014-10-20 16:31 ` Stefano Stabellini
@ 2014-10-21 9:04 ` Ian Campbell
2014-10-21 9:14 ` Jan Beulich
0 siblings, 1 reply; 31+ messages in thread
From: Ian Campbell @ 2014-10-21 9:04 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: julien.grall, xen-devel, Jan Beulich
On Mon, 2014-10-20 at 17:31 +0100, Stefano Stabellini wrote:
> On Mon, 20 Oct 2014, Jan Beulich wrote:
> > >>> On 20.10.14 at 17:24, <Ian.Campbell@citrix.com> wrote:
> > > On Mon, 2014-10-20 at 10:48 +0100, Stefano Stabellini wrote:
> > >>
> > >> @@ -574,6 +576,25 @@ struct gnttab_swap_grant_ref {
> > >> typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t;
> > >> DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t);
> > >>
> > >> +/*
> > >> + * Issue one or more cache maintenance operations on a portion of a
> > >> + * page granted to the calling domain by a foreign domain.
> > >> + */
> > >> +struct gnttab_cache_flush {
> > >> + union {
> > >> + uint64_t dev_bus_addr;
> > >> + grant_ref_t ref;
> > >> + } a;
> > >> + uint16_t offset; /* offset from start of grant */
> > >> + uint16_t length; /* size within the grant */
> > >
> > > So are these not valid when used with dev_bus_addr? I can see that being
> > > the case for offset, but length too?
> >
> > Both ought to be valid, and dev_bus_addr should be page aligned.
>
> That's right.
I would have expected you to correct the comment in the repost then
(which I notice also still says arm in the subject)
Ian.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v8 6/8] xen/arm: introduce GNTTABOP_cache_flush
2014-10-21 9:04 ` Ian Campbell
@ 2014-10-21 9:14 ` Jan Beulich
2014-10-21 9:35 ` Ian Campbell
0 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2014-10-21 9:14 UTC (permalink / raw)
To: Ian Campbell; +Cc: julien.grall, xen-devel, Stefano Stabellini
>>> On 21.10.14 at 11:04, <Ian.Campbell@citrix.com> wrote:
> On Mon, 2014-10-20 at 17:31 +0100, Stefano Stabellini wrote:
>> On Mon, 20 Oct 2014, Jan Beulich wrote:
>> > >>> On 20.10.14 at 17:24, <Ian.Campbell@citrix.com> wrote:
>> > > On Mon, 2014-10-20 at 10:48 +0100, Stefano Stabellini wrote:
>> > >>
>> > >> @@ -574,6 +576,25 @@ struct gnttab_swap_grant_ref {
>> > >> typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t;
>> > >> DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t);
>> > >>
>> > >> +/*
>> > >> + * Issue one or more cache maintenance operations on a portion of a
>> > >> + * page granted to the calling domain by a foreign domain.
>> > >> + */
>> > >> +struct gnttab_cache_flush {
>> > >> + union {
>> > >> + uint64_t dev_bus_addr;
>> > >> + grant_ref_t ref;
>> > >> + } a;
>> > >> + uint16_t offset; /* offset from start of grant */
>> > >> + uint16_t length; /* size within the grant */
>> > >
>> > > So are these not valid when used with dev_bus_addr? I can see that being
>> > > the case for offset, but length too?
>> >
>> > Both ought to be valid, and dev_bus_addr should be page aligned.
>>
>> That's right.
>
> I would have expected you to correct the comment in the repost then
> (which I notice also still says arm in the subject)
I have to admit I'm having trouble seeing which of the comments
you think is wrong/misleading.
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v8 6/8] xen/arm: introduce GNTTABOP_cache_flush
2014-10-21 9:14 ` Jan Beulich
@ 2014-10-21 9:35 ` Ian Campbell
2014-10-21 9:39 ` Stefano Stabellini
2014-10-21 14:33 ` Jan Beulich
0 siblings, 2 replies; 31+ messages in thread
From: Ian Campbell @ 2014-10-21 9:35 UTC (permalink / raw)
To: Jan Beulich; +Cc: julien.grall, xen-devel, Stefano Stabellini
On Tue, 2014-10-21 at 10:14 +0100, Jan Beulich wrote:
> >>> On 21.10.14 at 11:04, <Ian.Campbell@citrix.com> wrote:
> > On Mon, 2014-10-20 at 17:31 +0100, Stefano Stabellini wrote:
> >> On Mon, 20 Oct 2014, Jan Beulich wrote:
> >> > >>> On 20.10.14 at 17:24, <Ian.Campbell@citrix.com> wrote:
> >> > > On Mon, 2014-10-20 at 10:48 +0100, Stefano Stabellini wrote:
> >> > >>
> >> > >> @@ -574,6 +576,25 @@ struct gnttab_swap_grant_ref {
> >> > >> typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t;
> >> > >> DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t);
> >> > >>
> >> > >> +/*
> >> > >> + * Issue one or more cache maintenance operations on a portion of a
> >> > >> + * page granted to the calling domain by a foreign domain.
> >> > >> + */
> >> > >> +struct gnttab_cache_flush {
> >> > >> + union {
> >> > >> + uint64_t dev_bus_addr;
> >> > >> + grant_ref_t ref;
> >> > >> + } a;
> >> > >> + uint16_t offset; /* offset from start of grant */
> >> > >> + uint16_t length; /* size within the grant */
> >> > >
> >> > > So are these not valid when used with dev_bus_addr? I can see that being
> >> > > the case for offset, but length too?
> >> >
> >> > Both ought to be valid, and dev_bus_addr should be page aligned.
> >>
> >> That's right.
> >
> > I would have expected you to correct the comment in the repost then
> > (which I notice also still says arm in the subject)
>
> I have to admit I'm having trouble seeing which of the comments
> you think is wrong/misleading.
start of grant/size within the grant, in the context of using
dev_bus_addr, seemed strange/inconsistent.
But you made me think again and of course dev_bus_addr is required to
refer to a granted page isn't it. So I retract that comment.
Is it not worth being explicit about dev_bus_addr being page aligned and
equal to something previously returned by a map operation?
Ian.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v8 6/8] xen/arm: introduce GNTTABOP_cache_flush
2014-10-21 9:35 ` Ian Campbell
@ 2014-10-21 9:39 ` Stefano Stabellini
2014-10-21 14:33 ` Jan Beulich
1 sibling, 0 replies; 31+ messages in thread
From: Stefano Stabellini @ 2014-10-21 9:39 UTC (permalink / raw)
To: Ian Campbell; +Cc: julien.grall, xen-devel, Jan Beulich, Stefano Stabellini
On Tue, 21 Oct 2014, Ian Campbell wrote:
> On Tue, 2014-10-21 at 10:14 +0100, Jan Beulich wrote:
> > >>> On 21.10.14 at 11:04, <Ian.Campbell@citrix.com> wrote:
> > > On Mon, 2014-10-20 at 17:31 +0100, Stefano Stabellini wrote:
> > >> On Mon, 20 Oct 2014, Jan Beulich wrote:
> > >> > >>> On 20.10.14 at 17:24, <Ian.Campbell@citrix.com> wrote:
> > >> > > On Mon, 2014-10-20 at 10:48 +0100, Stefano Stabellini wrote:
> > >> > >>
> > >> > >> @@ -574,6 +576,25 @@ struct gnttab_swap_grant_ref {
> > >> > >> typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t;
> > >> > >> DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t);
> > >> > >>
> > >> > >> +/*
> > >> > >> + * Issue one or more cache maintenance operations on a portion of a
> > >> > >> + * page granted to the calling domain by a foreign domain.
> > >> > >> + */
> > >> > >> +struct gnttab_cache_flush {
> > >> > >> + union {
> > >> > >> + uint64_t dev_bus_addr;
> > >> > >> + grant_ref_t ref;
> > >> > >> + } a;
> > >> > >> + uint16_t offset; /* offset from start of grant */
> > >> > >> + uint16_t length; /* size within the grant */
> > >> > >
> > >> > > So are these not valid when used with dev_bus_addr? I can see that being
> > >> > > the case for offset, but length too?
> > >> >
> > >> > Both ought to be valid, and dev_bus_addr should be page aligned.
> > >>
> > >> That's right.
> > >
> > > I would have expected you to correct the comment in the repost then
> > > (which I notice also still says arm in the subject)
> >
> > I have to admit I'm having trouble seeing which of the comments
> > you think is wrong/misleading.
>
> start of grant/size within the grant, in the context of using
> dev_bus_addr, seemed strange/inconsistent.
>
> But you made me think again and of course dev_bus_addr is required to
> refer to a granted page isn't it. So I retract that comment.
>
> Is it not worth being explicit about dev_bus_addr being page aligned and
> equal to something previously returned by a map operation?
I can add a comment about it
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v8 6/8] xen/arm: introduce GNTTABOP_cache_flush
2014-10-21 9:35 ` Ian Campbell
2014-10-21 9:39 ` Stefano Stabellini
@ 2014-10-21 14:33 ` Jan Beulich
1 sibling, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2014-10-21 14:33 UTC (permalink / raw)
To: Ian Campbell; +Cc: julien.grall, xen-devel, Stefano Stabellini
>>> On 21.10.14 at 11:35, <Ian.Campbell@citrix.com> wrote:
> On Tue, 2014-10-21 at 10:14 +0100, Jan Beulich wrote:
>> >>> On 21.10.14 at 11:04, <Ian.Campbell@citrix.com> wrote:
>> > On Mon, 2014-10-20 at 17:31 +0100, Stefano Stabellini wrote:
>> >> On Mon, 20 Oct 2014, Jan Beulich wrote:
>> >> > >>> On 20.10.14 at 17:24, <Ian.Campbell@citrix.com> wrote:
>> >> > > On Mon, 2014-10-20 at 10:48 +0100, Stefano Stabellini wrote:
>> >> > >>
>> >> > >> @@ -574,6 +576,25 @@ struct gnttab_swap_grant_ref {
>> >> > >> typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t;
>> >> > >> DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t);
>> >> > >>
>> >> > >> +/*
>> >> > >> + * Issue one or more cache maintenance operations on a portion of a
>> >> > >> + * page granted to the calling domain by a foreign domain.
>> >> > >> + */
>> >> > >> +struct gnttab_cache_flush {
>> >> > >> + union {
>> >> > >> + uint64_t dev_bus_addr;
>> >> > >> + grant_ref_t ref;
>> >> > >> + } a;
>> >> > >> + uint16_t offset; /* offset from start of grant */
>> >> > >> + uint16_t length; /* size within the grant */
>> >> > >
>> >> > > So are these not valid when used with dev_bus_addr? I can see that being
>> >> > > the case for offset, but length too?
>> >> >
>> >> > Both ought to be valid, and dev_bus_addr should be page aligned.
>> >>
>> >> That's right.
>> >
>> > I would have expected you to correct the comment in the repost then
>> > (which I notice also still says arm in the subject)
>>
>> I have to admit I'm having trouble seeing which of the comments
>> you think is wrong/misleading.
>
> start of grant/size within the grant, in the context of using
> dev_bus_addr, seemed strange/inconsistent.
>
> But you made me think again and of course dev_bus_addr is required to
> refer to a granted page isn't it. So I retract that comment.
>
> Is it not worth being explicit about dev_bus_addr being page aligned and
> equal to something previously returned by a map operation?
Maybe, but that would then apply to pre-existing structures/fields
too.
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v8 7/8] Revert "xen/arm: introduce XENFEAT_grant_map_identity"
2014-10-20 9:47 [PATCH v8 0/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
` (5 preceding siblings ...)
2014-10-20 9:48 ` [PATCH v8 6/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
@ 2014-10-20 9:48 ` Stefano Stabellini
2014-10-20 15:00 ` Ian Campbell
2014-10-20 9:48 ` [PATCH v8 8/8] Revert "xen: introduce arch_grant_(un)map_page_identity" Stefano Stabellini
2014-10-20 10:26 ` [PATCH v8 0/8] xen/arm: introduce GNTTABOP_cache_flush Jan Beulich
8 siblings, 1 reply; 31+ messages in thread
From: Stefano Stabellini @ 2014-10-20 9:48 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, Ian.Campbell, JBeulich, Stefano Stabellini
Revert commit id 8d09ef6906ca0a9957e21334ad2c3eed626abe63.
Just keep the definition of XENFEAT_grant_map_identity.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
Changes in v2:
- comment out the definition of XENFEAT_grant_map_identity.
---
xen/common/grant_table.c | 30 +++++-------------------------
xen/common/kernel.c | 2 --
xen/drivers/passthrough/arm/smmu.c | 33 +++++++++++++++++++++++++++++++++
xen/include/asm-arm/grant_table.h | 3 ++-
xen/include/public/features.h | 4 +++-
5 files changed, 43 insertions(+), 29 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index c56ad5e..d53f627 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -779,23 +779,13 @@ __gnttab_map_grant_ref(
!(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
{
if ( wrc == 0 )
- {
- if ( is_domain_direct_mapped(ld) )
- err = arch_grant_map_page_identity(ld, frame, 1);
- else
- err = iommu_map_page(ld, frame, frame,
- IOMMUF_readable|IOMMUF_writable);
- }
+ err = iommu_map_page(ld, frame, frame,
+ IOMMUF_readable|IOMMUF_writable);
}
else if ( act_pin && !old_pin )
{
if ( (wrc + rdc) == 0 )
- {
- if ( is_domain_direct_mapped(ld) )
- err = arch_grant_map_page_identity(ld, frame, 0);
- else
- err = iommu_map_page(ld, frame, frame, IOMMUF_readable);
- }
+ err = iommu_map_page(ld, frame, frame, IOMMUF_readable);
}
if ( err )
{
@@ -992,19 +982,9 @@ __gnttab_unmap_common(
int err = 0;
mapcount(lgt, rd, op->frame, &wrc, &rdc);
if ( (wrc + rdc) == 0 )
- {
- if ( is_domain_direct_mapped(ld) )
- err = arch_grant_unmap_page_identity(ld, op->frame);
- else
- err = iommu_unmap_page(ld, op->frame);
- }
+ err = iommu_unmap_page(ld, op->frame);
else if ( wrc == 0 )
- {
- if ( is_domain_direct_mapped(ld) )
- err = arch_grant_map_page_identity(ld, op->frame, 0);
- else
- err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
- }
+ err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
if ( err )
{
rc = GNTST_general_error;
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index ce65486..d23c422 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -332,8 +332,6 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
break;
}
#endif
- if ( is_domain_direct_mapped(d) )
- fi.submap |= 1U << XENFEAT_grant_map_identity;
break;
default:
return -EINVAL;
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 3cbd206..9a95ac9 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -1536,6 +1536,37 @@ static void arm_smmu_iommu_domain_teardown(struct domain *d)
xfree(smmu_domain);
}
+static int arm_smmu_map_page(struct domain *d, unsigned long gfn,
+ unsigned long mfn, unsigned int flags)
+{
+ /* Grant mappings can be used for DMA requests. The dev_bus_addr returned by
+ * the hypercall is the MFN (not the IPA). For device protected by
+ * an IOMMU, Xen needs to add a 1:1 mapping in the domain p2m to
+ * allow DMA request to work.
+ * This is only valid when the domain is directed mapped. Hence this
+ * function should only be used by gnttab code with gfn == mfn.
+ */
+ BUG_ON(!is_domain_direct_mapped(d));
+ BUG_ON(mfn != gfn);
+
+ /* We only support readable and writable flags */
+ if ( !(flags & (IOMMUF_readable | IOMMUF_writable)) )
+ return -EINVAL;
+
+ return arch_grant_map_page_identity(d, mfn, flags & IOMMUF_writable);
+}
+
+static int arm_smmu_unmap_page(struct domain *d, unsigned long gfn)
+{
+ /* This function should only be used by gnttab code when the domain
+ * is direct mapped
+ */
+ if ( !is_domain_direct_mapped(d) )
+ return -EINVAL;
+
+ return arch_grant_unmap_page_identity(d, gfn);
+}
+
static const struct iommu_ops arm_smmu_iommu_ops = {
.init = arm_smmu_iommu_domain_init,
.hwdom_init = arm_smmu_iommu_hwdom_init,
@@ -1544,6 +1575,8 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
.iotlb_flush_all = arm_smmu_iotlb_flush_all,
.assign_dt_device = arm_smmu_attach_dev,
.reassign_dt_device = arm_smmu_reassign_dt_dev,
+ .map_page = arm_smmu_map_page,
+ .unmap_page = arm_smmu_unmap_page,
};
static int __init smmu_init(struct dt_device_node *dev,
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index e798880..0edad67 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -33,7 +33,8 @@ static inline int replace_grant_supported(void)
( ((i >= nr_grant_frames(d->grant_table)) && \
(i < max_grant_frames)) ? 0 : (d->arch.grant_table_gpfn[i]))
-#define gnttab_need_iommu_mapping(d) (is_domain_direct_mapped(d))
+#define gnttab_need_iommu_mapping(d) \
+ (is_domain_direct_mapped(d) && need_iommu(d))
#endif /* __ASM_GRANT_TABLE_H__ */
/*
diff --git a/xen/include/public/features.h b/xen/include/public/features.h
index b7bf83f..16d92aa 100644
--- a/xen/include/public/features.h
+++ b/xen/include/public/features.h
@@ -94,8 +94,10 @@
/* operation as Dom0 is supported */
#define XENFEAT_dom0 11
-/* Xen also maps grant references at pfn = mfn */
+/* Xen also maps grant references at pfn = mfn.
+ * This feature flag is deprecated and should not be used.
#define XENFEAT_grant_map_identity 12
+ */
#define XENFEAT_NR_SUBMAPS 1
--
1.7.10.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* [PATCH v8 8/8] Revert "xen: introduce arch_grant_(un)map_page_identity"
2014-10-20 9:47 [PATCH v8 0/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
` (6 preceding siblings ...)
2014-10-20 9:48 ` [PATCH v8 7/8] Revert "xen/arm: introduce XENFEAT_grant_map_identity" Stefano Stabellini
@ 2014-10-20 9:48 ` Stefano Stabellini
2014-10-20 15:01 ` Ian Campbell
2014-10-20 10:26 ` [PATCH v8 0/8] xen/arm: introduce GNTTABOP_cache_flush Jan Beulich
8 siblings, 1 reply; 31+ messages in thread
From: Stefano Stabellini @ 2014-10-20 9:48 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, Ian.Campbell, JBeulich, Stefano Stabellini
This reverts commit e25a5f4d8cf3b55718048abdd21c7d0de64ae54c.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
xen/arch/arm/p2m.c | 26 --------------------------
xen/drivers/passthrough/arm/smmu.c | 13 +++++++++++--
xen/include/asm-arm/p2m.h | 4 ----
xen/include/asm-x86/p2m.h | 4 ----
4 files changed, 11 insertions(+), 36 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 20bcc9e..6b7569a 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -982,32 +982,6 @@ void guest_physmap_remove_page(struct domain *d,
pfn_to_paddr(mfn), MATTR_MEM, p2m_invalid);
}
-int arch_grant_map_page_identity(struct domain *d, unsigned long frame,
- bool_t writeable)
-{
- p2m_type_t t;
-
- ASSERT(is_domain_direct_mapped(d));
-
- /* This is not an IOMMU mapping but it is not a regular RAM p2m type
- * either. We are using IOMMU p2m types here to prevent the pages
- * from being used as grants. */
- if ( writeable )
- t = p2m_iommu_map_rw;
- else
- t = p2m_iommu_map_ro;
-
- return guest_physmap_add_entry(d, frame, frame, 0, t);
-}
-
-int arch_grant_unmap_page_identity(struct domain *d, unsigned long frame)
-{
- ASSERT(is_domain_direct_mapped(d));
-
- guest_physmap_remove_page(d, frame, frame, 0);
- return 0;
-}
-
int p2m_alloc_table(struct domain *d)
{
struct p2m_domain *p2m = &d->arch.p2m;
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 9a95ac9..42bde75 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -1539,6 +1539,8 @@ static void arm_smmu_iommu_domain_teardown(struct domain *d)
static int arm_smmu_map_page(struct domain *d, unsigned long gfn,
unsigned long mfn, unsigned int flags)
{
+ p2m_type_t t;
+
/* Grant mappings can be used for DMA requests. The dev_bus_addr returned by
* the hypercall is the MFN (not the IPA). For device protected by
* an IOMMU, Xen needs to add a 1:1 mapping in the domain p2m to
@@ -1553,7 +1555,12 @@ static int arm_smmu_map_page(struct domain *d, unsigned long gfn,
if ( !(flags & (IOMMUF_readable | IOMMUF_writable)) )
return -EINVAL;
- return arch_grant_map_page_identity(d, mfn, flags & IOMMUF_writable);
+ t = (flags & IOMMUF_writable) ? p2m_iommu_map_rw : p2m_iommu_map_ro;
+
+ /* The function guest_physmap_add_entry replaces the current mapping
+ * if there is already one...
+ */
+ return guest_physmap_add_entry(d, gfn, mfn, 0, t);
}
static int arm_smmu_unmap_page(struct domain *d, unsigned long gfn)
@@ -1564,7 +1571,9 @@ static int arm_smmu_unmap_page(struct domain *d, unsigned long gfn)
if ( !is_domain_direct_mapped(d) )
return -EINVAL;
- return arch_grant_unmap_page_identity(d, gfn);
+ guest_physmap_remove_page(d, gfn, gfn, 0);
+
+ return 0;
}
static const struct iommu_ops arm_smmu_iommu_ops = {
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 10bf111..da36504 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -214,10 +214,6 @@ static inline int get_page_and_type(struct page_info *page,
return rc;
}
-int arch_grant_map_page_identity(struct domain *d, unsigned long frame,
- bool_t writeable);
-int arch_grant_unmap_page_identity(struct domain *d, unsigned long frame);
-
/* get host p2m table */
#define p2m_get_hostp2m(d) (&(d)->arch.p2m)
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 90ddd15..5f7fe71 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -709,10 +709,6 @@ static inline unsigned int p2m_get_iommu_flags(p2m_type_t p2mt)
return flags;
}
-extern int arch_grant_map_page_identity(struct domain *d, unsigned long frame,
- bool_t writeable);
-extern int arch_grant_unmap_page_identity(struct domain *d, unsigned long frame);
-
#endif /* _XEN_P2M_H */
/*
--
1.7.10.4
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v8 0/8] xen/arm: introduce GNTTABOP_cache_flush
2014-10-20 9:47 [PATCH v8 0/8] xen/arm: introduce GNTTABOP_cache_flush Stefano Stabellini
` (7 preceding siblings ...)
2014-10-20 9:48 ` [PATCH v8 8/8] Revert "xen: introduce arch_grant_(un)map_page_identity" Stefano Stabellini
@ 2014-10-20 10:26 ` Jan Beulich
2014-10-20 10:36 ` Stefano Stabellini
8 siblings, 1 reply; 31+ messages in thread
From: Jan Beulich @ 2014-10-20 10:26 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Julien Grall, xen-devel, Ian Campbell
>>> On 20.10.14 at 11:47, <stefano.stabellini@eu.citrix.com> wrote:
> Hi all,
> this patch series introduces a new hypercall to perform cache
> maintenance operations on behalf of the guest. It is useful for dom0 to
> be able to cache flush pages involved in a dma operation with
> non-coherent devices.
>
> It also removes XENFEAT_grant_map_identity as the feature is no longer
> necessary: it was used to achieve the same goal but the guest can now
> use the hypercall instead. Keeping the flag would also have a
> significant performance impact as a new p2m mapping gets created and
> then destroyed for every grant that is mapped and unmapped in dom0.
>
>
> Changes in v8:
> - remove unused #include;
> - set max_nr_grant_frames as __initdata;
> - set max_grant_frames and max_maptrack_frames as __read_mostly;
> - respent coding style for comments;
> - remove MAX_MAPTRACK_TO_GRANTS_RATIO;
> - avoid security issues, use two separate opaque variables to store the
> input argument and the output argument;
Security issues?
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v8 0/8] xen/arm: introduce GNTTABOP_cache_flush
2014-10-20 10:26 ` [PATCH v8 0/8] xen/arm: introduce GNTTABOP_cache_flush Jan Beulich
@ 2014-10-20 10:36 ` Stefano Stabellini
2014-10-20 11:00 ` Jan Beulich
0 siblings, 1 reply; 31+ messages in thread
From: Stefano Stabellini @ 2014-10-20 10:36 UTC (permalink / raw)
To: Jan Beulich; +Cc: Julien Grall, xen-devel, Ian Campbell, Stefano Stabellini
On Mon, 20 Oct 2014, Jan Beulich wrote:
> >>> On 20.10.14 at 11:47, <stefano.stabellini@eu.citrix.com> wrote:
> > Hi all,
> > this patch series introduces a new hypercall to perform cache
> > maintenance operations on behalf of the guest. It is useful for dom0 to
> > be able to cache flush pages involved in a dma operation with
> > non-coherent devices.
> >
> > It also removes XENFEAT_grant_map_identity as the feature is no longer
> > necessary: it was used to achieve the same goal but the guest can now
> > use the hypercall instead. Keeping the flag would also have a
> > significant performance impact as a new p2m mapping gets created and
> > then destroyed for every grant that is mapped and unmapped in dom0.
> >
> >
> > Changes in v8:
> > - remove unused #include;
> > - set max_nr_grant_frames as __initdata;
> > - set max_grant_frames and max_maptrack_frames as __read_mostly;
> > - respent coding style for comments;
> > - remove MAX_MAPTRACK_TO_GRANTS_RATIO;
> > - avoid security issues, use two separate opaque variables to store the
> > input argument and the output argument;
>
> Security issues?
The user could have passed in an opaque value for a gnttabop that
doesn't use it, causing Xen to keep looping over the continuation.
I fixed it by using two different variable to save the opaque input and
output.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v8 0/8] xen/arm: introduce GNTTABOP_cache_flush
2014-10-20 10:36 ` Stefano Stabellini
@ 2014-10-20 11:00 ` Jan Beulich
0 siblings, 0 replies; 31+ messages in thread
From: Jan Beulich @ 2014-10-20 11:00 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Julien Grall, xen-devel, Ian Campbell
>>> On 20.10.14 at 12:36, <stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 20 Oct 2014, Jan Beulich wrote:
>> >>> On 20.10.14 at 11:47, <stefano.stabellini@eu.citrix.com> wrote:
>> > Hi all,
>> > this patch series introduces a new hypercall to perform cache
>> > maintenance operations on behalf of the guest. It is useful for dom0 to
>> > be able to cache flush pages involved in a dma operation with
>> > non-coherent devices.
>> >
>> > It also removes XENFEAT_grant_map_identity as the feature is no longer
>> > necessary: it was used to achieve the same goal but the guest can now
>> > use the hypercall instead. Keeping the flag would also have a
>> > significant performance impact as a new p2m mapping gets created and
>> > then destroyed for every grant that is mapped and unmapped in dom0.
>> >
>> >
>> > Changes in v8:
>> > - remove unused #include;
>> > - set max_nr_grant_frames as __initdata;
>> > - set max_grant_frames and max_maptrack_frames as __read_mostly;
>> > - respent coding style for comments;
>> > - remove MAX_MAPTRACK_TO_GRANTS_RATIO;
>> > - avoid security issues, use two separate opaque variables to store the
>> > input argument and the output argument;
>>
>> Security issues?
>
> The user could have passed in an opaque value for a gnttabop that
> doesn't use it, causing Xen to keep looping over the continuation.
> I fixed it by using two different variable to save the opaque input and
> output.
Hmm, but that's not really a security issue, since the preemption
in between guarantees Xen's health. All it would end up being is a
guest vCPU looping indefinitely.
Jan
^ permalink raw reply [flat|nested] 31+ messages in thread