* [PATCH V13 1/7] xen/arm: p2m changes for mem_access support
2015-03-06 21:24 [PATCH V13 0/7] Mem_access for ARM Tamas K Lengyel
@ 2015-03-06 21:24 ` Tamas K Lengyel
2015-03-11 16:07 ` Stefano Stabellini
` (2 more replies)
2015-03-06 21:24 ` [PATCH V13 2/7] xen/arm: Implement domain_get_maximum_gpfn Tamas K Lengyel
` (6 subsequent siblings)
7 siblings, 3 replies; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-06 21:24 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson,
julien.grall, tim, stefano.stabellini, jbeulich, keir,
Tamas K Lengyel
Add necessary changes for page table construction routines to pass
the default access information. We store the p2m_access_t info in a
Radix tree as the PTE lacks enough software programmable bits.
Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
---
v13: - Rename access_in_use to mem_access_enabled.
- Define p2m_get_mem_access function prototype but
return -ENOSYS for now.
v11: - Move including common/mem_event.h down the series.
v10: - Typo fix and drop reshuffling things that no longer need
shuffling.
v8: - Drop lock inputs as common mem_access_check is postponed.
- Resurrect the radix tree with an extra boolean access_in_use flag
to indicate if the tree is empty to avoid lookups.
v7: - Remove radix tree init/destroy and move p2m_access_t store to page_info.
- Add p2m_gpfn_lock/unlock functions.
- Add bool_t lock input to p2m_lookup and apply_p2m_changes so the caller
can specify if locking should be performed. This is needed in order to
support mem_access_check from common.
v6: - Move mem_event header include to first patch that needs it.
v5: - #include grouping style-fix.
v4: - Move p2m_get_hostp2m definition here.
---
xen/arch/arm/p2m.c | 52 +++++++++++++++++++++++++++--------------
xen/include/asm-arm/domain.h | 1 +
xen/include/asm-arm/p2m.h | 35 ++++++++++++++++++++++++++-
xen/include/asm-arm/processor.h | 2 +-
4 files changed, 70 insertions(+), 20 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 8809f5a..137e5a0 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -305,7 +305,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a)
}
static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
- p2m_type_t t)
+ p2m_type_t t, p2m_access_t a)
{
paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
/* sh, xn and write bit will be defined in the following switches
@@ -335,8 +335,7 @@ static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
break;
}
- /* We pass p2m_access_rwx as a placeholder for now. */
- p2m_set_permission(&e, t, p2m_access_rwx);
+ p2m_set_permission(&e, t, a);
ASSERT(!(pa & ~PAGE_MASK));
ASSERT(!(pa & ~PADDR_MASK));
@@ -394,7 +393,7 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
for ( i=0 ; i < LPAE_ENTRIES; i++ )
{
pte = mfn_to_p2m_entry(base_pfn + (i<<(level_shift-LPAE_SHIFT)),
- MATTR_MEM, t);
+ MATTR_MEM, t, p2m->default_access);
/*
* First and second level super pages set p2m.table = 0, but
@@ -414,7 +413,8 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
unmap_domain_page(p);
- pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, p2m_invalid);
+ pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, p2m_invalid,
+ p2m->default_access);
p2m_write_pte(entry, pte, flush_cache);
@@ -537,7 +537,8 @@ static int apply_one_level(struct domain *d,
paddr_t *maddr,
bool_t *flush,
int mattr,
- p2m_type_t t)
+ p2m_type_t t,
+ p2m_access_t a)
{
const paddr_t level_size = level_sizes[level];
const paddr_t level_mask = level_masks[level];
@@ -566,7 +567,7 @@ static int apply_one_level(struct domain *d,
page = alloc_domheap_pages(d, level_shift - PAGE_SHIFT, 0);
if ( page )
{
- pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t);
+ pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t, a);
if ( level < 3 )
pte.p2m.table = 0;
p2m_write_pte(entry, pte, flush_cache);
@@ -601,7 +602,7 @@ static int apply_one_level(struct domain *d,
(level == 3 || !p2m_table(orig_pte)) )
{
/* New mapping is superpage aligned, make it */
- pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT, mattr, t);
+ pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT, mattr, t, a);
if ( level < 3 )
pte.p2m.table = 0; /* Superpage entry */
@@ -770,7 +771,9 @@ static int apply_p2m_changes(struct domain *d,
paddr_t end_gpaddr,
paddr_t maddr,
int mattr,
- p2m_type_t t)
+ uint32_t mask,
+ p2m_type_t t,
+ p2m_access_t a)
{
int rc, ret;
struct p2m_domain *p2m = &d->arch.p2m;
@@ -863,7 +866,7 @@ static int apply_p2m_changes(struct domain *d,
level, flush_pt, op,
start_gpaddr, end_gpaddr,
&addr, &maddr, &flush,
- mattr, t);
+ mattr, t, a);
if ( ret < 0 ) { rc = ret ; goto out; }
count += ret;
/* L3 had better have done something! We cannot descend any further */
@@ -921,7 +924,7 @@ out:
*/
apply_p2m_changes(d, REMOVE,
start_gpaddr, addr + level_sizes[level], orig_maddr,
- mattr, p2m_invalid);
+ mattr, 0, p2m_invalid, d->arch.p2m.default_access);
}
for ( level = P2M_ROOT_LEVEL; level < 4; level ++ )
@@ -940,7 +943,8 @@ int p2m_populate_ram(struct domain *d,
paddr_t end)
{
return apply_p2m_changes(d, ALLOCATE, start, end,
- 0, MATTR_MEM, p2m_ram_rw);
+ 0, MATTR_MEM, 0, p2m_ram_rw,
+ d->arch.p2m.default_access);
}
int map_mmio_regions(struct domain *d,
@@ -952,7 +956,8 @@ int map_mmio_regions(struct domain *d,
pfn_to_paddr(start_gfn),
pfn_to_paddr(start_gfn + nr),
pfn_to_paddr(mfn),
- MATTR_DEV, p2m_mmio_direct);
+ MATTR_DEV, 0, p2m_mmio_direct,
+ d->arch.p2m.default_access);
}
int unmap_mmio_regions(struct domain *d,
@@ -964,7 +969,8 @@ int unmap_mmio_regions(struct domain *d,
pfn_to_paddr(start_gfn),
pfn_to_paddr(start_gfn + nr),
pfn_to_paddr(mfn),
- MATTR_DEV, p2m_invalid);
+ MATTR_DEV, 0, p2m_invalid,
+ d->arch.p2m.default_access);
}
int guest_physmap_add_entry(struct domain *d,
@@ -976,7 +982,8 @@ int guest_physmap_add_entry(struct domain *d,
return apply_p2m_changes(d, INSERT,
pfn_to_paddr(gpfn),
pfn_to_paddr(gpfn + (1 << page_order)),
- pfn_to_paddr(mfn), MATTR_MEM, t);
+ pfn_to_paddr(mfn), MATTR_MEM, 0, t,
+ d->arch.p2m.default_access);
}
void guest_physmap_remove_page(struct domain *d,
@@ -986,7 +993,8 @@ void guest_physmap_remove_page(struct domain *d,
apply_p2m_changes(d, REMOVE,
pfn_to_paddr(gpfn),
pfn_to_paddr(gpfn + (1<<page_order)),
- pfn_to_paddr(mfn), MATTR_MEM, p2m_invalid);
+ pfn_to_paddr(mfn), MATTR_MEM, 0, p2m_invalid,
+ d->arch.p2m.default_access);
}
int p2m_alloc_table(struct domain *d)
@@ -1090,6 +1098,8 @@ void p2m_teardown(struct domain *d)
p2m_free_vmid(d);
+ radix_tree_destroy(&p2m->mem_access_settings, NULL);
+
spin_unlock(&p2m->lock);
}
@@ -1115,6 +1125,10 @@ int p2m_init(struct domain *d)
p2m->max_mapped_gfn = 0;
p2m->lowest_mapped_gfn = ULONG_MAX;
+ p2m->default_access = p2m_access_rwx;
+ p2m->mem_access_enabled = false;
+ radix_tree_init(&p2m->mem_access_settings);
+
err:
spin_unlock(&p2m->lock);
@@ -1129,7 +1143,8 @@ int relinquish_p2m_mapping(struct domain *d)
pfn_to_paddr(p2m->lowest_mapped_gfn),
pfn_to_paddr(p2m->max_mapped_gfn),
pfn_to_paddr(INVALID_MFN),
- MATTR_MEM, p2m_invalid);
+ MATTR_MEM, 0, p2m_invalid,
+ d->arch.p2m.default_access);
}
int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn)
@@ -1143,7 +1158,8 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn)
pfn_to_paddr(start_mfn),
pfn_to_paddr(end_mfn),
pfn_to_paddr(INVALID_MFN),
- MATTR_MEM, p2m_invalid);
+ MATTR_MEM, 0, p2m_invalid,
+ d->arch.p2m.default_access);
}
unsigned long gmfn_to_mfn(struct domain *d, unsigned long gpfn)
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 9e0419e..f1a087e 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -17,6 +17,7 @@ struct hvm_domain
{
uint64_t params[HVM_NR_PARAMS];
struct hvm_iommu iommu;
+ bool_t introspection_enabled;
} __cacheline_aligned;
#ifdef CONFIG_ARM_64
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index da36504..7583d9b 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -2,8 +2,9 @@
#define _XEN_P2M_H
#include <xen/mm.h>
-
+#include <xen/radix-tree.h>
#include <xen/p2m-common.h>
+#include <public/memory.h>
#define paddr_bits PADDR_BITS
@@ -48,6 +49,18 @@ struct p2m_domain {
/* If true, and an access fault comes in and there is no mem_event listener,
* pause domain. Otherwise, remove access restrictions. */
bool_t access_required;
+
+ /* Defines if mem_access is in use for the domain. */
+ bool_t mem_access_enabled;
+
+ /* Default P2M access type for each page in the the domain: new pages,
+ * swapped in pages, cleared pages, and pages that are ambiguously
+ * retyped get this access type. See definition of p2m_access_t. */
+ p2m_access_t default_access;
+
+ /* Radix tree to store the p2m_access_t settings as the pte's don't have
+ * enough available bits to store this information. */
+ struct radix_tree_root mem_access_settings;
};
/* List of possible type for each page in the p2m entry.
@@ -217,6 +230,26 @@ static inline int get_page_and_type(struct page_info *page,
/* get host p2m table */
#define p2m_get_hostp2m(d) (&(d)->arch.p2m)
+/* mem_event and mem_access are supported on any ARM guest */
+static inline bool_t p2m_mem_access_sanity_check(struct domain *d)
+{
+ return 1;
+}
+
+static inline bool_t p2m_mem_event_sanity_check(struct domain *d)
+{
+ return 1;
+}
+
+/* Get access type for a pfn
+ * If pfn == -1ul, gets the default access type */
+static inline
+int p2m_get_mem_access(struct domain *d, unsigned long pfn,
+ xenmem_access_t *access)
+{
+ return -ENOSYS;
+}
+
#endif /* _XEN_P2M_H */
/*
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index fcd26fb..cf7ab7c 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -441,7 +441,7 @@ union hsr {
struct hsr_dabt {
unsigned long dfsc:6; /* Data Fault Status Code */
unsigned long write:1; /* Write / not Read */
- unsigned long s1ptw:1; /* */
+ unsigned long s1ptw:1; /* Stage 2 fault during stage 1 translation */
unsigned long cache:1; /* Cache Maintenance */
unsigned long eat:1; /* External Abort Type */
#ifdef CONFIG_ARM_32
--
2.1.4
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [PATCH V13 1/7] xen/arm: p2m changes for mem_access support
2015-03-06 21:24 ` [PATCH V13 1/7] xen/arm: p2m changes for mem_access support Tamas K Lengyel
@ 2015-03-11 16:07 ` Stefano Stabellini
2015-03-11 17:05 ` Tamas K Lengyel
2015-03-12 11:27 ` Ian Campbell
2015-03-12 12:57 ` Julien Grall
2 siblings, 1 reply; 71+ messages in thread
From: Stefano Stabellini @ 2015-03-11 16:07 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, ian.campbell, stefano.stabellini, tim, julien.grall,
ian.jackson, xen-devel, stefano.stabellini, jbeulich, keir
On Fri, 6 Mar 2015, Tamas K Lengyel wrote:
> Add necessary changes for page table construction routines to pass
> the default access information. We store the p2m_access_t info in a
> Radix tree as the PTE lacks enough software programmable bits.
>
> Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
> ---
> v13: - Rename access_in_use to mem_access_enabled.
> - Define p2m_get_mem_access function prototype but
> return -ENOSYS for now.
> v11: - Move including common/mem_event.h down the series.
> v10: - Typo fix and drop reshuffling things that no longer need
> shuffling.
> v8: - Drop lock inputs as common mem_access_check is postponed.
> - Resurrect the radix tree with an extra boolean access_in_use flag
> to indicate if the tree is empty to avoid lookups.
> v7: - Remove radix tree init/destroy and move p2m_access_t store to page_info.
> - Add p2m_gpfn_lock/unlock functions.
> - Add bool_t lock input to p2m_lookup and apply_p2m_changes so the caller
> can specify if locking should be performed. This is needed in order to
> support mem_access_check from common.
> v6: - Move mem_event header include to first patch that needs it.
> v5: - #include grouping style-fix.
> v4: - Move p2m_get_hostp2m definition here.
> ---
> xen/arch/arm/p2m.c | 52 +++++++++++++++++++++++++++--------------
> xen/include/asm-arm/domain.h | 1 +
> xen/include/asm-arm/p2m.h | 35 ++++++++++++++++++++++++++-
> xen/include/asm-arm/processor.h | 2 +-
> 4 files changed, 70 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 8809f5a..137e5a0 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -305,7 +305,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a)
> }
>
> static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
> - p2m_type_t t)
> + p2m_type_t t, p2m_access_t a)
> {
> paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
> /* sh, xn and write bit will be defined in the following switches
> @@ -335,8 +335,7 @@ static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
> break;
> }
>
> - /* We pass p2m_access_rwx as a placeholder for now. */
> - p2m_set_permission(&e, t, p2m_access_rwx);
> + p2m_set_permission(&e, t, a);
>
> ASSERT(!(pa & ~PAGE_MASK));
> ASSERT(!(pa & ~PADDR_MASK));
> @@ -394,7 +393,7 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
> for ( i=0 ; i < LPAE_ENTRIES; i++ )
> {
> pte = mfn_to_p2m_entry(base_pfn + (i<<(level_shift-LPAE_SHIFT)),
> - MATTR_MEM, t);
> + MATTR_MEM, t, p2m->default_access);
>
> /*
> * First and second level super pages set p2m.table = 0, but
> @@ -414,7 +413,8 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
>
> unmap_domain_page(p);
>
> - pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, p2m_invalid);
> + pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, p2m_invalid,
> + p2m->default_access);
>
> p2m_write_pte(entry, pte, flush_cache);
>
> @@ -537,7 +537,8 @@ static int apply_one_level(struct domain *d,
> paddr_t *maddr,
> bool_t *flush,
> int mattr,
> - p2m_type_t t)
> + p2m_type_t t,
> + p2m_access_t a)
> {
> const paddr_t level_size = level_sizes[level];
> const paddr_t level_mask = level_masks[level];
> @@ -566,7 +567,7 @@ static int apply_one_level(struct domain *d,
> page = alloc_domheap_pages(d, level_shift - PAGE_SHIFT, 0);
> if ( page )
> {
> - pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t);
> + pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t, a);
> if ( level < 3 )
> pte.p2m.table = 0;
> p2m_write_pte(entry, pte, flush_cache);
> @@ -601,7 +602,7 @@ static int apply_one_level(struct domain *d,
> (level == 3 || !p2m_table(orig_pte)) )
> {
> /* New mapping is superpage aligned, make it */
> - pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT, mattr, t);
> + pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT, mattr, t, a);
> if ( level < 3 )
> pte.p2m.table = 0; /* Superpage entry */
>
> @@ -770,7 +771,9 @@ static int apply_p2m_changes(struct domain *d,
> paddr_t end_gpaddr,
> paddr_t maddr,
> int mattr,
> - p2m_type_t t)
> + uint32_t mask,
What is this for? It is not used and you always pass 0 here and in a
later patch ~0.
> + p2m_type_t t,
> + p2m_access_t a)
> {
> int rc, ret;
> struct p2m_domain *p2m = &d->arch.p2m;
> @@ -863,7 +866,7 @@ static int apply_p2m_changes(struct domain *d,
> level, flush_pt, op,
> start_gpaddr, end_gpaddr,
> &addr, &maddr, &flush,
> - mattr, t);
> + mattr, t, a);
> if ( ret < 0 ) { rc = ret ; goto out; }
> count += ret;
> /* L3 had better have done something! We cannot descend any further */
> @@ -921,7 +924,7 @@ out:
> */
> apply_p2m_changes(d, REMOVE,
> start_gpaddr, addr + level_sizes[level], orig_maddr,
> - mattr, p2m_invalid);
> + mattr, 0, p2m_invalid, d->arch.p2m.default_access);
> }
>
> for ( level = P2M_ROOT_LEVEL; level < 4; level ++ )
> @@ -940,7 +943,8 @@ int p2m_populate_ram(struct domain *d,
> paddr_t end)
> {
> return apply_p2m_changes(d, ALLOCATE, start, end,
> - 0, MATTR_MEM, p2m_ram_rw);
> + 0, MATTR_MEM, 0, p2m_ram_rw,
> + d->arch.p2m.default_access);
> }
>
> int map_mmio_regions(struct domain *d,
> @@ -952,7 +956,8 @@ int map_mmio_regions(struct domain *d,
> pfn_to_paddr(start_gfn),
> pfn_to_paddr(start_gfn + nr),
> pfn_to_paddr(mfn),
> - MATTR_DEV, p2m_mmio_direct);
> + MATTR_DEV, 0, p2m_mmio_direct,
> + d->arch.p2m.default_access);
> }
>
> int unmap_mmio_regions(struct domain *d,
> @@ -964,7 +969,8 @@ int unmap_mmio_regions(struct domain *d,
> pfn_to_paddr(start_gfn),
> pfn_to_paddr(start_gfn + nr),
> pfn_to_paddr(mfn),
> - MATTR_DEV, p2m_invalid);
> + MATTR_DEV, 0, p2m_invalid,
> + d->arch.p2m.default_access);
> }
>
> int guest_physmap_add_entry(struct domain *d,
> @@ -976,7 +982,8 @@ int guest_physmap_add_entry(struct domain *d,
> return apply_p2m_changes(d, INSERT,
> pfn_to_paddr(gpfn),
> pfn_to_paddr(gpfn + (1 << page_order)),
> - pfn_to_paddr(mfn), MATTR_MEM, t);
> + pfn_to_paddr(mfn), MATTR_MEM, 0, t,
> + d->arch.p2m.default_access);
> }
>
> void guest_physmap_remove_page(struct domain *d,
> @@ -986,7 +993,8 @@ void guest_physmap_remove_page(struct domain *d,
> apply_p2m_changes(d, REMOVE,
> pfn_to_paddr(gpfn),
> pfn_to_paddr(gpfn + (1<<page_order)),
> - pfn_to_paddr(mfn), MATTR_MEM, p2m_invalid);
> + pfn_to_paddr(mfn), MATTR_MEM, 0, p2m_invalid,
> + d->arch.p2m.default_access);
> }
>
> int p2m_alloc_table(struct domain *d)
> @@ -1090,6 +1098,8 @@ void p2m_teardown(struct domain *d)
>
> p2m_free_vmid(d);
>
> + radix_tree_destroy(&p2m->mem_access_settings, NULL);
> +
> spin_unlock(&p2m->lock);
> }
>
> @@ -1115,6 +1125,10 @@ int p2m_init(struct domain *d)
> p2m->max_mapped_gfn = 0;
> p2m->lowest_mapped_gfn = ULONG_MAX;
>
> + p2m->default_access = p2m_access_rwx;
> + p2m->mem_access_enabled = false;
> + radix_tree_init(&p2m->mem_access_settings);
> +
> err:
> spin_unlock(&p2m->lock);
>
> @@ -1129,7 +1143,8 @@ int relinquish_p2m_mapping(struct domain *d)
> pfn_to_paddr(p2m->lowest_mapped_gfn),
> pfn_to_paddr(p2m->max_mapped_gfn),
> pfn_to_paddr(INVALID_MFN),
> - MATTR_MEM, p2m_invalid);
> + MATTR_MEM, 0, p2m_invalid,
> + d->arch.p2m.default_access);
> }
>
> int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn)
> @@ -1143,7 +1158,8 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn)
> pfn_to_paddr(start_mfn),
> pfn_to_paddr(end_mfn),
> pfn_to_paddr(INVALID_MFN),
> - MATTR_MEM, p2m_invalid);
> + MATTR_MEM, 0, p2m_invalid,
> + d->arch.p2m.default_access);
> }
>
> unsigned long gmfn_to_mfn(struct domain *d, unsigned long gpfn)
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 9e0419e..f1a087e 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -17,6 +17,7 @@ struct hvm_domain
> {
> uint64_t params[HVM_NR_PARAMS];
> struct hvm_iommu iommu;
> + bool_t introspection_enabled;
> } __cacheline_aligned;
>
> #ifdef CONFIG_ARM_64
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index da36504..7583d9b 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -2,8 +2,9 @@
> #define _XEN_P2M_H
>
> #include <xen/mm.h>
> -
> +#include <xen/radix-tree.h>
> #include <xen/p2m-common.h>
> +#include <public/memory.h>
>
> #define paddr_bits PADDR_BITS
>
> @@ -48,6 +49,18 @@ struct p2m_domain {
> /* If true, and an access fault comes in and there is no mem_event listener,
> * pause domain. Otherwise, remove access restrictions. */
> bool_t access_required;
> +
> + /* Defines if mem_access is in use for the domain. */
> + bool_t mem_access_enabled;
> +
> + /* Default P2M access type for each page in the the domain: new pages,
> + * swapped in pages, cleared pages, and pages that are ambiguously
> + * retyped get this access type. See definition of p2m_access_t. */
> + p2m_access_t default_access;
> +
> + /* Radix tree to store the p2m_access_t settings as the pte's don't have
> + * enough available bits to store this information. */
> + struct radix_tree_root mem_access_settings;
> };
>
> /* List of possible type for each page in the p2m entry.
> @@ -217,6 +230,26 @@ static inline int get_page_and_type(struct page_info *page,
> /* get host p2m table */
> #define p2m_get_hostp2m(d) (&(d)->arch.p2m)
>
> +/* mem_event and mem_access are supported on any ARM guest */
> +static inline bool_t p2m_mem_access_sanity_check(struct domain *d)
> +{
> + return 1;
> +}
> +
> +static inline bool_t p2m_mem_event_sanity_check(struct domain *d)
> +{
> + return 1;
> +}
> +
> +/* Get access type for a pfn
> + * If pfn == -1ul, gets the default access type */
> +static inline
> +int p2m_get_mem_access(struct domain *d, unsigned long pfn,
> + xenmem_access_t *access)
> +{
> + return -ENOSYS;
> +}
> +
> #endif /* _XEN_P2M_H */
>
> /*
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index fcd26fb..cf7ab7c 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -441,7 +441,7 @@ union hsr {
> struct hsr_dabt {
> unsigned long dfsc:6; /* Data Fault Status Code */
> unsigned long write:1; /* Write / not Read */
> - unsigned long s1ptw:1; /* */
> + unsigned long s1ptw:1; /* Stage 2 fault during stage 1 translation */
> unsigned long cache:1; /* Cache Maintenance */
> unsigned long eat:1; /* External Abort Type */
> #ifdef CONFIG_ARM_32
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 1/7] xen/arm: p2m changes for mem_access support
2015-03-11 16:07 ` Stefano Stabellini
@ 2015-03-11 17:05 ` Tamas K Lengyel
0 siblings, 0 replies; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-11 17:05 UTC (permalink / raw)
To: Stefano Stabellini
Cc: wei.liu2@citrix.com, Ian Campbell, Tim Deegan, Julien Grall,
Ian Jackson, xen-devel@lists.xen.org, Stefano Stabellini,
Jan Beulich, Keir Fraser, Tamas K Lengyel
On Wed, Mar 11, 2015 at 5:07 PM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 6 Mar 2015, Tamas K Lengyel wrote:
>> Add necessary changes for page table construction routines to pass
>> the default access information. We store the p2m_access_t info in a
>> Radix tree as the PTE lacks enough software programmable bits.
>>
>> Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
>> ---
>> v13: - Rename access_in_use to mem_access_enabled.
>> - Define p2m_get_mem_access function prototype but
>> return -ENOSYS for now.
>> v11: - Move including common/mem_event.h down the series.
>> v10: - Typo fix and drop reshuffling things that no longer need
>> shuffling.
>> v8: - Drop lock inputs as common mem_access_check is postponed.
>> - Resurrect the radix tree with an extra boolean access_in_use flag
>> to indicate if the tree is empty to avoid lookups.
>> v7: - Remove radix tree init/destroy and move p2m_access_t store to page_info.
>> - Add p2m_gpfn_lock/unlock functions.
>> - Add bool_t lock input to p2m_lookup and apply_p2m_changes so the caller
>> can specify if locking should be performed. This is needed in order to
>> support mem_access_check from common.
>> v6: - Move mem_event header include to first patch that needs it.
>> v5: - #include grouping style-fix.
>> v4: - Move p2m_get_hostp2m definition here.
>> ---
>> xen/arch/arm/p2m.c | 52 +++++++++++++++++++++++++++--------------
>> xen/include/asm-arm/domain.h | 1 +
>> xen/include/asm-arm/p2m.h | 35 ++++++++++++++++++++++++++-
>> xen/include/asm-arm/processor.h | 2 +-
>> 4 files changed, 70 insertions(+), 20 deletions(-)
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 8809f5a..137e5a0 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -305,7 +305,7 @@ static void p2m_set_permission(lpae_t *e, p2m_type_t t, p2m_access_t a)
>> }
>>
>> static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
>> - p2m_type_t t)
>> + p2m_type_t t, p2m_access_t a)
>> {
>> paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
>> /* sh, xn and write bit will be defined in the following switches
>> @@ -335,8 +335,7 @@ static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr,
>> break;
>> }
>>
>> - /* We pass p2m_access_rwx as a placeholder for now. */
>> - p2m_set_permission(&e, t, p2m_access_rwx);
>> + p2m_set_permission(&e, t, a);
>>
>> ASSERT(!(pa & ~PAGE_MASK));
>> ASSERT(!(pa & ~PADDR_MASK));
>> @@ -394,7 +393,7 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
>> for ( i=0 ; i < LPAE_ENTRIES; i++ )
>> {
>> pte = mfn_to_p2m_entry(base_pfn + (i<<(level_shift-LPAE_SHIFT)),
>> - MATTR_MEM, t);
>> + MATTR_MEM, t, p2m->default_access);
>>
>> /*
>> * First and second level super pages set p2m.table = 0, but
>> @@ -414,7 +413,8 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
>>
>> unmap_domain_page(p);
>>
>> - pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, p2m_invalid);
>> + pte = mfn_to_p2m_entry(page_to_mfn(page), MATTR_MEM, p2m_invalid,
>> + p2m->default_access);
>>
>> p2m_write_pte(entry, pte, flush_cache);
>>
>> @@ -537,7 +537,8 @@ static int apply_one_level(struct domain *d,
>> paddr_t *maddr,
>> bool_t *flush,
>> int mattr,
>> - p2m_type_t t)
>> + p2m_type_t t,
>> + p2m_access_t a)
>> {
>> const paddr_t level_size = level_sizes[level];
>> const paddr_t level_mask = level_masks[level];
>> @@ -566,7 +567,7 @@ static int apply_one_level(struct domain *d,
>> page = alloc_domheap_pages(d, level_shift - PAGE_SHIFT, 0);
>> if ( page )
>> {
>> - pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t);
>> + pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t, a);
>> if ( level < 3 )
>> pte.p2m.table = 0;
>> p2m_write_pte(entry, pte, flush_cache);
>> @@ -601,7 +602,7 @@ static int apply_one_level(struct domain *d,
>> (level == 3 || !p2m_table(orig_pte)) )
>> {
>> /* New mapping is superpage aligned, make it */
>> - pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT, mattr, t);
>> + pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT, mattr, t, a);
>> if ( level < 3 )
>> pte.p2m.table = 0; /* Superpage entry */
>>
>> @@ -770,7 +771,9 @@ static int apply_p2m_changes(struct domain *d,
>> paddr_t end_gpaddr,
>> paddr_t maddr,
>> int mattr,
>> - p2m_type_t t)
>> + uint32_t mask,
>
> What is this for? It is not used and you always pass 0 here and in a
> later patch ~0.
It is used by the hypercall preemption mechanism. See
common/mem_access.c:105 for more info.
>
>> + p2m_type_t t,
>> + p2m_access_t a)
>> {
>> int rc, ret;
>> struct p2m_domain *p2m = &d->arch.p2m;
>> @@ -863,7 +866,7 @@ static int apply_p2m_changes(struct domain *d,
>> level, flush_pt, op,
>> start_gpaddr, end_gpaddr,
>> &addr, &maddr, &flush,
>> - mattr, t);
>> + mattr, t, a);
>> if ( ret < 0 ) { rc = ret ; goto out; }
>> count += ret;
>> /* L3 had better have done something! We cannot descend any further */
>> @@ -921,7 +924,7 @@ out:
>> */
>> apply_p2m_changes(d, REMOVE,
>> start_gpaddr, addr + level_sizes[level], orig_maddr,
>> - mattr, p2m_invalid);
>> + mattr, 0, p2m_invalid, d->arch.p2m.default_access);
>> }
>>
>> for ( level = P2M_ROOT_LEVEL; level < 4; level ++ )
>> @@ -940,7 +943,8 @@ int p2m_populate_ram(struct domain *d,
>> paddr_t end)
>> {
>> return apply_p2m_changes(d, ALLOCATE, start, end,
>> - 0, MATTR_MEM, p2m_ram_rw);
>> + 0, MATTR_MEM, 0, p2m_ram_rw,
>> + d->arch.p2m.default_access);
>> }
>>
>> int map_mmio_regions(struct domain *d,
>> @@ -952,7 +956,8 @@ int map_mmio_regions(struct domain *d,
>> pfn_to_paddr(start_gfn),
>> pfn_to_paddr(start_gfn + nr),
>> pfn_to_paddr(mfn),
>> - MATTR_DEV, p2m_mmio_direct);
>> + MATTR_DEV, 0, p2m_mmio_direct,
>> + d->arch.p2m.default_access);
>> }
>>
>> int unmap_mmio_regions(struct domain *d,
>> @@ -964,7 +969,8 @@ int unmap_mmio_regions(struct domain *d,
>> pfn_to_paddr(start_gfn),
>> pfn_to_paddr(start_gfn + nr),
>> pfn_to_paddr(mfn),
>> - MATTR_DEV, p2m_invalid);
>> + MATTR_DEV, 0, p2m_invalid,
>> + d->arch.p2m.default_access);
>> }
>>
>> int guest_physmap_add_entry(struct domain *d,
>> @@ -976,7 +982,8 @@ int guest_physmap_add_entry(struct domain *d,
>> return apply_p2m_changes(d, INSERT,
>> pfn_to_paddr(gpfn),
>> pfn_to_paddr(gpfn + (1 << page_order)),
>> - pfn_to_paddr(mfn), MATTR_MEM, t);
>> + pfn_to_paddr(mfn), MATTR_MEM, 0, t,
>> + d->arch.p2m.default_access);
>> }
>>
>> void guest_physmap_remove_page(struct domain *d,
>> @@ -986,7 +993,8 @@ void guest_physmap_remove_page(struct domain *d,
>> apply_p2m_changes(d, REMOVE,
>> pfn_to_paddr(gpfn),
>> pfn_to_paddr(gpfn + (1<<page_order)),
>> - pfn_to_paddr(mfn), MATTR_MEM, p2m_invalid);
>> + pfn_to_paddr(mfn), MATTR_MEM, 0, p2m_invalid,
>> + d->arch.p2m.default_access);
>> }
>>
>> int p2m_alloc_table(struct domain *d)
>> @@ -1090,6 +1098,8 @@ void p2m_teardown(struct domain *d)
>>
>> p2m_free_vmid(d);
>>
>> + radix_tree_destroy(&p2m->mem_access_settings, NULL);
>> +
>> spin_unlock(&p2m->lock);
>> }
>>
>> @@ -1115,6 +1125,10 @@ int p2m_init(struct domain *d)
>> p2m->max_mapped_gfn = 0;
>> p2m->lowest_mapped_gfn = ULONG_MAX;
>>
>> + p2m->default_access = p2m_access_rwx;
>> + p2m->mem_access_enabled = false;
>> + radix_tree_init(&p2m->mem_access_settings);
>> +
>> err:
>> spin_unlock(&p2m->lock);
>>
>> @@ -1129,7 +1143,8 @@ int relinquish_p2m_mapping(struct domain *d)
>> pfn_to_paddr(p2m->lowest_mapped_gfn),
>> pfn_to_paddr(p2m->max_mapped_gfn),
>> pfn_to_paddr(INVALID_MFN),
>> - MATTR_MEM, p2m_invalid);
>> + MATTR_MEM, 0, p2m_invalid,
>> + d->arch.p2m.default_access);
>> }
>>
>> int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn)
>> @@ -1143,7 +1158,8 @@ int p2m_cache_flush(struct domain *d, xen_pfn_t start_mfn, xen_pfn_t end_mfn)
>> pfn_to_paddr(start_mfn),
>> pfn_to_paddr(end_mfn),
>> pfn_to_paddr(INVALID_MFN),
>> - MATTR_MEM, p2m_invalid);
>> + MATTR_MEM, 0, p2m_invalid,
>> + d->arch.p2m.default_access);
>> }
>>
>> unsigned long gmfn_to_mfn(struct domain *d, unsigned long gpfn)
>> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
>> index 9e0419e..f1a087e 100644
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -17,6 +17,7 @@ struct hvm_domain
>> {
>> uint64_t params[HVM_NR_PARAMS];
>> struct hvm_iommu iommu;
>> + bool_t introspection_enabled;
>> } __cacheline_aligned;
>>
>> #ifdef CONFIG_ARM_64
>> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
>> index da36504..7583d9b 100644
>> --- a/xen/include/asm-arm/p2m.h
>> +++ b/xen/include/asm-arm/p2m.h
>> @@ -2,8 +2,9 @@
>> #define _XEN_P2M_H
>>
>> #include <xen/mm.h>
>> -
>> +#include <xen/radix-tree.h>
>> #include <xen/p2m-common.h>
>> +#include <public/memory.h>
>>
>> #define paddr_bits PADDR_BITS
>>
>> @@ -48,6 +49,18 @@ struct p2m_domain {
>> /* If true, and an access fault comes in and there is no mem_event listener,
>> * pause domain. Otherwise, remove access restrictions. */
>> bool_t access_required;
>> +
>> + /* Defines if mem_access is in use for the domain. */
>> + bool_t mem_access_enabled;
>> +
>> + /* Default P2M access type for each page in the the domain: new pages,
>> + * swapped in pages, cleared pages, and pages that are ambiguously
>> + * retyped get this access type. See definition of p2m_access_t. */
>> + p2m_access_t default_access;
>> +
>> + /* Radix tree to store the p2m_access_t settings as the pte's don't have
>> + * enough available bits to store this information. */
>> + struct radix_tree_root mem_access_settings;
>> };
>>
>> /* List of possible type for each page in the p2m entry.
>> @@ -217,6 +230,26 @@ static inline int get_page_and_type(struct page_info *page,
>> /* get host p2m table */
>> #define p2m_get_hostp2m(d) (&(d)->arch.p2m)
>>
>> +/* mem_event and mem_access are supported on any ARM guest */
>> +static inline bool_t p2m_mem_access_sanity_check(struct domain *d)
>> +{
>> + return 1;
>> +}
>> +
>> +static inline bool_t p2m_mem_event_sanity_check(struct domain *d)
>> +{
>> + return 1;
>> +}
>> +
>> +/* Get access type for a pfn
>> + * If pfn == -1ul, gets the default access type */
>> +static inline
>> +int p2m_get_mem_access(struct domain *d, unsigned long pfn,
>> + xenmem_access_t *access)
>> +{
>> + return -ENOSYS;
>> +}
>> +
>> #endif /* _XEN_P2M_H */
>>
>> /*
>> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
>> index fcd26fb..cf7ab7c 100644
>> --- a/xen/include/asm-arm/processor.h
>> +++ b/xen/include/asm-arm/processor.h
>> @@ -441,7 +441,7 @@ union hsr {
>> struct hsr_dabt {
>> unsigned long dfsc:6; /* Data Fault Status Code */
>> unsigned long write:1; /* Write / not Read */
>> - unsigned long s1ptw:1; /* */
>> + unsigned long s1ptw:1; /* Stage 2 fault during stage 1 translation */
>> unsigned long cache:1; /* Cache Maintenance */
>> unsigned long eat:1; /* External Abort Type */
>> #ifdef CONFIG_ARM_32
>> --
>> 2.1.4
>>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 1/7] xen/arm: p2m changes for mem_access support
2015-03-06 21:24 ` [PATCH V13 1/7] xen/arm: p2m changes for mem_access support Tamas K Lengyel
2015-03-11 16:07 ` Stefano Stabellini
@ 2015-03-12 11:27 ` Ian Campbell
2015-03-12 12:22 ` Tamas K Lengyel
2015-03-12 12:57 ` Julien Grall
2 siblings, 1 reply; 71+ messages in thread
From: Ian Campbell @ 2015-03-12 11:27 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, stefano.stabellini, ian.jackson, julien.grall, tim,
xen-devel, stefano.stabellini, jbeulich, keir
On Fri, 2015-03-06 at 22:24 +0100, Tamas K Lengyel wrote:
> Add necessary changes for page table construction routines to pass
> the default access information.
You do more than just plumb through the access information variable
thoguh. You seem to setup preemption support, add some tother
needed-by-generic-code infrastructure functions and data fields too etc.
The actual code changes seem OK to me, assuming I have inferred your
intention correctly. But please could you expand on this sentence to
describe the full scope/intention of the changes.
> We store the p2m_access_t info in a
> Radix tree as the PTE lacks enough software programmable bits.
I think here you just setup the radix tree which will eventually be used
to do this, at least I can't see the actual store, nor any lookups.
Assuming that is what was intended please indicate that here.
It's possible that some of these changes would have been clearer done as
individual patches later on, or part of a later patch which actually
uses things. We're at v13 though so I'll leave that for you to decide.
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 1/7] xen/arm: p2m changes for mem_access support
2015-03-12 11:27 ` Ian Campbell
@ 2015-03-12 12:22 ` Tamas K Lengyel
2015-03-12 13:53 ` Ian Campbell
0 siblings, 1 reply; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-12 12:22 UTC (permalink / raw)
To: Ian Campbell
Cc: wei.liu2, Stefano Stabellini, Ian Jackson, Julien Grall,
Tim Deegan, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 1622 bytes --]
On Thu, Mar 12, 2015 at 12:27 PM, Ian Campbell <ian.campbell@citrix.com>
wrote:
> On Fri, 2015-03-06 at 22:24 +0100, Tamas K Lengyel wrote:
> > Add necessary changes for page table construction routines to pass
> > the default access information.
>
> You do more than just plumb through the access information variable
> thoguh. You seem to setup preemption support, add some tother
> needed-by-generic-code infrastructure functions and data fields too etc.
>
> The actual code changes seem OK to me, assuming I have inferred your
> intention correctly. But please could you expand on this sentence to
> describe the full scope/intention of the changes.
>
Ack, certainly.
>
> > We store the p2m_access_t info in a
> > Radix tree as the PTE lacks enough software programmable bits.
>
> I think here you just setup the radix tree which will eventually be used
> to do this, at least I can't see the actual store, nor any lookups.
> Assuming that is what was intended please indicate that here.
>
> It's possible that some of these changes would have been clearer done as
> individual patches later on, or part of a later patch which actually
> uses things. We're at v13 though so I'll leave that for you to decide.
>
I guess naming this patch something like "groundwork for mem_access
support" would be more descriptive. Adding everything when they are
actually getting used would have resulted in a massive patch that I think
would have been harder to review. If possible, I will just clean up the
patch message to better describe what is being added by this patch instead
of breaking it down further.
Thanks,
Tamas
[-- Attachment #1.2: Type: text/html, Size: 2288 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 1/7] xen/arm: p2m changes for mem_access support
2015-03-12 12:22 ` Tamas K Lengyel
@ 2015-03-12 13:53 ` Ian Campbell
0 siblings, 0 replies; 71+ messages in thread
From: Ian Campbell @ 2015-03-12 13:53 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, Stefano Stabellini, Ian Jackson, Julien Grall,
Tim Deegan, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
On Thu, 2015-03-12 at 13:22 +0100, Tamas K Lengyel wrote:
> I guess naming this patch something like "groundwork for mem_access
> support" would be more descriptive. Adding everything when they are
> actually getting used would have resulted in a massive patch that I
> think would have been harder to review. If possible, I will just clean
> up the patch message to better describe what is being added by this
> patch instead of breaking it down further.
Sure, thanks.
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 1/7] xen/arm: p2m changes for mem_access support
2015-03-06 21:24 ` [PATCH V13 1/7] xen/arm: p2m changes for mem_access support Tamas K Lengyel
2015-03-11 16:07 ` Stefano Stabellini
2015-03-12 11:27 ` Ian Campbell
@ 2015-03-12 12:57 ` Julien Grall
2015-03-12 13:18 ` Tamas K Lengyel
2015-03-12 13:56 ` Ian Campbell
2 siblings, 2 replies; 71+ messages in thread
From: Julien Grall @ 2015-03-12 12:57 UTC (permalink / raw)
To: Tamas K Lengyel, xen-devel
Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson, tim,
stefano.stabellini, jbeulich, keir
Hi Tamas,
On 06/03/15 21:24, Tamas K Lengyel wrote:
> @@ -1090,6 +1098,8 @@ void p2m_teardown(struct domain *d)
>
> p2m_free_vmid(d);
>
> + radix_tree_destroy(&p2m->mem_access_settings, NULL);
> +
> spin_unlock(&p2m->lock);
> }
>
> @@ -1115,6 +1125,10 @@ int p2m_init(struct domain *d)
> p2m->max_mapped_gfn = 0;
> p2m->lowest_mapped_gfn = ULONG_MAX;
>
> + p2m->default_access = p2m_access_rwx;
> + p2m->mem_access_enabled = false;
false is defined for bool not bool_t.
Please use 0 here.
[..]
> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> index da36504..7583d9b 100644
> --- a/xen/include/asm-arm/p2m.h
> +++ b/xen/include/asm-arm/p2m.h
> @@ -2,8 +2,9 @@
> #define _XEN_P2M_H
>
> #include <xen/mm.h>
> -
> +#include <xen/radix-tree.h>
> #include <xen/p2m-common.h>
> +#include <public/memory.h>
>
> #define paddr_bits PADDR_BITS
>
> @@ -48,6 +49,18 @@ struct p2m_domain {
> /* If true, and an access fault comes in and there is no mem_event listener,
> * pause domain. Otherwise, remove access restrictions. */
> bool_t access_required;
> +
> + /* Defines if mem_access is in use for the domain. */
> + bool_t mem_access_enabled;
> +
> + /* Default P2M access type for each page in the the domain: new pages,
> + * swapped in pages, cleared pages, and pages that are ambiguously
> + * retyped get this access type. See definition of p2m_access_t. */
Coding style. It should be:
/*
* Default ...
* ....
*/
> + p2m_access_t default_access;
> +
> + /* Radix tree to store the p2m_access_t settings as the pte's don't have
> + * enough available bits to store this information. */
Ditto
> + struct radix_tree_root mem_access_settings;
> };
>
> /* List of possible type for each page in the p2m entry.
> @@ -217,6 +230,26 @@ static inline int get_page_and_type(struct page_info *page,
> /* get host p2m table */
> #define p2m_get_hostp2m(d) (&(d)->arch.p2m)
>
> +/* mem_event and mem_access are supported on any ARM guest */
> +static inline bool_t p2m_mem_access_sanity_check(struct domain *d)
> +{
> + return 1;
> +}
> +
> +static inline bool_t p2m_mem_event_sanity_check(struct domain *d)
> +{
> + return 1;
> +}
> +
> +/* Get access type for a pfn
> + * If pfn == -1ul, gets the default access type */
Ditto
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 1/7] xen/arm: p2m changes for mem_access support
2015-03-12 12:57 ` Julien Grall
@ 2015-03-12 13:18 ` Tamas K Lengyel
2015-03-12 13:25 ` Julien Grall
2015-03-12 13:56 ` Ian Campbell
1 sibling, 1 reply; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-12 13:18 UTC (permalink / raw)
To: Julien Grall
Cc: wei.liu2, Ian Campbell, Stefano Stabellini, Tim Deegan,
Ian Jackson, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 2821 bytes --]
On Thu, Mar 12, 2015 at 1:57 PM, Julien Grall <julien.grall@linaro.org>
wrote:
> Hi Tamas,
>
> On 06/03/15 21:24, Tamas K Lengyel wrote:
> > @@ -1090,6 +1098,8 @@ void p2m_teardown(struct domain *d)
> >
> > p2m_free_vmid(d);
> >
> > + radix_tree_destroy(&p2m->mem_access_settings, NULL);
> > +
> > spin_unlock(&p2m->lock);
> > }
> >
> > @@ -1115,6 +1125,10 @@ int p2m_init(struct domain *d)
> > p2m->max_mapped_gfn = 0;
> > p2m->lowest_mapped_gfn = ULONG_MAX;
> >
> > + p2m->default_access = p2m_access_rwx;
> > + p2m->mem_access_enabled = false;
>
> false is defined for bool not bool_t.
> Please use 0 here.
>
Ack.
>
> [..]
>
> > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
> > index da36504..7583d9b 100644
> > --- a/xen/include/asm-arm/p2m.h
> > +++ b/xen/include/asm-arm/p2m.h
> > @@ -2,8 +2,9 @@
> > #define _XEN_P2M_H
> >
> > #include <xen/mm.h>
> > -
> > +#include <xen/radix-tree.h>
> > #include <xen/p2m-common.h>
> > +#include <public/memory.h>
> >
> > #define paddr_bits PADDR_BITS
> >
> > @@ -48,6 +49,18 @@ struct p2m_domain {
> > /* If true, and an access fault comes in and there is no mem_event
> listener,
> > * pause domain. Otherwise, remove access restrictions. */
> > bool_t access_required;
> > +
> > + /* Defines if mem_access is in use for the domain. */
> > + bool_t mem_access_enabled;
> > +
> > + /* Default P2M access type for each page in the the domain: new
> pages,
> > + * swapped in pages, cleared pages, and pages that are ambiguously
> > + * retyped get this access type. See definition of p2m_access_t. */
>
> Coding style. It should be:
>
> /*
> * Default ...
> * ....
> */
>
> > + p2m_access_t default_access;
> > +
> > + /* Radix tree to store the p2m_access_t settings as the pte's don't
> have
> > + * enough available bits to store this information. */
>
> Ditto
>
> > + struct radix_tree_root mem_access_settings;
> > };
> >
> > /* List of possible type for each page in the p2m entry.
> > @@ -217,6 +230,26 @@ static inline int get_page_and_type(struct
> page_info *page,
> > /* get host p2m table */
> > #define p2m_get_hostp2m(d) (&(d)->arch.p2m)
> >
> > +/* mem_event and mem_access are supported on any ARM guest */
> > +static inline bool_t p2m_mem_access_sanity_check(struct domain *d)
> > +{
> > + return 1;
> > +}
> > +
> > +static inline bool_t p2m_mem_event_sanity_check(struct domain *d)
> > +{
> > + return 1;
> > +}
> > +
> > +/* Get access type for a pfn
> > + * If pfn == -1ul, gets the default access type */
>
> Ditto
>
Ack, however, all other comments here followed this style even for
multi-line comments. If I change the style only on my comments it will be
mixed (and ugly) IMHO.
>
> Regards,
>
> --
> Julien Grall
>
Thanks,
Tamas
[-- Attachment #1.2: Type: text/html, Size: 4224 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 1/7] xen/arm: p2m changes for mem_access support
2015-03-12 13:18 ` Tamas K Lengyel
@ 2015-03-12 13:25 ` Julien Grall
2015-03-12 13:55 ` Ian Campbell
0 siblings, 1 reply; 71+ messages in thread
From: Julien Grall @ 2015-03-12 13:25 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, Ian Campbell, Stefano Stabellini, Tim Deegan,
Ian Jackson, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
On 12/03/15 13:18, Tamas K Lengyel wrote:
> > + struct radix_tree_root mem_access_settings;
> > };
> >
> > /* List of possible type for each page in the p2m entry.
> > @@ -217,6 +230,26 @@ static inline int get_page_and_type(struct page_info *page,
> > /* get host p2m table */
> > #define p2m_get_hostp2m(d) (&(d)->arch.p2m)
> >
> > +/* mem_event and mem_access are supported on any ARM guest */
> > +static inline bool_t p2m_mem_access_sanity_check(struct domain *d)
> > +{
> > + return 1;
> > +}
> > +
> > +static inline bool_t p2m_mem_event_sanity_check(struct domain *d)
> > +{
> > + return 1;
> > +}
> > +
> > +/* Get access type for a pfn
> > + * If pfn == -1ul, gets the default access type */
>
> Ditto
>
>
> Ack, however, all other comments here followed this style even for
> multi-line comments. If I change the style only on my comments it will
> be mixed (and ugly) IMHO.
Hmmm... right. Ian, Stefano, any comments?
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 1/7] xen/arm: p2m changes for mem_access support
2015-03-12 13:25 ` Julien Grall
@ 2015-03-12 13:55 ` Ian Campbell
0 siblings, 0 replies; 71+ messages in thread
From: Ian Campbell @ 2015-03-12 13:55 UTC (permalink / raw)
To: Julien Grall
Cc: wei.liu2, Stefano Stabellini, Tim Deegan, Ian Jackson, xen-devel,
stefano.stabellini, Jan Beulich, Keir Fraser, Tamas K Lengyel
On Thu, 2015-03-12 at 13:25 +0000, Julien Grall wrote:
> On 12/03/15 13:18, Tamas K Lengyel wrote:
> > > + struct radix_tree_root mem_access_settings;
> > > };
> > >
> > > /* List of possible type for each page in the p2m entry.
> > > @@ -217,6 +230,26 @@ static inline int get_page_and_type(struct page_info *page,
> > > /* get host p2m table */
> > > #define p2m_get_hostp2m(d) (&(d)->arch.p2m)
> > >
> > > +/* mem_event and mem_access are supported on any ARM guest */
> > > +static inline bool_t p2m_mem_access_sanity_check(struct domain *d)
> > > +{
> > > + return 1;
> > > +}
> > > +
> > > +static inline bool_t p2m_mem_event_sanity_check(struct domain *d)
> > > +{
> > > + return 1;
> > > +}
> > > +
> > > +/* Get access type for a pfn
> > > + * If pfn == -1ul, gets the default access type */
> >
> > Ditto
> >
> >
> > Ack, however, all other comments here followed this style even for
> > multi-line comments. If I change the style only on my comments it will
> > be mixed (and ugly) IMHO.
>
> Hmmm... right. Ian, Stefano, any comments?
Follow the existing style in the file.
>
> Regards,
>
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 1/7] xen/arm: p2m changes for mem_access support
2015-03-12 12:57 ` Julien Grall
2015-03-12 13:18 ` Tamas K Lengyel
@ 2015-03-12 13:56 ` Ian Campbell
2015-03-12 14:10 ` Andrew Cooper
1 sibling, 1 reply; 71+ messages in thread
From: Ian Campbell @ 2015-03-12 13:56 UTC (permalink / raw)
To: Julien Grall
Cc: wei.liu2, stefano.stabellini, tim, ian.jackson, xen-devel,
stefano.stabellini, jbeulich, keir, Tamas K Lengyel
On Thu, 2015-03-12 at 12:57 +0000, Julien Grall wrote:
> Hi Tamas,
>
> On 06/03/15 21:24, Tamas K Lengyel wrote:
> > @@ -1090,6 +1098,8 @@ void p2m_teardown(struct domain *d)
> >
> > p2m_free_vmid(d);
> >
> > + radix_tree_destroy(&p2m->mem_access_settings, NULL);
> > +
> > spin_unlock(&p2m->lock);
> > }
> >
> > @@ -1115,6 +1125,10 @@ int p2m_init(struct domain *d)
> > p2m->max_mapped_gfn = 0;
> > p2m->lowest_mapped_gfn = ULONG_MAX;
> >
> > + p2m->default_access = p2m_access_rwx;
> > + p2m->mem_access_enabled = false;
>
> false is defined for bool not bool_t.
> Please use 0 here.
I'm not convinced, false is false whatever you assign it to.
Ian.
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 1/7] xen/arm: p2m changes for mem_access support
2015-03-12 13:56 ` Ian Campbell
@ 2015-03-12 14:10 ` Andrew Cooper
2015-03-12 16:56 ` Julien Grall
0 siblings, 1 reply; 71+ messages in thread
From: Andrew Cooper @ 2015-03-12 14:10 UTC (permalink / raw)
To: Ian Campbell, Julien Grall
Cc: wei.liu2, stefano.stabellini, ian.jackson, tim, xen-devel,
stefano.stabellini, jbeulich, keir, Tamas K Lengyel
On 12/03/15 13:56, Ian Campbell wrote:
> On Thu, 2015-03-12 at 12:57 +0000, Julien Grall wrote:
>> Hi Tamas,
>>
>> On 06/03/15 21:24, Tamas K Lengyel wrote:
>>> @@ -1090,6 +1098,8 @@ void p2m_teardown(struct domain *d)
>>>
>>> p2m_free_vmid(d);
>>>
>>> + radix_tree_destroy(&p2m->mem_access_settings, NULL);
>>> +
>>> spin_unlock(&p2m->lock);
>>> }
>>>
>>> @@ -1115,6 +1125,10 @@ int p2m_init(struct domain *d)
>>> p2m->max_mapped_gfn = 0;
>>> p2m->lowest_mapped_gfn = ULONG_MAX;
>>>
>>> + p2m->default_access = p2m_access_rwx;
>>> + p2m->mem_access_enabled = false;
>> false is defined for bool not bool_t.
>> Please use 0 here.
> I'm not convinced, false is false whatever you assign it to.
C specified that false expands to the constant 0, and true to the
constant 1.
They are fine for use with any integral type.
~Andrew
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 1/7] xen/arm: p2m changes for mem_access support
2015-03-12 14:10 ` Andrew Cooper
@ 2015-03-12 16:56 ` Julien Grall
2015-03-12 17:11 ` Ian Campbell
0 siblings, 1 reply; 71+ messages in thread
From: Julien Grall @ 2015-03-12 16:56 UTC (permalink / raw)
To: Andrew Cooper, Ian Campbell
Cc: wei.liu2, stefano.stabellini, ian.jackson, tim, xen-devel,
stefano.stabellini, jbeulich, keir, Tamas K Lengyel
On 12/03/15 14:10, Andrew Cooper wrote:
> On 12/03/15 13:56, Ian Campbell wrote:
>> On Thu, 2015-03-12 at 12:57 +0000, Julien Grall wrote:
>>> Hi Tamas,
>>>
>>> On 06/03/15 21:24, Tamas K Lengyel wrote:
>>>> @@ -1090,6 +1098,8 @@ void p2m_teardown(struct domain *d)
>>>>
>>>> p2m_free_vmid(d);
>>>>
>>>> + radix_tree_destroy(&p2m->mem_access_settings, NULL);
>>>> +
>>>> spin_unlock(&p2m->lock);
>>>> }
>>>>
>>>> @@ -1115,6 +1125,10 @@ int p2m_init(struct domain *d)
>>>> p2m->max_mapped_gfn = 0;
>>>> p2m->lowest_mapped_gfn = ULONG_MAX;
>>>>
>>>> + p2m->default_access = p2m_access_rwx;
>>>> + p2m->mem_access_enabled = false;
>>> false is defined for bool not bool_t.
>>> Please use 0 here.
>> I'm not convinced, false is false whatever you assign it to.
>
> C specified that false expands to the constant 0, and true to the
> constant 1.
>
> They are fine for use with any integral type.
It's still mixing types... which are defined in different headers.
--
Julien Grall
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 1/7] xen/arm: p2m changes for mem_access support
2015-03-12 16:56 ` Julien Grall
@ 2015-03-12 17:11 ` Ian Campbell
0 siblings, 0 replies; 71+ messages in thread
From: Ian Campbell @ 2015-03-12 17:11 UTC (permalink / raw)
To: Julien Grall
Cc: wei.liu2, stefano.stabellini, Andrew Cooper, tim, xen-devel,
stefano.stabellini, jbeulich, keir, Tamas K Lengyel, ian.jackson
On Thu, 2015-03-12 at 16:56 +0000, Julien Grall wrote:
> On 12/03/15 14:10, Andrew Cooper wrote:
> > On 12/03/15 13:56, Ian Campbell wrote:
> >> On Thu, 2015-03-12 at 12:57 +0000, Julien Grall wrote:
> >>> Hi Tamas,
> >>>
> >>> On 06/03/15 21:24, Tamas K Lengyel wrote:
> >>>> @@ -1090,6 +1098,8 @@ void p2m_teardown(struct domain *d)
> >>>>
> >>>> p2m_free_vmid(d);
> >>>>
> >>>> + radix_tree_destroy(&p2m->mem_access_settings, NULL);
> >>>> +
> >>>> spin_unlock(&p2m->lock);
> >>>> }
> >>>>
> >>>> @@ -1115,6 +1125,10 @@ int p2m_init(struct domain *d)
> >>>> p2m->max_mapped_gfn = 0;
> >>>> p2m->lowest_mapped_gfn = ULONG_MAX;
> >>>>
> >>>> + p2m->default_access = p2m_access_rwx;
> >>>> + p2m->mem_access_enabled = false;
> >>> false is defined for bool not bool_t.
> >>> Please use 0 here.
> >> I'm not convinced, false is false whatever you assign it to.
> >
> > C specified that false expands to the constant 0, and true to the
> > constant 1.
> >
> > They are fine for use with any integral type.
>
> It's still mixing types... which are defined in different headers.
Whatever, if you care so much please send a patch to fix it. I'm not
going to reject Tamas' patches on this basis.
Ian.
^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH V13 2/7] xen/arm: Implement domain_get_maximum_gpfn
2015-03-06 21:24 [PATCH V13 0/7] Mem_access for ARM Tamas K Lengyel
2015-03-06 21:24 ` [PATCH V13 1/7] xen/arm: p2m changes for mem_access support Tamas K Lengyel
@ 2015-03-06 21:24 ` Tamas K Lengyel
2015-03-11 15:43 ` Stefano Stabellini
2015-03-06 21:24 ` [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages Tamas K Lengyel
` (5 subsequent siblings)
7 siblings, 1 reply; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-06 21:24 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson,
julien.grall, tim, stefano.stabellini, jbeulich, keir
From: Julien Grall <julien.grall@linaro.org>
The function domain_get_maximum_gpfn is returning the maximum gpfn ever
mapped in the guest. We can use d->arch.p2m.max_mapped_gfn for this purpose.
We use this in xenaccess as to avoid the user attempting to set page
permissions on pages which don't exist for the domain, as a non-arch specific
sanity check.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
xen/arch/arm/mm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 7d4ba0c..ca0aa69 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -985,7 +985,7 @@ int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
unsigned long domain_get_maximum_gpfn(struct domain *d)
{
- return -ENOSYS;
+ return d->arch.p2m.max_mapped_gfn;
}
void share_xen_page_with_guest(struct page_info *page,
--
2.1.4
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [PATCH V13 2/7] xen/arm: Implement domain_get_maximum_gpfn
2015-03-06 21:24 ` [PATCH V13 2/7] xen/arm: Implement domain_get_maximum_gpfn Tamas K Lengyel
@ 2015-03-11 15:43 ` Stefano Stabellini
0 siblings, 0 replies; 71+ messages in thread
From: Stefano Stabellini @ 2015-03-11 15:43 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, ian.campbell, stefano.stabellini, tim, julien.grall,
ian.jackson, xen-devel, stefano.stabellini, jbeulich, keir
On Fri, 6 Mar 2015, Tamas K Lengyel wrote:
> From: Julien Grall <julien.grall@linaro.org>
>
> The function domain_get_maximum_gpfn is returning the maximum gpfn ever
> mapped in the guest. We can use d->arch.p2m.max_mapped_gfn for this purpose.
>
> We use this in xenaccess as to avoid the user attempting to set page
> permissions on pages which don't exist for the domain, as a non-arch specific
> sanity check.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> xen/arch/arm/mm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 7d4ba0c..ca0aa69 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -985,7 +985,7 @@ int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
>
> unsigned long domain_get_maximum_gpfn(struct domain *d)
> {
> - return -ENOSYS;
> + return d->arch.p2m.max_mapped_gfn;
> }
>
> void share_xen_page_with_guest(struct page_info *page,
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-06 21:24 [PATCH V13 0/7] Mem_access for ARM Tamas K Lengyel
2015-03-06 21:24 ` [PATCH V13 1/7] xen/arm: p2m changes for mem_access support Tamas K Lengyel
2015-03-06 21:24 ` [PATCH V13 2/7] xen/arm: Implement domain_get_maximum_gpfn Tamas K Lengyel
@ 2015-03-06 21:24 ` Tamas K Lengyel
2015-03-12 12:08 ` Ian Campbell
` (2 more replies)
2015-03-06 21:24 ` [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events Tamas K Lengyel
` (4 subsequent siblings)
7 siblings, 3 replies; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-06 21:24 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson,
julien.grall, tim, stefano.stabellini, jbeulich, keir,
Tamas K Lengyel
The guestcopy helpers use the MMU to verify that the given guest has read/write
access to a given page during hypercalls. As we may have custom mem_access
permissions set on these pages, we do a software-based type checking in case
the MMU based approach failed, but only if mem_access_enabled is set.
These memory accesses are not forwarded to the mem_event listener. Accesses
performed by the hypervisor are currently not part of the mem_access scheme.
This is consistent behaviour with the x86 side as well.
Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
---
v12: - Check for mfn_valid as well.
---
xen/arch/arm/guestcopy.c | 124 +++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 121 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
index 7dbaeca..d42a469 100644
--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -6,6 +6,115 @@
#include <asm/mm.h>
#include <asm/guest_access.h>
+#include <asm/p2m.h>
+
+/*
+ * If mem_access is in use it might have been the reason why get_page_from_gva
+ * failed to fetch the page, as it uses the MMU for the permission checking.
+ * Only in these cases we do a software-based type check and fetch the page if
+ * we indeed found a conflicting mem_access setting.
+ */
+static int check_type_get_page(vaddr_t gva, unsigned long flag,
+ struct page_info** page)
+{
+ long rc;
+ paddr_t ipa;
+ unsigned long maddr;
+ unsigned long mfn;
+ xenmem_access_t xma;
+ p2m_type_t t;
+
+ rc = gva_to_ipa(gva, &ipa);
+ if ( rc < 0 )
+ return rc;
+
+ /*
+ * We do this first as this is faster in the default case when no
+ * permission is set on the page.
+ */
+ rc = p2m_get_mem_access(current->domain, paddr_to_pfn(ipa), &xma);
+ if ( rc < 0 )
+ return rc;
+
+ /* Let's check if mem_access limited the access. */
+ switch ( xma )
+ {
+ default:
+ case XENMEM_access_rwx:
+ case XENMEM_access_rw:
+ return -EFAULT;
+ case XENMEM_access_n2rwx:
+ case XENMEM_access_n:
+ case XENMEM_access_x:
+ break;
+ case XENMEM_access_wx:
+ case XENMEM_access_w:
+ if ( flag == GV2M_READ )
+ break;
+ else return -EFAULT;
+ case XENMEM_access_rx2rw:
+ case XENMEM_access_rx:
+ case XENMEM_access_r:
+ if ( flag == GV2M_WRITE )
+ break;
+ else return -EFAULT;
+ }
+
+ /*
+ * We had a mem_access permission limiting the access, but the page type
+ * could also be limiting, so we need to check that as well.
+ */
+ maddr = p2m_lookup(current->domain, ipa, &t);
+ if ( maddr == INVALID_PADDR )
+ return -EFAULT;
+
+ mfn = maddr >> PAGE_SHIFT;
+ if ( !mfn_valid(mfn) )
+ return -EFAULT;
+
+ /*
+ * All page types are readable so we only have to check the
+ * type if writing.
+ */
+ if ( flag == GV2M_WRITE )
+ {
+ switch ( t )
+ {
+ case p2m_ram_rw:
+ case p2m_iommu_map_rw:
+ case p2m_map_foreign:
+ case p2m_grant_map_rw:
+ case p2m_mmio_direct:
+ /* Base type allows writing, we are good to get the page. */
+ break;
+ default:
+ return -EFAULT;
+ }
+ }
+
+ *page = mfn_to_page(mfn);
+
+ if ( unlikely(!get_page(*page, current->domain)) )
+ {
+ *page = NULL;
+ return -EFAULT;
+ }
+
+ return 0;
+}
+
+/*
+ * If mem_access is not in use, we have a fault. If mem_access is in use, do the
+ * software-based type checking.
+ */
+static inline
+int check_mem_access(vaddr_t gva, unsigned long flag, struct page_info **page)
+{
+ if( !current->domain->arch.p2m.mem_access_enabled )
+ return -EFAULT;
+
+ return check_type_get_page(gva, flag, page);
+}
static unsigned long raw_copy_to_guest_helper(void *to, const void *from,
unsigned len, int flush_dcache)
@@ -21,7 +130,10 @@ static unsigned long raw_copy_to_guest_helper(void *to, const void *from,
page = get_page_from_gva(current->domain, (vaddr_t) to, GV2M_WRITE);
if ( page == NULL )
- return len;
+ {
+ if ( check_mem_access((vaddr_t) to, GV2M_WRITE, &page) < 0 )
+ return len;
+ }
p = __map_domain_page(page);
p += offset;
@@ -68,7 +180,10 @@ unsigned long raw_clear_guest(void *to, unsigned len)
page = get_page_from_gva(current->domain, (vaddr_t) to, GV2M_WRITE);
if ( page == NULL )
- return len;
+ {
+ if ( check_mem_access((vaddr_t) to, GV2M_WRITE, &page) < 0 )
+ return len;
+ }
p = __map_domain_page(page);
p += offset;
@@ -100,7 +215,10 @@ unsigned long raw_copy_from_guest(void *to, const void __user *from, unsigned le
page = get_page_from_gva(current->domain, (vaddr_t) from, GV2M_READ);
if ( page == NULL )
- return len;
+ {
+ if ( check_mem_access((vaddr_t) from, GV2M_READ, &page) < 0 )
+ return len;
+ }
p = __map_domain_page(page);
p += ((vaddr_t)from & (~PAGE_MASK));
--
2.1.4
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-06 21:24 ` [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages Tamas K Lengyel
@ 2015-03-12 12:08 ` Ian Campbell
2015-03-12 12:31 ` Tamas K Lengyel
2015-03-12 13:24 ` Julien Grall
2015-03-12 13:50 ` Julien Grall
2 siblings, 1 reply; 71+ messages in thread
From: Ian Campbell @ 2015-03-12 12:08 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, stefano.stabellini, ian.jackson, julien.grall, tim,
xen-devel, stefano.stabellini, jbeulich, keir
On Fri, 2015-03-06 at 22:24 +0100, Tamas K Lengyel wrote:
> The guestcopy helpers use the MMU to verify that the given guest has read/write
> access to a given page during hypercalls. As we may have custom mem_access
> permissions set on these pages, we do a software-based type checking in case
> the MMU based approach failed, but only if mem_access_enabled is set.
>
> These memory accesses are not forwarded to the mem_event listener. Accesses
> performed by the hypervisor are currently not part of the mem_access scheme.
> This is consistent behaviour with the x86 side as well.
>
> Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
> ---
> v12: - Check for mfn_valid as well.
> ---
> xen/arch/arm/guestcopy.c | 124 +++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 121 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index 7dbaeca..d42a469 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -6,6 +6,115 @@
>
> #include <asm/mm.h>
> #include <asm/guest_access.h>
> +#include <asm/p2m.h>
> +
> +/*
> + * If mem_access is in use it might have been the reason why get_page_from_gva
> + * failed to fetch the page, as it uses the MMU for the permission checking.
> + * Only in these cases we do a software-based type check and fetch the page if
> + * we indeed found a conflicting mem_access setting.
> + */
> +static int check_type_get_page(vaddr_t gva, unsigned long flag,
> + struct page_info** page)
> +{
> + long rc;
> + paddr_t ipa;
> + unsigned long maddr;
> + unsigned long mfn;
> + xenmem_access_t xma;
> + p2m_type_t t;
> +
> + rc = gva_to_ipa(gva, &ipa);
> + if ( rc < 0 )
> + return rc;
> +
> + /*
> + * We do this first as this is faster in the default case when no
> + * permission is set on the page.
> + */
> + rc = p2m_get_mem_access(current->domain, paddr_to_pfn(ipa), &xma);
> + if ( rc < 0 )
> + return rc;
> +
> + /* Let's check if mem_access limited the access. */
> + switch ( xma )
> + {
> + default:
> + case XENMEM_access_rwx:
> + case XENMEM_access_rw:
If I've understood correctly then this is deliberately backwards
looking.
i.e. if we have gotten here then we have failed an earlier
get_page_from_gva check, so if xma is XENMEM_access_rwx that means that
the result of that first get_page_from_gva is valid because memaccess
hasn't been messing with the permissions.
Since this interface only does reads or writes and not executableness
rwx and rw are effectively the same.
Is my understanding correct?
> + return -EFAULT;
> + case XENMEM_access_n2rwx:
> + case XENMEM_access_n:
> + case XENMEM_access_x:
> + break;
If xenaccess contains no rw perms at all then we need to check what the
original p2m type was in order to decide what to do.
> + case XENMEM_access_wx:
> + case XENMEM_access_w:
> + if ( flag == GV2M_READ )
> + break;
> + else return -EFAULT;
If thing was a read then it might be because of xenaccess, so we must
check, but if it was a write then the origianl get_page_from_gva fault
was correct.
> + case XENMEM_access_rx2rw:
> + case XENMEM_access_rx:
> + case XENMEM_access_r:
> + if ( flag == GV2M_WRITE )
> + break;
> + else return -EFAULT;
and vice versa.
(sorry to be so tedious, I wanted to work through them all to ensure I'd
understood correctly and that they were right).
> @@ -68,7 +180,10 @@ unsigned long raw_clear_guest(void *to, unsigned len)
>
> page = get_page_from_gva(current->domain, (vaddr_t) to, GV2M_WRITE);
> if ( page == NULL )
> - return len;
> + {
> + if ( check_mem_access((vaddr_t) to, GV2M_WRITE, &page) < 0 )
> + return len;
> + }
I think this would be better done by a making check_mem_access include
the initial call to get_page_from_gva and calling that helper here
instead instead of repeating this code.
I'd even consider putting the memaccess check as a call at the tail end
of get_page_from_gva, I don't think there are any callers who don't want
this behaviour.
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-12 12:08 ` Ian Campbell
@ 2015-03-12 12:31 ` Tamas K Lengyel
0 siblings, 0 replies; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-12 12:31 UTC (permalink / raw)
To: Ian Campbell
Cc: wei.liu2, Stefano Stabellini, Ian Jackson, Julien Grall,
Tim Deegan, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 5045 bytes --]
On Thu, Mar 12, 2015 at 1:08 PM, Ian Campbell <ian.campbell@citrix.com>
wrote:
> On Fri, 2015-03-06 at 22:24 +0100, Tamas K Lengyel wrote:
> > The guestcopy helpers use the MMU to verify that the given guest has
> read/write
> > access to a given page during hypercalls. As we may have custom
> mem_access
> > permissions set on these pages, we do a software-based type checking in
> case
> > the MMU based approach failed, but only if mem_access_enabled is set.
> >
> > These memory accesses are not forwarded to the mem_event listener.
> Accesses
> > performed by the hypervisor are currently not part of the mem_access
> scheme.
> > This is consistent behaviour with the x86 side as well.
> >
> > Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
> > ---
> > v12: - Check for mfn_valid as well.
> > ---
> > xen/arch/arm/guestcopy.c | 124
> +++++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 121 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> > index 7dbaeca..d42a469 100644
> > --- a/xen/arch/arm/guestcopy.c
> > +++ b/xen/arch/arm/guestcopy.c
> > @@ -6,6 +6,115 @@
> >
> > #include <asm/mm.h>
> > #include <asm/guest_access.h>
> > +#include <asm/p2m.h>
> > +
> > +/*
> > + * If mem_access is in use it might have been the reason why
> get_page_from_gva
> > + * failed to fetch the page, as it uses the MMU for the permission
> checking.
> > + * Only in these cases we do a software-based type check and fetch the
> page if
> > + * we indeed found a conflicting mem_access setting.
> > + */
> > +static int check_type_get_page(vaddr_t gva, unsigned long flag,
> > + struct page_info** page)
> > +{
> > + long rc;
> > + paddr_t ipa;
> > + unsigned long maddr;
> > + unsigned long mfn;
> > + xenmem_access_t xma;
> > + p2m_type_t t;
> > +
> > + rc = gva_to_ipa(gva, &ipa);
> > + if ( rc < 0 )
> > + return rc;
> > +
> > + /*
> > + * We do this first as this is faster in the default case when no
> > + * permission is set on the page.
> > + */
> > + rc = p2m_get_mem_access(current->domain, paddr_to_pfn(ipa), &xma);
> > + if ( rc < 0 )
> > + return rc;
> > +
> > + /* Let's check if mem_access limited the access. */
> > + switch ( xma )
> > + {
> > + default:
> > + case XENMEM_access_rwx:
> > + case XENMEM_access_rw:
>
> If I've understood correctly then this is deliberately backwards
> looking.
>
> i.e. if we have gotten here then we have failed an earlier
> get_page_from_gva check, so if xma is XENMEM_access_rwx that means that
> the result of that first get_page_from_gva is valid because memaccess
> hasn't been messing with the permissions.
>
> Since this interface only does reads or writes and not executableness
> rwx and rw are effectively the same.
>
> Is my understanding correct?
>
Yes, correct. If mem_access had no r/w restriction on this page, the result
of get_page_from_gva is valid.
>
> > + return -EFAULT;
> > + case XENMEM_access_n2rwx:
> > + case XENMEM_access_n:
> > + case XENMEM_access_x:
> > + break;
>
> If xenaccess contains no rw perms at all then we need to check what the
> original p2m type was in order to decide what to do.
>
Correct.
>
> > + case XENMEM_access_wx:
> > + case XENMEM_access_w:
> > + if ( flag == GV2M_READ )
> > + break;
> > + else return -EFAULT;
>
> If thing was a read then it might be because of xenaccess, so we must
> check, but if it was a write then the origianl get_page_from_gva fault
> was correct.
>
Correct, mem_access here had a write restriction but get_page_from_gva
faulted with read, so that fault was correct.
>
>
> > + case XENMEM_access_rx2rw:
> > + case XENMEM_access_rx:
> > + case XENMEM_access_r:
> > + if ( flag == GV2M_WRITE )
> > + break;
> > + else return -EFAULT;
>
> and vice versa.
>
> (sorry to be so tedious, I wanted to work through them all to ensure I'd
> understood correctly and that they were right).
>
Yes, this is a bit complex but your understanding is correct. I will add a
comment describing it in human readable way as Julien also had the same
question couple months ago.
>
> > @@ -68,7 +180,10 @@ unsigned long raw_clear_guest(void *to, unsigned len)
> >
> > page = get_page_from_gva(current->domain, (vaddr_t) to,
> GV2M_WRITE);
> > if ( page == NULL )
> > - return len;
> > + {
> > + if ( check_mem_access((vaddr_t) to, GV2M_WRITE, &page) < 0 )
> > + return len;
> > + }
>
> I think this would be better done by a making check_mem_access include
> the initial call to get_page_from_gva and calling that helper here
> instead instead of repeating this code.
>
> I'd even consider putting the memaccess check as a call at the tail end
> of get_page_from_gva, I don't think there are any callers who don't want
> this behaviour.
>
Sure, that sounds reasonable.
Thanks,
Tamas
[-- Attachment #1.2: Type: text/html, Size: 7141 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-06 21:24 ` [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages Tamas K Lengyel
2015-03-12 12:08 ` Ian Campbell
@ 2015-03-12 13:24 ` Julien Grall
2015-03-12 13:38 ` Tamas K Lengyel
2015-03-12 13:50 ` Julien Grall
2 siblings, 1 reply; 71+ messages in thread
From: Julien Grall @ 2015-03-12 13:24 UTC (permalink / raw)
To: Tamas K Lengyel, xen-devel
Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson, tim,
stefano.stabellini, jbeulich, keir
Hi Tamas,
On 06/03/15 21:24, Tamas K Lengyel wrote:
> The guestcopy helpers use the MMU to verify that the given guest has read/write
> access to a given page during hypercalls. As we may have custom mem_access
> permissions set on these pages, we do a software-based type checking in case
> the MMU based approach failed, but only if mem_access_enabled is set.
>
> These memory accesses are not forwarded to the mem_event listener. Accesses
> performed by the hypervisor are currently not part of the mem_access scheme.
> This is consistent behaviour with the x86 side as well.
>
> Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
> ---
> v12: - Check for mfn_valid as well.
> ---
> xen/arch/arm/guestcopy.c | 124 +++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 121 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index 7dbaeca..d42a469 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -6,6 +6,115 @@
>
> #include <asm/mm.h>
> #include <asm/guest_access.h>
> +#include <asm/p2m.h>
> +
> +/*
> + * If mem_access is in use it might have been the reason why get_page_from_gva
> + * failed to fetch the page, as it uses the MMU for the permission checking.
> + * Only in these cases we do a software-based type check and fetch the page if
> + * we indeed found a conflicting mem_access setting.
> + */
> +static int check_type_get_page(vaddr_t gva, unsigned long flag,
> + struct page_info** page)
> +{
> + long rc;
AFAICT, all the return value stored in rc are int.
> + paddr_t ipa;
> + unsigned long maddr;
> + unsigned long mfn;
> + xenmem_access_t xma;
> + p2m_type_t t;
> +
> + rc = gva_to_ipa(gva, &ipa);
> + if ( rc < 0 )
> + return rc;
> +
> + /*
> + * We do this first as this is faster in the default case when no
> + * permission is set on the page.
> + */
> + rc = p2m_get_mem_access(current->domain, paddr_to_pfn(ipa), &xma);
> + if ( rc < 0 )
Maybe a likely would be good here?
> + return rc;
> +
> + /* Let's check if mem_access limited the access. */
> + switch ( xma )
> + {
> + default:
> + case XENMEM_access_rwx:
> + case XENMEM_access_rw:
> + return -EFAULT;
> + case XENMEM_access_n2rwx:
> + case XENMEM_access_n:
> + case XENMEM_access_x:
> + break;
> + case XENMEM_access_wx:
> + case XENMEM_access_w:
> + if ( flag == GV2M_READ )
> + break;
> + else return -EFAULT;
> + case XENMEM_access_rx2rw:
> + case XENMEM_access_rx:
> + case XENMEM_access_r:
> + if ( flag == GV2M_WRITE )
> + break;
> + else return -EFAULT;
> + }
> +
> + /*
> + * We had a mem_access permission limiting the access, but the page type
> + * could also be limiting, so we need to check that as well.
> + */
> + maddr = p2m_lookup(current->domain, ipa, &t);
> + if ( maddr == INVALID_PADDR )
> + return -EFAULT;
> +
> + mfn = maddr >> PAGE_SHIFT;
> + if ( !mfn_valid(mfn) )
> + return -EFAULT;
> +
> + /*
> + * All page types are readable so we only have to check the
> + * type if writing.
> + */
That's may change in the future. A white-list may be better in order to
avoid giving wrong access when a new p2m type is added.
> + if ( flag == GV2M_WRITE )
> + {
> + switch ( t )
> + {
> + case p2m_ram_rw:
> + case p2m_iommu_map_rw:
> + case p2m_map_foreign:
> + case p2m_grant_map_rw:
> + case p2m_mmio_direct:
We disallow guest copy from the above 4 types via get_page_from_gva. So
I'm not sure if it's worth to check them here.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-12 13:24 ` Julien Grall
@ 2015-03-12 13:38 ` Tamas K Lengyel
2015-03-12 13:43 ` Julien Grall
0 siblings, 1 reply; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-12 13:38 UTC (permalink / raw)
To: Julien Grall
Cc: wei.liu2, Ian Campbell, Stefano Stabellini, Tim Deegan,
Ian Jackson, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 4413 bytes --]
On Thu, Mar 12, 2015 at 2:24 PM, Julien Grall <julien.grall@linaro.org>
wrote:
> Hi Tamas,
>
> On 06/03/15 21:24, Tamas K Lengyel wrote:
> > The guestcopy helpers use the MMU to verify that the given guest has
> read/write
> > access to a given page during hypercalls. As we may have custom
> mem_access
> > permissions set on these pages, we do a software-based type checking in
> case
> > the MMU based approach failed, but only if mem_access_enabled is set.
> >
> > These memory accesses are not forwarded to the mem_event listener.
> Accesses
> > performed by the hypervisor are currently not part of the mem_access
> scheme.
> > This is consistent behaviour with the x86 side as well.
> >
> > Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
> > ---
> > v12: - Check for mfn_valid as well.
> > ---
> > xen/arch/arm/guestcopy.c | 124
> +++++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 121 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> > index 7dbaeca..d42a469 100644
> > --- a/xen/arch/arm/guestcopy.c
> > +++ b/xen/arch/arm/guestcopy.c
> > @@ -6,6 +6,115 @@
> >
> > #include <asm/mm.h>
> > #include <asm/guest_access.h>
> > +#include <asm/p2m.h>
> > +
> > +/*
> > + * If mem_access is in use it might have been the reason why
> get_page_from_gva
> > + * failed to fetch the page, as it uses the MMU for the permission
> checking.
> > + * Only in these cases we do a software-based type check and fetch the
> page if
> > + * we indeed found a conflicting mem_access setting.
> > + */
> > +static int check_type_get_page(vaddr_t gva, unsigned long flag,
> > + struct page_info** page)
> > +{
> > + long rc;
>
> AFAICT, all the return value stored in rc are int.
>
Ack. I actually relocated this function into p2m.c in the next iteration of
the series and change the return type to struct page_info* page.
>
> > + paddr_t ipa;
> > + unsigned long maddr;
> > + unsigned long mfn;
> > + xenmem_access_t xma;
> > + p2m_type_t t;
> > +
> > + rc = gva_to_ipa(gva, &ipa);
> > + if ( rc < 0 )
> > + return rc;
> > +
> > + /*
> > + * We do this first as this is faster in the default case when no
> > + * permission is set on the page.
> > + */
> > + rc = p2m_get_mem_access(current->domain, paddr_to_pfn(ipa), &xma);
> > + if ( rc < 0 )
>
> Maybe a likely would be good here?
>
Sure.
>
> > + return rc;
> > +
> > + /* Let's check if mem_access limited the access. */
> > + switch ( xma )
> > + {
> > + default:
> > + case XENMEM_access_rwx:
> > + case XENMEM_access_rw:
> > + return -EFAULT;
> > + case XENMEM_access_n2rwx:
> > + case XENMEM_access_n:
> > + case XENMEM_access_x:
> > + break;
> > + case XENMEM_access_wx:
> > + case XENMEM_access_w:
> > + if ( flag == GV2M_READ )
> > + break;
> > + else return -EFAULT;
> > + case XENMEM_access_rx2rw:
> > + case XENMEM_access_rx:
> > + case XENMEM_access_r:
> > + if ( flag == GV2M_WRITE )
> > + break;
> > + else return -EFAULT;
> > + }
> > +
> > + /*
> > + * We had a mem_access permission limiting the access, but the page
> type
> > + * could also be limiting, so we need to check that as well.
> > + */
> > + maddr = p2m_lookup(current->domain, ipa, &t);
> > + if ( maddr == INVALID_PADDR )
> > + return -EFAULT;
> > +
> > + mfn = maddr >> PAGE_SHIFT;
> > + if ( !mfn_valid(mfn) )
> > + return -EFAULT;
> > +
> > + /*
> > + * All page types are readable so we only have to check the
> > + * type if writing.
> > + */
>
> That's may change in the future. A white-list may be better in order to
> avoid giving wrong access when a new p2m type is added.
>
Hm, sure.
>
> > + if ( flag == GV2M_WRITE )
> > + {
> > + switch ( t )
> > + {
> > + case p2m_ram_rw:
>
> > + case p2m_iommu_map_rw:
> > + case p2m_map_foreign:
> > + case p2m_grant_map_rw:
> > + case p2m_mmio_direct:
>
> We disallow guest copy from the above 4 types via get_page_from_gva. So
> I'm not sure if it's worth to check them here.
>
You mean get_page_from_gva only works for p2m_ram_rw type? Is this the case
for GV2M_READ as well?
>
> Regards,
>
> --
> Julien Grall
>
Thanks,
Tamas
[-- Attachment #1.2: Type: text/html, Size: 6653 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-12 13:38 ` Tamas K Lengyel
@ 2015-03-12 13:43 ` Julien Grall
2015-03-12 14:33 ` Tamas K Lengyel
0 siblings, 1 reply; 71+ messages in thread
From: Julien Grall @ 2015-03-12 13:43 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, Ian Campbell, Stefano Stabellini, Tim Deegan,
Ian Jackson, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
On 12/03/15 13:38, Tamas K Lengyel wrote:
> > + if ( flag == GV2M_WRITE )
> > + {
> > + switch ( t )
> > + {
> > + case p2m_ram_rw:
>
> > + case p2m_iommu_map_rw:
> > + case p2m_map_foreign:
> > + case p2m_grant_map_rw:
> > + case p2m_mmio_direct:
>
> We disallow guest copy from the above 4 types via get_page_from_gva. So
> I'm not sure if it's worth to check them here.
>
>
> You mean get_page_from_gva only works for p2m_ram_rw type? Is this the
> case for GV2M_READ as well?
Yes, currently p2m_ram_rw is the only type bound to a struct page.
--
Julien Grall
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-12 13:43 ` Julien Grall
@ 2015-03-12 14:33 ` Tamas K Lengyel
0 siblings, 0 replies; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-12 14:33 UTC (permalink / raw)
To: Julien Grall
Cc: wei.liu2@citrix.com, Ian Campbell, Stefano Stabellini, Tim Deegan,
Ian Jackson, xen-devel@lists.xen.org, Stefano Stabellini,
Jan Beulich, Keir Fraser, Tamas K Lengyel
On Thu, Mar 12, 2015 at 2:43 PM, Julien Grall <julien.grall@linaro.org> wrote:
> On 12/03/15 13:38, Tamas K Lengyel wrote:
>> > + if ( flag == GV2M_WRITE )
>> > + {
>> > + switch ( t )
>> > + {
>> > + case p2m_ram_rw:
>>
>> > + case p2m_iommu_map_rw:
>> > + case p2m_map_foreign:
>> > + case p2m_grant_map_rw:
>> > + case p2m_mmio_direct:
>>
>> We disallow guest copy from the above 4 types via get_page_from_gva. So
>> I'm not sure if it's worth to check them here.
>>
>>
>> You mean get_page_from_gva only works for p2m_ram_rw type? Is this the
>> case for GV2M_READ as well?
>
> Yes, currently p2m_ram_rw is the only type bound to a struct page.
That makes the white-list quite simple, t has to be p2m_ram_rw
regardless of the flag, otherwise fault.
>
> --
> Julien Grall
>
Thanks,
Tamas
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-06 21:24 ` [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages Tamas K Lengyel
2015-03-12 12:08 ` Ian Campbell
2015-03-12 13:24 ` Julien Grall
@ 2015-03-12 13:50 ` Julien Grall
2015-03-12 14:13 ` Tamas K Lengyel
2 siblings, 1 reply; 71+ messages in thread
From: Julien Grall @ 2015-03-12 13:50 UTC (permalink / raw)
To: Tamas K Lengyel, xen-devel
Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson, tim,
stefano.stabellini, jbeulich, keir
Hi Tamas,
On 06/03/15 21:24, Tamas K Lengyel wrote:
> +/*
> + * If mem_access is in use it might have been the reason why get_page_from_gva
> + * failed to fetch the page, as it uses the MMU for the permission checking.
> + * Only in these cases we do a software-based type check and fetch the page if
> + * we indeed found a conflicting mem_access setting.
> + */
> +static int check_type_get_page(vaddr_t gva, unsigned long flag,
> + struct page_info** page)
> +{
> + long rc;
> + paddr_t ipa;
> + unsigned long maddr;
> + unsigned long mfn;
> + xenmem_access_t xma;
> + p2m_type_t t;
> +
> + rc = gva_to_ipa(gva, &ipa);
I though a bit more about this call.
gva_to_ipa only checks if the mapping has read-permission. That would
allow a guest to write on read-only mapping.
You have to pass the flags to gva_to_ipa in order to avoid
re-introducing XSA-98 [1]
Regards,
[1] http://xenbits.xen.org/xsa/advisory-98.html
--
Julien Grall
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-12 13:50 ` Julien Grall
@ 2015-03-12 14:13 ` Tamas K Lengyel
2015-03-12 14:52 ` Julien Grall
0 siblings, 1 reply; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-12 14:13 UTC (permalink / raw)
To: Julien Grall
Cc: wei.liu2, Ian Campbell, Stefano Stabellini, Tim Deegan,
Ian Jackson, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 1516 bytes --]
On Thu, Mar 12, 2015 at 2:50 PM, Julien Grall <julien.grall@linaro.org>
wrote:
> Hi Tamas,
>
> On 06/03/15 21:24, Tamas K Lengyel wrote:
> > +/*
> > + * If mem_access is in use it might have been the reason why
> get_page_from_gva
> > + * failed to fetch the page, as it uses the MMU for the permission
> checking.
> > + * Only in these cases we do a software-based type check and fetch the
> page if
> > + * we indeed found a conflicting mem_access setting.
> > + */
> > +static int check_type_get_page(vaddr_t gva, unsigned long flag,
> > + struct page_info** page)
> > +{
> > + long rc;
> > + paddr_t ipa;
> > + unsigned long maddr;
> > + unsigned long mfn;
> > + xenmem_access_t xma;
> > + p2m_type_t t;
> > +
> > + rc = gva_to_ipa(gva, &ipa);
>
> I though a bit more about this call.
>
> gva_to_ipa only checks if the mapping has read-permission. That would
> allow a guest to write on read-only mapping.
>
> You have to pass the flags to gva_to_ipa in order to avoid
> re-introducing XSA-98 [1]
>
Here I really just care if the mapping exist to see if we have a mem_access
restriction, r/w permission checking is then performed afterwards by
checking the page type. If there are additional restrictions on the page
beside the type, those certainly should be added. Can you point me to where
that additional restriction is stored so I can query for it?
> Regards,
>
> [1] http://xenbits.xen.org/xsa/advisory-98.html
>
> --
> Julien Grall
>
Thanks,
Tamas
[-- Attachment #1.2: Type: text/html, Size: 2496 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-12 14:13 ` Tamas K Lengyel
@ 2015-03-12 14:52 ` Julien Grall
2015-03-12 15:27 ` Ian Campbell
0 siblings, 1 reply; 71+ messages in thread
From: Julien Grall @ 2015-03-12 14:52 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, Ian Campbell, Stefano Stabellini, Tim Deegan,
Ian Jackson, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
On 12/03/15 14:13, Tamas K Lengyel wrote:
>
>
> On Thu, Mar 12, 2015 at 2:50 PM, Julien Grall <julien.grall@linaro.org
> <mailto:julien.grall@linaro.org>> wrote:
>
> Hi Tamas,
>
> On 06/03/15 21:24, Tamas K Lengyel wrote:
> > +/*
> > + * If mem_access is in use it might have been the reason why get_page_from_gva
> > + * failed to fetch the page, as it uses the MMU for the permission checking.
> > + * Only in these cases we do a software-based type check and fetch the page if
> > + * we indeed found a conflicting mem_access setting.
> > + */
> > +static int check_type_get_page(vaddr_t gva, unsigned long flag,
> > + struct page_info** page)
> > +{
> > + long rc;
> > + paddr_t ipa;
> > + unsigned long maddr;
> > + unsigned long mfn;
> > + xenmem_access_t xma;
> > + p2m_type_t t;
> > +
> > + rc = gva_to_ipa(gva, &ipa);
>
> I though a bit more about this call.
>
> gva_to_ipa only checks if the mapping has read-permission. That would
> allow a guest to write on read-only mapping.
>
>
> You have to pass the flags to gva_to_ipa in order to avoid
> re-introducing XSA-98 [1]
>
>
> Here I really just care if the mapping exist to see if we have a
> mem_access restriction, r/w permission checking is then performed
> afterwards by checking the page type. If there are additional
> restrictions on the page beside the type, those certainly should be
> added. Can you point me to where that additional restriction is stored
> so I can query for it?
The R/W permission checking done afterwards is not enough.
What does get_page_from_gva is:
1) Check the permission on Stage 1 page table (controlled by the guest
and translate VA -> IPA)
2) Check the permission on Stage 2 page table (controlled by the
hypervisor and translate IPA -> PA).
get_page_from_gva may fail because of 1), which is not related to memaccess.
Currently, check_type_get_page emulate only the check for 2). So you may
end up to allow Xen writing in read-only mapping (from the Stage 1 POV).
This was XSA-98.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-12 14:52 ` Julien Grall
@ 2015-03-12 15:27 ` Ian Campbell
2015-03-12 15:40 ` Julien Grall
2015-03-12 15:41 ` Tamas K Lengyel
0 siblings, 2 replies; 71+ messages in thread
From: Ian Campbell @ 2015-03-12 15:27 UTC (permalink / raw)
To: Julien Grall
Cc: wei.liu2, Stefano Stabellini, Tim Deegan, Ian Jackson, xen-devel,
stefano.stabellini, Jan Beulich, Keir Fraser, Tamas K Lengyel
On Thu, 2015-03-12 at 14:52 +0000, Julien Grall wrote:
> On 12/03/15 14:13, Tamas K Lengyel wrote:
> >
> >
> > On Thu, Mar 12, 2015 at 2:50 PM, Julien Grall <julien.grall@linaro.org
> > <mailto:julien.grall@linaro.org>> wrote:
> >
> > Hi Tamas,
> >
> > On 06/03/15 21:24, Tamas K Lengyel wrote:
> > > +/*
> > > + * If mem_access is in use it might have been the reason why get_page_from_gva
> > > + * failed to fetch the page, as it uses the MMU for the permission checking.
> > > + * Only in these cases we do a software-based type check and fetch the page if
> > > + * we indeed found a conflicting mem_access setting.
> > > + */
> > > +static int check_type_get_page(vaddr_t gva, unsigned long flag,
> > > + struct page_info** page)
> > > +{
> > > + long rc;
> > > + paddr_t ipa;
> > > + unsigned long maddr;
> > > + unsigned long mfn;
> > > + xenmem_access_t xma;
> > > + p2m_type_t t;
> > > +
> > > + rc = gva_to_ipa(gva, &ipa);
> >
> > I though a bit more about this call.
> >
> > gva_to_ipa only checks if the mapping has read-permission. That would
> > allow a guest to write on read-only mapping.
> >
> >
> > You have to pass the flags to gva_to_ipa in order to avoid
> > re-introducing XSA-98 [1]
> >
> >
> > Here I really just care if the mapping exist to see if we have a
> > mem_access restriction, r/w permission checking is then performed
> > afterwards by checking the page type. If there are additional
> > restrictions on the page beside the type, those certainly should be
> > added. Can you point me to where that additional restriction is stored
> > so I can query for it?
>
> The R/W permission checking done afterwards is not enough.
>
> What does get_page_from_gva is:
> 1) Check the permission on Stage 1 page table (controlled by the guest
> and translate VA -> IPA)
> 2) Check the permission on Stage 2 page table (controlled by the
> hypervisor and translate IPA -> PA).
>
> get_page_from_gva may fail because of 1), which is not related to memaccess.
>
> Currently, check_type_get_page emulate only the check for 2). So you may
> end up to allow Xen writing in read-only mapping (from the Stage 1 POV).
> This was XSA-98.
XSA-98 was purely about stage-2 permissions (e.g. read-only grants). The
fact that the resulting patch also checks stage-1 permissions is not a
security property AFAICT.
Ian.
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-12 15:27 ` Ian Campbell
@ 2015-03-12 15:40 ` Julien Grall
2015-03-12 15:44 ` Tamas K Lengyel
2015-03-12 15:54 ` Ian Campbell
2015-03-12 15:41 ` Tamas K Lengyel
1 sibling, 2 replies; 71+ messages in thread
From: Julien Grall @ 2015-03-12 15:40 UTC (permalink / raw)
To: Ian Campbell
Cc: wei.liu2, Stefano Stabellini, Tim Deegan, Ian Jackson, xen-devel,
stefano.stabellini, Jan Beulich, Keir Fraser, Tamas K Lengyel
Hi Ian,
On 12/03/15 15:27, Ian Campbell wrote:
>> Currently, check_type_get_page emulate only the check for 2). So you may
>> end up to allow Xen writing in read-only mapping (from the Stage 1 POV).
>> This was XSA-98.
>
> XSA-98 was purely about stage-2 permissions (e.g. read-only grants). The
> fact that the resulting patch also checks stage-1 permissions is not a
> security property AFAICT.
XSA-98 was for both... Without checking stage-1 permission a userspace
which can issue an hypercall may be able to write into read-only kernel
space. Whoops.
Though it doesn't every possibility...
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-12 15:40 ` Julien Grall
@ 2015-03-12 15:44 ` Tamas K Lengyel
2015-03-12 15:56 ` Ian Campbell
2015-03-12 15:54 ` Ian Campbell
1 sibling, 1 reply; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-12 15:44 UTC (permalink / raw)
To: Julien Grall
Cc: wei.liu2, Ian Campbell, Stefano Stabellini, Tim Deegan,
Ian Jackson, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 786 bytes --]
On Thu, Mar 12, 2015 at 4:40 PM, Julien Grall <julien.grall@linaro.org>
wrote:
> Hi Ian,
>
> On 12/03/15 15:27, Ian Campbell wrote:
> >> Currently, check_type_get_page emulate only the check for 2). So you may
> >> end up to allow Xen writing in read-only mapping (from the Stage 1 POV).
> >> This was XSA-98.
> >
> > XSA-98 was purely about stage-2 permissions (e.g. read-only grants). The
> > fact that the resulting patch also checks stage-1 permissions is not a
> > security property AFAICT.
>
> XSA-98 was for both... Without checking stage-1 permission a userspace
> which can issue an hypercall may be able to write into read-only kernel
> space. Whoops.
>
Userspace is able to issue hypercall?
>
> Though it doesn't every possibility...
>
> Regards,
>
> --
> Julien Grall
>
[-- Attachment #1.2: Type: text/html, Size: 1421 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-12 15:44 ` Tamas K Lengyel
@ 2015-03-12 15:56 ` Ian Campbell
2015-03-12 16:02 ` Tamas K Lengyel
0 siblings, 1 reply; 71+ messages in thread
From: Ian Campbell @ 2015-03-12 15:56 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, Stefano Stabellini, Ian Jackson, Julien Grall,
Tim Deegan, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
On Thu, 2015-03-12 at 16:44 +0100, Tamas K Lengyel wrote:
>
>
> On Thu, Mar 12, 2015 at 4:40 PM, Julien Grall
> <julien.grall@linaro.org> wrote:
> Hi Ian,
>
> On 12/03/15 15:27, Ian Campbell wrote:
> >> Currently, check_type_get_page emulate only the check for
> 2). So you may
> >> end up to allow Xen writing in read-only mapping (from the
> Stage 1 POV).
> >> This was XSA-98.
> >
> > XSA-98 was purely about stage-2 permissions (e.g. read-only
> grants). The
> > fact that the resulting patch also checks stage-1
> permissions is not a
> > security property AFAICT.
>
> XSA-98 was for both... Without checking stage-1 permission a
> userspace
> which can issue an hypercall may be able to write into
> read-only kernel
> space. Whoops.
>
>
> Userspace is able to issue hypercall?
Via ioctls on /proc/xen/privcmd, yes. It's how the toolstack talks to
Xen...
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-12 15:56 ` Ian Campbell
@ 2015-03-12 16:02 ` Tamas K Lengyel
2015-03-12 16:48 ` Ian Campbell
0 siblings, 1 reply; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-12 16:02 UTC (permalink / raw)
To: Ian Campbell
Cc: wei.liu2, Stefano Stabellini, Ian Jackson, Julien Grall,
Tim Deegan, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 1280 bytes --]
On Thu, Mar 12, 2015 at 4:56 PM, Ian Campbell <ian.campbell@citrix.com>
wrote:
> On Thu, 2015-03-12 at 16:44 +0100, Tamas K Lengyel wrote:
> >
> >
> > On Thu, Mar 12, 2015 at 4:40 PM, Julien Grall
> > <julien.grall@linaro.org> wrote:
> > Hi Ian,
> >
> > On 12/03/15 15:27, Ian Campbell wrote:
> > >> Currently, check_type_get_page emulate only the check for
> > 2). So you may
> > >> end up to allow Xen writing in read-only mapping (from the
> > Stage 1 POV).
> > >> This was XSA-98.
> > >
> > > XSA-98 was purely about stage-2 permissions (e.g. read-only
> > grants). The
> > > fact that the resulting patch also checks stage-1
> > permissions is not a
> > > security property AFAICT.
> >
> > XSA-98 was for both... Without checking stage-1 permission a
> > userspace
> > which can issue an hypercall may be able to write into
> > read-only kernel
> > space. Whoops.
> >
> >
> > Userspace is able to issue hypercall?
>
> Via ioctls on /proc/xen/privcmd, yes. It's how the toolstack talks to
> Xen...
>
Well, that is not the userspace issuing the hypercall, its a kernel module
issuing the hypercall on behalf of a process ;)
Tamas
[-- Attachment #1.2: Type: text/html, Size: 2024 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-12 16:02 ` Tamas K Lengyel
@ 2015-03-12 16:48 ` Ian Campbell
2015-03-12 16:55 ` Tamas K Lengyel
0 siblings, 1 reply; 71+ messages in thread
From: Ian Campbell @ 2015-03-12 16:48 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, Stefano Stabellini, Ian Jackson, Julien Grall,
Tim Deegan, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
On Thu, 2015-03-12 at 17:02 +0100, Tamas K Lengyel wrote:
>
>
> On Thu, Mar 12, 2015 at 4:56 PM, Ian Campbell
> <ian.campbell@citrix.com> wrote:
> On Thu, 2015-03-12 at 16:44 +0100, Tamas K Lengyel wrote:
> >
> >
> > On Thu, Mar 12, 2015 at 4:40 PM, Julien Grall
> > <julien.grall@linaro.org> wrote:
> > Hi Ian,
> >
> > On 12/03/15 15:27, Ian Campbell wrote:
> > >> Currently, check_type_get_page emulate only the
> check for
> > 2). So you may
> > >> end up to allow Xen writing in read-only mapping
> (from the
> > Stage 1 POV).
> > >> This was XSA-98.
> > >
> > > XSA-98 was purely about stage-2 permissions (e.g.
> read-only
> > grants). The
> > > fact that the resulting patch also checks stage-1
> > permissions is not a
> > > security property AFAICT.
> >
> > XSA-98 was for both... Without checking stage-1
> permission a
> > userspace
> > which can issue an hypercall may be able to write
> into
> > read-only kernel
> > space. Whoops.
> >
> >
> > Userspace is able to issue hypercall?
>
>
> Via ioctls on /proc/xen/privcmd, yes. It's how the toolstack
> talks to
> Xen...
>
>
> Well, that is not the userspace issuing the hypercall, its a kernel
> module issuing the hypercall on behalf of a process ;)
But the vaddrs etc in there are userspace controlled and the kernel does
not validate them.
Ian.
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-12 16:48 ` Ian Campbell
@ 2015-03-12 16:55 ` Tamas K Lengyel
0 siblings, 0 replies; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-12 16:55 UTC (permalink / raw)
To: Ian Campbell
Cc: wei.liu2, Stefano Stabellini, Ian Jackson, Julien Grall,
Tim Deegan, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 2111 bytes --]
On Thu, Mar 12, 2015 at 5:48 PM, Ian Campbell <ian.campbell@citrix.com>
wrote:
> On Thu, 2015-03-12 at 17:02 +0100, Tamas K Lengyel wrote:
> >
> >
> > On Thu, Mar 12, 2015 at 4:56 PM, Ian Campbell
> > <ian.campbell@citrix.com> wrote:
> > On Thu, 2015-03-12 at 16:44 +0100, Tamas K Lengyel wrote:
> > >
> > >
> > > On Thu, Mar 12, 2015 at 4:40 PM, Julien Grall
> > > <julien.grall@linaro.org> wrote:
> > > Hi Ian,
> > >
> > > On 12/03/15 15:27, Ian Campbell wrote:
> > > >> Currently, check_type_get_page emulate only the
> > check for
> > > 2). So you may
> > > >> end up to allow Xen writing in read-only mapping
> > (from the
> > > Stage 1 POV).
> > > >> This was XSA-98.
> > > >
> > > > XSA-98 was purely about stage-2 permissions (e.g.
> > read-only
> > > grants). The
> > > > fact that the resulting patch also checks stage-1
> > > permissions is not a
> > > > security property AFAICT.
> > >
> > > XSA-98 was for both... Without checking stage-1
> > permission a
> > > userspace
> > > which can issue an hypercall may be able to write
> > into
> > > read-only kernel
> > > space. Whoops.
> > >
> > >
> > > Userspace is able to issue hypercall?
> >
> >
> > Via ioctls on /proc/xen/privcmd, yes. It's how the toolstack
> > talks to
> > Xen...
> >
> >
> > Well, that is not the userspace issuing the hypercall, its a kernel
> > module issuing the hypercall on behalf of a process ;)
>
> But the vaddrs etc in there are userspace controlled and the kernel does
> not validate them.
>
> Ian.
>
Right, it's a bit splitting hairs and my point was just that the kernel is
always in the middle and theoretically could implement input validation and
access control as well.
Tamas
[-- Attachment #1.2: Type: text/html, Size: 3349 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-12 15:40 ` Julien Grall
2015-03-12 15:44 ` Tamas K Lengyel
@ 2015-03-12 15:54 ` Ian Campbell
1 sibling, 0 replies; 71+ messages in thread
From: Ian Campbell @ 2015-03-12 15:54 UTC (permalink / raw)
To: Julien Grall
Cc: wei.liu2, Stefano Stabellini, Tim Deegan, Ian Jackson, xen-devel,
stefano.stabellini, Jan Beulich, Keir Fraser, Tamas K Lengyel
On Thu, 2015-03-12 at 15:40 +0000, Julien Grall wrote:
> Hi Ian,
>
> On 12/03/15 15:27, Ian Campbell wrote:
> >> Currently, check_type_get_page emulate only the check for 2). So you may
> >> end up to allow Xen writing in read-only mapping (from the Stage 1 POV).
> >> This was XSA-98.
> >
> > XSA-98 was purely about stage-2 permissions (e.g. read-only grants). The
> > fact that the resulting patch also checks stage-1 permissions is not a
> > security property AFAICT.
>
> XSA-98 was for both... Without checking stage-1 permission a userspace
> which can issue an hypercall may be able to write into read-only kernel
> space. Whoops.
XSA-98 doesn't make any mention of this particular attack and talks
solely about guests writing to memory they shouldn't, not processes.
A userspace which can issue a hypercall is already root and has lots of
ways to rewrite kernel memory (starting with /dev/mem).
Anyway, enough splitting hairs: it probably is worth retaining this
behaviour since it seems pretty simple, just make gva_to_ipa_par take
the same flags as gva_to_ma_par and use it in the same way.
Ian.
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-12 15:27 ` Ian Campbell
2015-03-12 15:40 ` Julien Grall
@ 2015-03-12 15:41 ` Tamas K Lengyel
2015-03-12 15:55 ` Ian Campbell
1 sibling, 1 reply; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-12 15:41 UTC (permalink / raw)
To: Ian Campbell
Cc: wei.liu2, Stefano Stabellini, Tim Deegan, Julien Grall,
Ian Jackson, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 3179 bytes --]
On Thu, Mar 12, 2015 at 4:27 PM, Ian Campbell <ian.campbell@citrix.com>
wrote:
> On Thu, 2015-03-12 at 14:52 +0000, Julien Grall wrote:
> > On 12/03/15 14:13, Tamas K Lengyel wrote:
> > >
> > >
> > > On Thu, Mar 12, 2015 at 2:50 PM, Julien Grall <julien.grall@linaro.org
> > > <mailto:julien.grall@linaro.org>> wrote:
> > >
> > > Hi Tamas,
> > >
> > > On 06/03/15 21:24, Tamas K Lengyel wrote:
> > > > +/*
> > > > + * If mem_access is in use it might have been the reason why
> get_page_from_gva
> > > > + * failed to fetch the page, as it uses the MMU for the
> permission checking.
> > > > + * Only in these cases we do a software-based type check and
> fetch the page if
> > > > + * we indeed found a conflicting mem_access setting.
> > > > + */
> > > > +static int check_type_get_page(vaddr_t gva, unsigned long flag,
> > > > + struct page_info** page)
> > > > +{
> > > > + long rc;
> > > > + paddr_t ipa;
> > > > + unsigned long maddr;
> > > > + unsigned long mfn;
> > > > + xenmem_access_t xma;
> > > > + p2m_type_t t;
> > > > +
> > > > + rc = gva_to_ipa(gva, &ipa);
> > >
> > > I though a bit more about this call.
> > >
> > > gva_to_ipa only checks if the mapping has read-permission. That
> would
> > > allow a guest to write on read-only mapping.
> > >
> > >
> > > You have to pass the flags to gva_to_ipa in order to avoid
> > > re-introducing XSA-98 [1]
> > >
> > >
> > > Here I really just care if the mapping exist to see if we have a
> > > mem_access restriction, r/w permission checking is then performed
> > > afterwards by checking the page type. If there are additional
> > > restrictions on the page beside the type, those certainly should be
> > > added. Can you point me to where that additional restriction is stored
> > > so I can query for it?
> >
> > The R/W permission checking done afterwards is not enough.
> >
> > What does get_page_from_gva is:
> > 1) Check the permission on Stage 1 page table (controlled by the
> guest
> > and translate VA -> IPA)
> > 2) Check the permission on Stage 2 page table (controlled by the
> > hypervisor and translate IPA -> PA).
> >
> > get_page_from_gva may fail because of 1), which is not related to
> memaccess.
> >
> > Currently, check_type_get_page emulate only the check for 2). So you may
> > end up to allow Xen writing in read-only mapping (from the Stage 1 POV).
> > This was XSA-98.
>
> XSA-98 was purely about stage-2 permissions (e.g. read-only grants). The
> fact that the resulting patch also checks stage-1 permissions is not a
> security property AFAICT.
>
> Ian.
>
We could check stage-1 permissions by walking the guest page tables and
looking at the pte permissions, however I'm not aware of having this
function implemented within Xen. I have it implemented in LibVMI (
https://github.com/libvmi/libvmi/blob/master/libvmi/arch/arm_aarch32.c#L81)
so I could potentially port it if necessary. However, since the in-guest
permissions are in control of the guest to begin with, I don't see any
added security benefit in doing so.
Tamas
[-- Attachment #1.2: Type: text/html, Size: 4513 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-12 15:41 ` Tamas K Lengyel
@ 2015-03-12 15:55 ` Ian Campbell
2015-03-12 16:10 ` Tamas K Lengyel
0 siblings, 1 reply; 71+ messages in thread
From: Ian Campbell @ 2015-03-12 15:55 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, Stefano Stabellini, Tim Deegan, Julien Grall,
Ian Jackson, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
On Thu, 2015-03-12 at 16:41 +0100, Tamas K Lengyel wrote:
> We could check stage-1 permissions by walking the guest page tables
> and looking at the pte permissions, however I'm not aware of having
> this function implemented within Xen.
The h/w can do this for you, you just need to arrange for gva_to_ipa_par
to select between ATS1CPR and ATS1CPW like gva_to_ma_par does.
Ian.
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages
2015-03-12 15:55 ` Ian Campbell
@ 2015-03-12 16:10 ` Tamas K Lengyel
0 siblings, 0 replies; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-12 16:10 UTC (permalink / raw)
To: Ian Campbell
Cc: wei.liu2, Stefano Stabellini, Tim Deegan, Julien Grall,
Ian Jackson, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 512 bytes --]
On Thu, Mar 12, 2015 at 4:55 PM, Ian Campbell <ian.campbell@citrix.com>
wrote:
> On Thu, 2015-03-12 at 16:41 +0100, Tamas K Lengyel wrote:
>
> > We could check stage-1 permissions by walking the guest page tables
> > and looking at the pte permissions, however I'm not aware of having
> > this function implemented within Xen.
>
> The h/w can do this for you, you just need to arrange for gva_to_ipa_par
> to select between ATS1CPR and ATS1CPW like gva_to_ma_par does.
>
> Ian.
>
Thanks, that's simple.
Tamas
[-- Attachment #1.2: Type: text/html, Size: 986 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events.
2015-03-06 21:24 [PATCH V13 0/7] Mem_access for ARM Tamas K Lengyel
` (2 preceding siblings ...)
2015-03-06 21:24 ` [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages Tamas K Lengyel
@ 2015-03-06 21:24 ` Tamas K Lengyel
2015-03-12 13:35 ` Ian Campbell
2015-03-12 15:13 ` Julien Grall
2015-03-06 21:24 ` [PATCH V13 5/7] xen/arm: Instruction prefetch abort (X) mem_event handling Tamas K Lengyel
` (3 subsequent siblings)
7 siblings, 2 replies; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-06 21:24 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson,
julien.grall, tim, stefano.stabellini, jbeulich, keir,
Tamas K Lengyel
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. The tree is only looked at if
mem_access_enabled is turned on.
Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
---
v12: - Move the flush in apply_p2m_changes to the label out,
so we it used in the memaccess preemption case as well.
v11: - Move including common/mem_event.h in here in p2m.h.
- Flush the tlb in p2m_set_mem_access to cover both the preemption
and successful finish cases.
v10: - Remove ASSERT from MEMACCESS case.
- Flush the tlb in the MEMACCESS case as we progress.
- Typos and style fixes.
v8: - Revert to arch specific p2m_mem_access_check.
- Retire dabt_dfsc enum and use FSC_FLT defines.
- Revert to Radix tree approach and use access_in_use flag to
indicate if the tree is in use or not to avoid uneccessary lookups.
v7: - Removed p2m_shatter_page and p2m_set_permission into separate
patch.
- Replace Radix tree settings store with extended struct page_info
approach. This way the trap handlers can use the MMU directly to
locate the permission store instead of having to do a tree-lookup.
- Add p2m_get_entry/set_entry compat functions which are required by
the common mem_access_check function.
- Typo fixes.
v6: - Add helper function p2m_shatter_page.
- Only allocate 4k pages when mem_access is in use.
- If no setting was found in radix tree but PTE exists,
return rwx as permission.
- Move the inclusion of various headers into this patch.
- Make npfec a const.
v5: - Move p2m_set_entry's logic into apply_one_level via
a new p2m_op, MEMACCESS.
v4: - Add p2m_mem_access_radix_set function to be called
when inserting new PTE's and when updating existing entries.
- Switch p2m_mem_access_check to return bool_t.
- Use new struct npfec to pass violation info.
v3: - Add new function for updating the PTE entries, p2m_set_entry.
- Use the new struct npfec to pass violation information.
- Implement n2rwx, rx2rw and listener required routines.
v2: - Patch been split to ease the review process.
- Add definitions of data abort data fetch status codes (enum dabt_dfsc)
and only call p2m_mem_access_check for traps caused by permission violations.
- Only call p2m_write_pte in p2m_lookup if the PTE permission actually changed.
- Properly save settings in the Radix tree and pause the VCPU with
mem_event_vcpu_pause.
---
xen/arch/arm/p2m.c | 382 +++++++++++++++++++++++++++++++++++++++++++---
xen/arch/arm/traps.c | 26 +++-
xen/include/asm-arm/p2m.h | 17 ++-
3 files changed, 400 insertions(+), 25 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 137e5a0..896da16 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -5,6 +5,9 @@
#include <xen/errno.h>
#include <xen/domain_page.h>
#include <xen/bitops.h>
+#include <xen/mem_event.h>
+#include <xen/mem_access.h>
+#include <public/mem_event.h>
#include <asm/flushtlb.h>
#include <asm/gic.h>
#include <asm/event.h>
@@ -421,12 +424,41 @@ static int p2m_create_table(struct domain *d, lpae_t *entry,
return 0;
}
+static int p2m_mem_access_radix_set(struct p2m_domain *p2m, unsigned long pfn,
+ p2m_access_t a)
+{
+ int rc;
+
+ if ( p2m_access_rwx == a )
+ {
+ if ( p2m->mem_access_enabled )
+ radix_tree_delete(&p2m->mem_access_settings, pfn);
+
+ return 0;
+ }
+
+ rc = radix_tree_insert(&p2m->mem_access_settings, pfn,
+ radix_tree_int_to_ptr(a));
+ if ( rc == -EEXIST )
+ {
+ /* If a setting existed already, change it to the new one */
+ radix_tree_replace_slot(
+ radix_tree_lookup_slot(
+ &p2m->mem_access_settings, pfn),
+ radix_tree_int_to_ptr(a));
+ rc = 0;
+ }
+
+ return rc;
+}
+
enum p2m_operation {
INSERT,
ALLOCATE,
REMOVE,
RELINQUISH,
CACHEFLUSH,
+ MEMACCESS,
};
/* Put any references on the single 4K page referenced by pte. TODO:
@@ -560,13 +592,22 @@ static int apply_one_level(struct domain *d,
if ( p2m_valid(orig_pte) )
return P2M_ONE_DESCEND;
- if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) )
+ if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) &&
+ /* We only create superpages when mem_access is not in use. */
+ (level == 3 || (level < 3 && !p2m->mem_access_enabled)) )
{
struct page_info *page;
page = alloc_domheap_pages(d, level_shift - PAGE_SHIFT, 0);
if ( page )
{
+ rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a);
+ if ( rc < 0 )
+ {
+ free_domheap_page(page);
+ return rc;
+ }
+
pte = mfn_to_p2m_entry(page_to_mfn(page), mattr, t, a);
if ( level < 3 )
pte.p2m.table = 0;
@@ -587,8 +628,8 @@ static int apply_one_level(struct domain *d,
/*
* If we get here then we failed to allocate a sufficiently
* large contiguous region for this level (which can't be
- * L3). Create a page table and continue to descend so we try
- * smaller allocations.
+ * L3) or mem_access is in use. Create a page table and
+ * continue to descend so we try smaller allocations.
*/
rc = p2m_create_table(d, entry, 0, flush_cache);
if ( rc < 0 )
@@ -598,9 +639,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->mem_access_enabled)) )
{
+ rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a);
+ if ( rc < 0 )
+ return rc;
+
/* New mapping is superpage aligned, make it */
pte = mfn_to_p2m_entry(*maddr >> PAGE_SHIFT, mattr, t, a);
if ( level < 3 )
@@ -716,6 +762,7 @@ static int apply_one_level(struct domain *d,
memset(&pte, 0x00, sizeof(pte));
p2m_write_pte(entry, pte, flush_cache);
+ p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), p2m_access_rwx);
*addr += level_size;
*maddr += level_size;
@@ -760,6 +807,47 @@ 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;
+ } /* else: an existing table mapping -> descend */
+
+ return P2M_ONE_DESCEND;
+ }
+ else
+ {
+ pte = orig_pte;
+
+ if ( !p2m_table(pte) )
+ pte.bits = 0;
+
+ if ( p2m_valid(pte) )
+ {
+ rc = p2m_mem_access_radix_set(p2m, paddr_to_pfn(*addr), a);
+ if ( rc < 0 )
+ return rc;
+
+ p2m_set_permission(&pte, pte.p2m.type, a);
+ p2m_write_pte(entry, pte, flush_cache);
+ }
+
+ *addr += level_size;
+ *flush = true;
+ return P2M_ONE_PROGRESS;
+ }
}
BUG(); /* Should never get here */
@@ -783,6 +871,8 @@ static int apply_p2m_changes(struct domain *d,
unsigned int cur_root_table = ~0;
unsigned int cur_offset[4] = { ~0, ~0, ~0, ~0 };
unsigned int count = 0;
+ const unsigned long sgfn = paddr_to_pfn(start_gpaddr),
+ egfn = paddr_to_pfn(end_gpaddr);
bool_t flush = false;
bool_t flush_pt;
@@ -828,6 +918,22 @@ 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 )
+ {
+ uint32_t progress = paddr_to_pfn(addr) - sgfn + 1;
+
+ if ( (egfn - sgfn) > progress && !(progress & mask)
+ && hypercall_preempt_check() )
+ {
+ rc = progress;
+ goto out;
+ }
+ }
+
if ( P2M_ROOT_PAGES > 1 )
{
int i;
@@ -891,20 +997,8 @@ static int apply_p2m_changes(struct domain *d,
}
}
- if ( flush )
- {
- unsigned long sgfn = paddr_to_pfn(start_gpaddr);
- unsigned long egfn = paddr_to_pfn(end_gpaddr);
-
- flush_tlb_domain(d);
- iommu_iotlb_flush(d, sgfn, egfn - sgfn);
- }
-
if ( op == ALLOCATE || op == INSERT )
{
- unsigned long sgfn = paddr_to_pfn(start_gpaddr);
- unsigned long egfn = paddr_to_pfn(end_gpaddr);
-
p2m->max_mapped_gfn = max(p2m->max_mapped_gfn, egfn);
p2m->lowest_mapped_gfn = min(p2m->lowest_mapped_gfn, sgfn);
}
@@ -912,6 +1006,12 @@ static int apply_p2m_changes(struct domain *d,
rc = 0;
out:
+ if ( flush )
+ {
+ flush_tlb_domain(d);
+ iommu_iotlb_flush(d, sgfn, egfn - sgfn);
+ }
+
if ( rc < 0 && ( op == INSERT || op == ALLOCATE ) &&
addr != start_gpaddr )
{
@@ -1281,6 +1381,254 @@ void __init setup_virt_paging(void)
smp_call_function(setup_virt_paging_one, (void *)val, 1);
}
+bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
+{
+ int rc;
+ bool_t violation;
+ xenmem_access_t xma;
+ mem_event_request_t *req;
+ struct vcpu *v = current;
+ struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
+
+ /* Mem_access is not in use. */
+ if ( !p2m->mem_access_enabled )
+ return true;
+
+ rc = p2m_get_mem_access(v->domain, paddr_to_pfn(gpa), &xma);
+ if ( rc )
+ return true;
+
+ /* Now check for mem_access violation. */
+ switch ( xma )
+ {
+ case XENMEM_access_rwx:
+ violation = false;
+ break;
+ case XENMEM_access_rw:
+ violation = npfec.insn_fetch;
+ break;
+ case XENMEM_access_wx:
+ violation = npfec.read_access;
+ break;
+ case XENMEM_access_rx:
+ case XENMEM_access_rx2rw:
+ violation = npfec.write_access;
+ break;
+ case XENMEM_access_x:
+ violation = npfec.read_access || npfec.write_access;
+ break;
+ case XENMEM_access_w:
+ violation = npfec.read_access || npfec.insn_fetch;
+ break;
+ case XENMEM_access_r:
+ violation = npfec.write_access || npfec.insn_fetch;
+ break;
+ default:
+ case XENMEM_access_n:
+ case XENMEM_access_n2rwx:
+ violation = true;
+ break;
+ }
+
+ if ( !violation )
+ return true;
+
+ /* First, handle rx2rw and n2rwx conversion automatically. */
+ if ( npfec.write_access && xma == XENMEM_access_rx2rw )
+ {
+ rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
+ 0, ~0, XENMEM_access_rw);
+ return false;
+ }
+ else if ( xma == XENMEM_access_n2rwx )
+ {
+ rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
+ 0, ~0, XENMEM_access_rwx);
+ }
+
+ /* Otherwise, check if there is a memory event listener, and send the message along */
+ if ( !mem_event_check_ring(&v->domain->mem_event->access) )
+ {
+ /* No listener */
+ if ( p2m->access_required )
+ {
+ gdprintk(XENLOG_INFO, "Memory access permissions failure, "
+ "no mem_event listener VCPU %d, dom %d\n",
+ v->vcpu_id, v->domain->domain_id);
+ domain_crash(v->domain);
+ }
+ else
+ {
+ /* n2rwx was already handled */
+ if ( xma != XENMEM_access_n2rwx )
+ {
+ /* A listener is not required, so clear the access
+ * restrictions. */
+ rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
+ 0, ~0, XENMEM_access_rwx);
+ }
+ }
+
+ /* No need to reinject */
+ return false;
+ }
+
+ req = xzalloc(mem_event_request_t);
+ if ( req )
+ {
+ req->reason = MEM_EVENT_REASON_VIOLATION;
+ if ( xma != XENMEM_access_n2rwx )
+ req->flags |= MEM_EVENT_FLAG_VCPU_PAUSED;
+ req->gfn = gpa >> PAGE_SHIFT;
+ req->offset = gpa & ((1 << PAGE_SHIFT) - 1);
+ req->gla = gla;
+ req->gla_valid = npfec.gla_valid;
+ req->access_r = npfec.read_access;
+ req->access_w = npfec.write_access;
+ req->access_x = npfec.insn_fetch;
+ if ( npfec_kind_in_gpt == npfec.kind )
+ req->fault_in_gpt = 1;
+ if ( npfec_kind_with_gla == npfec.kind )
+ req->fault_with_gla = 1;
+ req->vcpu_id = v->vcpu_id;
+
+ mem_access_send_req(v->domain, req);
+ xfree(req);
+ }
+
+ /* Pause the current VCPU */
+ if ( xma != XENMEM_access_n2rwx )
+ mem_event_vcpu_pause(v);
+
+ return false;
+}
+
+/* Set access type for a region of pfns.
+ * If start_pfn == -1ul, sets the default access type */
+long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
+ uint32_t start, uint32_t mask, xenmem_access_t access)
+{
+ struct p2m_domain *p2m = p2m_get_hostp2m(d);
+ p2m_access_t a;
+ long rc = 0;
+
+ static const p2m_access_t memaccess[] = {
+#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
+ ACCESS(n),
+ ACCESS(r),
+ ACCESS(w),
+ ACCESS(rw),
+ ACCESS(x),
+ ACCESS(rx),
+ ACCESS(wx),
+ ACCESS(rwx),
+ ACCESS(rx2rw),
+ ACCESS(n2rwx),
+#undef ACCESS
+ };
+
+ switch ( access )
+ {
+ case 0 ... ARRAY_SIZE(memaccess) - 1:
+ a = memaccess[access];
+ break;
+ case XENMEM_access_default:
+ a = p2m->default_access;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ /*
+ * Flip mem_access_enabled to true when a permission is set, as to prevent
+ * allocating or inserting super-pages.
+ */
+ p2m->mem_access_enabled = true;
+
+ /* If request to set default access. */
+ if ( pfn == ~0ul )
+ {
+ p2m->default_access = a;
+ return 0;
+ }
+
+ rc = apply_p2m_changes(d, MEMACCESS,
+ pfn_to_paddr(pfn+start), pfn_to_paddr(pfn+nr),
+ 0, MATTR_MEM, mask, 0, a);
+ if ( rc < 0 )
+ return rc;
+ else if ( rc > 0 )
+ return start + rc;
+
+ return 0;
+}
+
+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
+ };
+
+ /* If no setting was ever set, just return rwx. */
+ if ( !p2m->mem_access_enabled )
+ {
+ *access = XENMEM_access_rwx;
+ return 0;
+ }
+
+ /* If request to get default access */
+ if ( gpfn == ~0ull )
+ {
+ *access = memaccess[p2m->default_access];
+ return 0;
+ }
+
+ spin_lock(&p2m->lock);
+ i = radix_tree_lookup(&p2m->mem_access_settings, gpfn);
+ spin_unlock(&p2m->lock);
+
+ if ( !i )
+ {
+ /*
+ * No setting was found in the Radix tree. Check if the
+ * entry exists in the page-tables.
+ */
+ paddr_t maddr = p2m_lookup(d, gpfn << PAGE_SHIFT, NULL);
+ if ( INVALID_PADDR == maddr )
+ return -ESRCH;
+
+ /* If entry exists then its rwx. */
+ *access = XENMEM_access_rwx;
+ }
+ else
+ {
+ /* Setting was found in the Radix tree. */
+ index = radix_tree_ptr_to_int(i);
+ if ( index >= ARRAY_SIZE(memaccess) )
+ return -ERANGE;
+
+ *access = memaccess[index];
+ }
+
+ return 0;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index ad046e8..f5aa647 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1985,11 +1985,31 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
info.gva = READ_SYSREG64(FAR_EL2);
#endif
- if (dabt.s1ptw)
+ rc = gva_to_ipa(info.gva, &info.gpa);
+ if ( -EFAULT == rc )
goto bad_data_abort;
- rc = gva_to_ipa(info.gva, &info.gpa);
- if ( rc == -EFAULT )
+ switch ( dabt.dfsc & 0x3f )
+ {
+ case FSC_FLT_PERM ... FSC_FLT_PERM + 3:
+ {
+ const struct npfec npfec = {
+ .read_access = 1,
+ .write_access = dabt.write,
+ .gla_valid = 1,
+ .kind = dabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
+ };
+
+ rc = p2m_mem_access_check(info.gpa, info.gva, npfec);
+
+ /* Trap was triggered by mem_access, work here is done */
+ if ( !rc )
+ return;
+ }
+ break;
+ }
+
+ if ( dabt.s1ptw )
goto bad_data_abort;
/* XXX: Decode the instruction if ISS is not valid */
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 7583d9b..5c38b3e 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -3,6 +3,8 @@
#include <xen/mm.h>
#include <xen/radix-tree.h>
+#include <public/mem_event.h> /* for mem_event_response_t */
+#include <public/memory.h>
#include <xen/p2m-common.h>
#include <public/memory.h>
@@ -243,12 +245,17 @@ static inline bool_t p2m_mem_event_sanity_check(struct domain *d)
/* Get access type for a pfn
* If pfn == -1ul, gets the default access type */
-static inline
int p2m_get_mem_access(struct domain *d, unsigned long pfn,
- xenmem_access_t *access)
-{
- return -ENOSYS;
-}
+ xenmem_access_t *access);
+
+/* Set access type for a region of pfns.
+ * If start_pfn == -1ul, sets the default access type */
+long p2m_set_mem_access(struct domain *d, unsigned long start_pfn, uint32_t nr,
+ uint32_t start, uint32_t mask, xenmem_access_t access);
+
+/* Send mem event based on the access. Boolean return value indicates if trap
+ * needs to be injected into guest. */
+bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec);
#endif /* _XEN_P2M_H */
--
2.1.4
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events.
2015-03-06 21:24 ` [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events Tamas K Lengyel
@ 2015-03-12 13:35 ` Ian Campbell
2015-03-12 15:13 ` Tamas K Lengyel
2015-03-12 15:13 ` Julien Grall
1 sibling, 1 reply; 71+ messages in thread
From: Ian Campbell @ 2015-03-12 13:35 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, stefano.stabellini, ian.jackson, julien.grall, tim,
xen-devel, stefano.stabellini, jbeulich, keir
On Fri, 2015-03-06 at 22:24 +0100, 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. The tree is only looked at if
> mem_access_enabled is turned on.
But it is maintained/updated regardless, is that deliberate?
> +static int p2m_mem_access_radix_set(struct p2m_domain *p2m, unsigned long pfn,
> + p2m_access_t a)
> +{
> + int rc;
> +
> + if ( p2m_access_rwx == a )
> + {
> + if ( p2m->mem_access_enabled )
In particular this is gated, but the rest of the function appears not to
be, which seems inconsistent...
> + radix_tree_delete(&p2m->mem_access_settings, pfn);
> +
> + return 0;
> + }
> +
> + rc = radix_tree_insert(&p2m->mem_access_settings, pfn,
> + radix_tree_int_to_ptr(a));
> + if ( rc == -EEXIST )
> + {
> + /* If a setting existed already, change it to the new one */
> + radix_tree_replace_slot(
> + radix_tree_lookup_slot(
> + &p2m->mem_access_settings, pfn),
> + radix_tree_int_to_ptr(a));
> + rc = 0;
> + }
> +
> + return rc;
> +}
> +
> enum p2m_operation {
> INSERT,
> ALLOCATE,
> REMOVE,
> RELINQUISH,
> CACHEFLUSH,
> + MEMACCESS,
> };
>
> /* Put any references on the single 4K page referenced by pte. TODO:
> @@ -560,13 +592,22 @@ static int apply_one_level(struct domain *d,
> if ( p2m_valid(orig_pte) )
> return P2M_ONE_DESCEND;
>
> - if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) )
> + if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) &&
> + /* We only create superpages when mem_access is not in use. */
> + (level == 3 || (level < 3 && !p2m->mem_access_enabled)) )
I don't think we can get away with adding this check to
is_mapping_aligned (it's used elsewhere), but perhaps you could wrap
this condition in a helper to use in both places.
mapping_allowed_at_level(p2m, level) or some such.
> - /* 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->mem_access_enabled)) )
Actually, this is very subtly different isn't it. Can it be unified? If
not then ignore the helper idea.
> @@ -760,6 +807,47 @@ 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;
> + } /* else: an existing table mapping -> descend */
> +
> + return P2M_ONE_DESCEND;
> + }
> + else
> + {
> + pte = orig_pte;
> +
> + if ( !p2m_table(pte) )
> + pte.bits = 0;
What is this about? Just clobbering an invalid PTE?
> @@ -783,6 +871,8 @@ static int apply_p2m_changes(struct domain *d,
> unsigned int cur_root_table = ~0;
> unsigned int cur_offset[4] = { ~0, ~0, ~0, ~0 };
> unsigned int count = 0;
> + const unsigned long sgfn = paddr_to_pfn(start_gpaddr),
> + egfn = paddr_to_pfn(end_gpaddr);
> bool_t flush = false;
> bool_t flush_pt;
>
> @@ -912,6 +1006,12 @@ static int apply_p2m_changes(struct domain *d,
> rc = 0;
>
> out:
> + if ( flush )
> + {
> + flush_tlb_domain(d);
> + iommu_iotlb_flush(d, sgfn, egfn - sgfn);
> + }
Is moving the flush out of the loop an independent bug fix? If so please
do in a separate commit with a rationale in the commit log. If it is
somehow related to the changes here then please mention it in this
commit log, since it's a bit subtle.
> +
> if ( rc < 0 && ( op == INSERT || op == ALLOCATE ) &&
> addr != start_gpaddr )
> {
> @@ -1281,6 +1381,254 @@ void __init setup_virt_paging(void)
> smp_call_function(setup_virt_paging_one, (void *)val, 1);
> }
>
> +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
This is different to the current x86 prototype, is that due to your
other cleanup series?
> +{
> + int rc;
> + bool_t violation;
> + xenmem_access_t xma;
> + mem_event_request_t *req;
> + struct vcpu *v = current;
> + struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
> +
> + /* Mem_access is not in use. */
> + if ( !p2m->mem_access_enabled )
> + return true;
> +
> + rc = p2m_get_mem_access(v->domain, paddr_to_pfn(gpa), &xma);
> + if ( rc )
> + return true;
> +
> + /* Now check for mem_access violation. */
> + switch ( xma )
> + {
> + case XENMEM_access_rwx:
> + violation = false;
> + break;
> + case XENMEM_access_rw:
> + violation = npfec.insn_fetch;
> + break;
> + case XENMEM_access_wx:
> + violation = npfec.read_access;
> + break;
> + case XENMEM_access_rx:
> + case XENMEM_access_rx2rw:
> + violation = npfec.write_access;
> + break;
> + case XENMEM_access_x:
> + violation = npfec.read_access || npfec.write_access;
> + break;
> + case XENMEM_access_w:
> + violation = npfec.read_access || npfec.insn_fetch;
> + break;
> + case XENMEM_access_r:
> + violation = npfec.write_access || npfec.insn_fetch;
> + break;
> + default:
> + case XENMEM_access_n:
> + case XENMEM_access_n2rwx:
> + violation = true;
> + break;
> + }
> +
> + if ( !violation )
> + return true;
The preceding section looks pretty similar to the guits of x86's
p2m_mem_event_emulate_check, can they be combined?
> +
> + /* First, handle rx2rw and n2rwx conversion automatically. */
> + if ( npfec.write_access && xma == XENMEM_access_rx2rw )
> + {
> + rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
> + 0, ~0, XENMEM_access_rw);
> + return false;
> + }
> + else if ( xma == XENMEM_access_n2rwx )
> + {
> + rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
> + 0, ~0, XENMEM_access_rwx);
> + }
This looks like a bit of p2m_mem_access_check, can it be made common?
> +
> + /* Otherwise, check if there is a memory event listener, and send the message along */
> + if ( !mem_event_check_ring(&v->domain->mem_event->access) )
> + {
> + /* No listener */
> + if ( p2m->access_required )
> + {
> + gdprintk(XENLOG_INFO, "Memory access permissions failure, "
> + "no mem_event listener VCPU %d, dom %d\n",
> + v->vcpu_id, v->domain->domain_id);
> + domain_crash(v->domain);
> + }
> + else
> + {
> + /* n2rwx was already handled */
> + if ( xma != XENMEM_access_n2rwx )
> + {
> + /* A listener is not required, so clear the access
> + * restrictions. */
> + rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
> + 0, ~0, XENMEM_access_rwx);
> + }
> + }
> +
> + /* No need to reinject */
> + return false;
> + }
And this
> + req = xzalloc(mem_event_request_t);
> + if ( req )
> + {
> + req->reason = MEM_EVENT_REASON_VIOLATION;
> + if ( xma != XENMEM_access_n2rwx )
> + req->flags |= MEM_EVENT_FLAG_VCPU_PAUSED;
> + req->gfn = gpa >> PAGE_SHIFT;
> + req->offset = gpa & ((1 << PAGE_SHIFT) - 1);
> + req->gla = gla;
> + req->gla_valid = npfec.gla_valid;
> + req->access_r = npfec.read_access;
> + req->access_w = npfec.write_access;
> + req->access_x = npfec.insn_fetch;
> + if ( npfec_kind_in_gpt == npfec.kind )
> + req->fault_in_gpt = 1;
> + if ( npfec_kind_with_gla == npfec.kind )
> + req->fault_with_gla = 1;
> + req->vcpu_id = v->vcpu_id;
> +
> + mem_access_send_req(v->domain, req);
> + xfree(req);
> + }
> +
> + /* Pause the current VCPU */
> + if ( xma != XENMEM_access_n2rwx )
> + mem_event_vcpu_pause(v);
> +
> + return false;
> +}
> +
> +/* Set access type for a region of pfns.
> + * If start_pfn == -1ul, sets the default access type */
> +long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t nr,
> + uint32_t start, uint32_t mask, xenmem_access_t access)
and this function is nearly identical to the x86 one too.
> +{
> + struct p2m_domain *p2m = p2m_get_hostp2m(d);
> + p2m_access_t a;
> + long rc = 0;
> +
> + static const p2m_access_t memaccess[] = {
> +#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
> + ACCESS(n),
> + ACCESS(r),
> + ACCESS(w),
> + ACCESS(rw),
> + ACCESS(x),
> + ACCESS(rx),
> + ACCESS(wx),
> + ACCESS(rwx),
> + ACCESS(rx2rw),
> + ACCESS(n2rwx),
> +#undef ACCESS
> + };
> +
> + switch ( access )
> + {
> + case 0 ... ARRAY_SIZE(memaccess) - 1:
> + a = memaccess[access];
> + break;
> + case XENMEM_access_default:
> + a = p2m->default_access;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + /*
> + * Flip mem_access_enabled to true when a permission is set, as to prevent
> + * allocating or inserting super-pages.
> + */
> + p2m->mem_access_enabled = true;
> +
> + /* If request to set default access. */
> + if ( pfn == ~0ul )
> + {
> + p2m->default_access = a;
> + return 0;
> + }
> +
> + rc = apply_p2m_changes(d, MEMACCESS,
> + pfn_to_paddr(pfn+start), pfn_to_paddr(pfn+nr),
> + 0, MATTR_MEM, mask, 0, a);
> + if ( rc < 0 )
> + return rc;
> + else if ( rc > 0 )
> + return start + rc;
> +
> + return 0;
> +}
> +
> +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[] = {
Would be nice not to have this static const thing twice in .rodata.
> +#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
> + };
> +
> + /* If no setting was ever set, just return rwx. */
> + if ( !p2m->mem_access_enabled )
> + {
> + *access = XENMEM_access_rwx;
> + return 0;
> + }
> +
> + /* If request to get default access */
> + if ( gpfn == ~0ull )
We should have a suitable constant for this, I think, INVALID_MFN looks
like the one.
> + {
> + *access = memaccess[p2m->default_access];
> + return 0;
> + }
> +
> + spin_lock(&p2m->lock);
> + i = radix_tree_lookup(&p2m->mem_access_settings, gpfn);
> + spin_unlock(&p2m->lock);
> +
> + if ( !i )
> + {
> + /*
> + * No setting was found in the Radix tree. Check if the
> + * entry exists in the page-tables.
> + */
> + paddr_t maddr = p2m_lookup(d, gpfn << PAGE_SHIFT, NULL);
> + if ( INVALID_PADDR == maddr )
> + return -ESRCH;
> +
> + /* If entry exists then its rwx. */
it's, please
> + *access = XENMEM_access_rwx;
> + }
> + else
> + {
> + /* Setting was found in the Radix tree. */
> + index = radix_tree_ptr_to_int(i);
> + if ( index >= ARRAY_SIZE(memaccess) )
> + return -ERANGE;
> +
> + *access = memaccess[index];
> + }
> +
> + return 0;
> +}
> +
> /*
> * Local variables:
> * mode: C
Ian.
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events.
2015-03-12 13:35 ` Ian Campbell
@ 2015-03-12 15:13 ` Tamas K Lengyel
2015-03-12 15:19 ` Tamas K Lengyel
2015-03-12 15:30 ` Ian Campbell
0 siblings, 2 replies; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-12 15:13 UTC (permalink / raw)
To: Ian Campbell
Cc: wei.liu2, Stefano Stabellini, Ian Jackson, Julien Grall,
Tim Deegan, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 15709 bytes --]
On Thu, Mar 12, 2015 at 2:35 PM, Ian Campbell <ian.campbell@citrix.com>
wrote:
> On Fri, 2015-03-06 at 22:24 +0100, 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. The tree is only looked at if
> > mem_access_enabled is turned on.
>
> But it is maintained/updated regardless, is that deliberate?
> > +static int p2m_mem_access_radix_set(struct p2m_domain *p2m, unsigned
> long pfn,
> > + p2m_access_t a)
> > +{
> > + int rc;
> > +
> > + if ( p2m_access_rwx == a )
> > + {
> > + if ( p2m->mem_access_enabled )
>
> In particular this is gated, but the rest of the function appears not to
> be, which seems inconsistent...
>
Ack, this is a bit awkward here. If mem_access is not enabled, the access
type passed here is always p2m_access_rwx, thus the tree wasn't
updated/maintained. I'll make it more explicit by moving the check for
p2m->mem_access_enabled up front to return 0 if not enabled..
>
> > + radix_tree_delete(&p2m->mem_access_settings, pfn);
> > +
> > + return 0;
> > + }
> > +
> > + rc = radix_tree_insert(&p2m->mem_access_settings, pfn,
> > + radix_tree_int_to_ptr(a));
> > + if ( rc == -EEXIST )
> > + {
> > + /* If a setting existed already, change it to the new one */
> > + radix_tree_replace_slot(
> > + radix_tree_lookup_slot(
> > + &p2m->mem_access_settings, pfn),
> > + radix_tree_int_to_ptr(a));
> > + rc = 0;
> > + }
> > +
> > + return rc;
> > +}
> > +
> > enum p2m_operation {
> > INSERT,
> > ALLOCATE,
> > REMOVE,
> > RELINQUISH,
> > CACHEFLUSH,
> > + MEMACCESS,
> > };
> >
> > /* Put any references on the single 4K page referenced by pte. TODO:
> > @@ -560,13 +592,22 @@ static int apply_one_level(struct domain *d,
> > if ( p2m_valid(orig_pte) )
> > return P2M_ONE_DESCEND;
> >
> > - if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) )
> > + if ( is_mapping_aligned(*addr, end_gpaddr, 0, level_size) &&
> > + /* We only create superpages when mem_access is not in use.
> */
> > + (level == 3 || (level < 3 && !p2m->mem_access_enabled)) )
>
> I don't think we can get away with adding this check to
> is_mapping_aligned (it's used elsewhere), but perhaps you could wrap
> this condition in a helper to use in both places.
>
> mapping_allowed_at_level(p2m, level) or some such.
>
> > - /* 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->mem_access_enabled)) )
>
> Actually, this is very subtly different isn't it. Can it be unified? If
> not then ignore the helper idea.
>
I don't think it would make it any easier to read if abstracted. I rather
keep it this way.
>
> > @@ -760,6 +807,47 @@ 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;
> > + } /* else: an existing table mapping -> descend */
> > +
> > + return P2M_ONE_DESCEND;
> > + }
> > + else
> > + {
> > + pte = orig_pte;
> > +
> > + if ( !p2m_table(pte) )
> > + pte.bits = 0;
>
> What is this about? Just clobbering an invalid PTE?
>
Hm, yea it's not actually required.
>
> > @@ -783,6 +871,8 @@ static int apply_p2m_changes(struct domain *d,
> > unsigned int cur_root_table = ~0;
> > unsigned int cur_offset[4] = { ~0, ~0, ~0, ~0 };
> > unsigned int count = 0;
> > + const unsigned long sgfn = paddr_to_pfn(start_gpaddr),
> > + egfn = paddr_to_pfn(end_gpaddr);
> > bool_t flush = false;
> > bool_t flush_pt;
> >
> > @@ -912,6 +1006,12 @@ static int apply_p2m_changes(struct domain *d,
> > rc = 0;
> >
> > out:
> > + if ( flush )
> > + {
> > + flush_tlb_domain(d);
> > + iommu_iotlb_flush(d, sgfn, egfn - sgfn);
> > + }
>
> Is moving the flush out of the loop an independent bug fix? If so please
> do in a separate commit with a rationale in the commit log. If it is
> somehow related to the changes here then please mention it in this
> commit log, since it's a bit subtle.
>
Right, it's not a bugfix and not required to be outside the loop, I think I
just moved it because it made sense to me to flush it only once instead at
every iteration. I'll place it back.
>
> > +
> > if ( rc < 0 && ( op == INSERT || op == ALLOCATE ) &&
> > addr != start_gpaddr )
> > {
> > @@ -1281,6 +1381,254 @@ void __init setup_virt_paging(void)
> > smp_call_function(setup_virt_paging_one, (void *)val, 1);
> > }
> >
> > +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct
> npfec npfec)
>
> This is different to the current x86 prototype, is that due to your
> other cleanup series?
>
It's different because of how the x86 code is structured. In ARM it is a
bit more straight forward thus the prototype is simpler. The function
doesn't get called from common, so arch specific discrepancies are OK.
>
> > +{
> > + int rc;
> > + bool_t violation;
> > + xenmem_access_t xma;
> > + mem_event_request_t *req;
> > + struct vcpu *v = current;
> > + struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
> > +
> > + /* Mem_access is not in use. */
> > + if ( !p2m->mem_access_enabled )
> > + return true;
> > +
> > + rc = p2m_get_mem_access(v->domain, paddr_to_pfn(gpa), &xma);
> > + if ( rc )
> > + return true;
> > +
> > + /* Now check for mem_access violation. */
> > + switch ( xma )
> > + {
> > + case XENMEM_access_rwx:
> > + violation = false;
> > + break;
> > + case XENMEM_access_rw:
> > + violation = npfec.insn_fetch;
> > + break;
> > + case XENMEM_access_wx:
> > + violation = npfec.read_access;
> > + break;
> > + case XENMEM_access_rx:
> > + case XENMEM_access_rx2rw:
> > + violation = npfec.write_access;
> > + break;
> > + case XENMEM_access_x:
> > + violation = npfec.read_access || npfec.write_access;
> > + break;
> > + case XENMEM_access_w:
> > + violation = npfec.read_access || npfec.insn_fetch;
> > + break;
> > + case XENMEM_access_r:
> > + violation = npfec.write_access || npfec.insn_fetch;
> > + break;
> > + default:
> > + case XENMEM_access_n:
> > + case XENMEM_access_n2rwx:
> > + violation = true;
> > + break;
> > + }
> > +
> > + if ( !violation )
> > + return true;
>
>
> The preceding section looks pretty similar to the guits of x86's
> p2m_mem_event_emulate_check, can they be combined?
>
Not without introducing another wrapper around them. The
mem_event_emulate_check in x86 takes mem_event_response_t as input, here we
take npfec. While the logic applied afterwards is similar, I would rather
do consolidation like that once this series and the other cleanup series
are both applied.
>
> > +
> > + /* First, handle rx2rw and n2rwx conversion automatically. */
> > + if ( npfec.write_access && xma == XENMEM_access_rx2rw )
> > + {
> > + rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
> > + 0, ~0, XENMEM_access_rw);
> > + return false;
> > + }
> > + else if ( xma == XENMEM_access_n2rwx )
> > + {
> > + rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
> > + 0, ~0, XENMEM_access_rwx);
> > + }
>
> This looks like a bit of p2m_mem_access_check, can it be made common?
>
Could be but for now I rather keep them separate.
>
> > +
> > + /* Otherwise, check if there is a memory event listener, and send
> the message along */
> > + if ( !mem_event_check_ring(&v->domain->mem_event->access) )
> > + {
> > + /* No listener */
> > + if ( p2m->access_required )
> > + {
> > + gdprintk(XENLOG_INFO, "Memory access permissions failure, "
> > + "no mem_event listener VCPU %d, dom
> %d\n",
> > + v->vcpu_id, v->domain->domain_id);
> > + domain_crash(v->domain);
> > + }
> > + else
> > + {
> > + /* n2rwx was already handled */
> > + if ( xma != XENMEM_access_n2rwx )
> > + {
> > + /* A listener is not required, so clear the access
> > + * restrictions. */
> > + rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
> > + 0, ~0, XENMEM_access_rwx);
> > + }
> > + }
> > +
> > + /* No need to reinject */
> > + return false;
> > + }
>
> And this
>
This is actually a bit different. On x86 the request is just being setup
here and sent only later, while on ARM we can actually send it right away.
I had made an attempt before to consolidate these two, but the x86 side
required some heavy cleanup before that was possible so it got postponed to
happen after the mem_event API itself is cleaned up.
>
> > + req = xzalloc(mem_event_request_t);
> > + if ( req )
> > + {
> > + req->reason = MEM_EVENT_REASON_VIOLATION;
> > + if ( xma != XENMEM_access_n2rwx )
> > + req->flags |= MEM_EVENT_FLAG_VCPU_PAUSED;
> > + req->gfn = gpa >> PAGE_SHIFT;
> > + req->offset = gpa & ((1 << PAGE_SHIFT) - 1);
> > + req->gla = gla;
> > + req->gla_valid = npfec.gla_valid;
> > + req->access_r = npfec.read_access;
> > + req->access_w = npfec.write_access;
> > + req->access_x = npfec.insn_fetch;
> > + if ( npfec_kind_in_gpt == npfec.kind )
> > + req->fault_in_gpt = 1;
> > + if ( npfec_kind_with_gla == npfec.kind )
> > + req->fault_with_gla = 1;
> > + req->vcpu_id = v->vcpu_id;
> > +
> > + mem_access_send_req(v->domain, req);
> > + xfree(req);
> > + }
> > +
> > + /* Pause the current VCPU */
> > + if ( xma != XENMEM_access_n2rwx )
> > + mem_event_vcpu_pause(v);
> > +
> > + return false;
> > +}
> > +
> > +/* Set access type for a region of pfns.
> > + * If start_pfn == -1ul, sets the default access type */
> > +long p2m_set_mem_access(struct domain *d, unsigned long pfn, uint32_t
> nr,
> > + uint32_t start, uint32_t mask, xenmem_access_t
> access)
>
> and this function is nearly identical to the x86 one too.
>
>
Nearly, but not completely. IMHO consolidation may be possible on some of
these bits, but I'm not sure if it would make it any easier to follow when
the code jumps back and forth between common and arch specific parts.
> > +{
> > + struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > + p2m_access_t a;
> > + long rc = 0;
> > +
> > + static const p2m_access_t memaccess[] = {
> > +#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
> > + ACCESS(n),
> > + ACCESS(r),
> > + ACCESS(w),
> > + ACCESS(rw),
> > + ACCESS(x),
> > + ACCESS(rx),
> > + ACCESS(wx),
> > + ACCESS(rwx),
> > + ACCESS(rx2rw),
> > + ACCESS(n2rwx),
> > +#undef ACCESS
> > + };
> > +
> > + switch ( access )
> > + {
> > + case 0 ... ARRAY_SIZE(memaccess) - 1:
> > + a = memaccess[access];
> > + break;
> > + case XENMEM_access_default:
> > + a = p2m->default_access;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + /*
> > + * Flip mem_access_enabled to true when a permission is set, as to
> prevent
> > + * allocating or inserting super-pages.
> > + */
> > + p2m->mem_access_enabled = true;
> > +
> > + /* If request to set default access. */
> > + if ( pfn == ~0ul )
> > + {
> > + p2m->default_access = a;
> > + return 0;
> > + }
> > +
> > + rc = apply_p2m_changes(d, MEMACCESS,
> > + pfn_to_paddr(pfn+start),
> pfn_to_paddr(pfn+nr),
> > + 0, MATTR_MEM, mask, 0, a);
> > + if ( rc < 0 )
> > + return rc;
> > + else if ( rc > 0 )
> > + return start + rc;
> > +
> > + return 0;
> > +}
> > +
> > +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[] = {
>
> Would be nice not to have this static const thing twice in .rodata.
>
What do you mean twice? One is converting from p2m_access to XENMEM_access,
the other is XENMEM_access to p2m_access.
>
> > +#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
> > + };
> > +
> > + /* If no setting was ever set, just return rwx. */
> > + if ( !p2m->mem_access_enabled )
> > + {
> > + *access = XENMEM_access_rwx;
> > + return 0;
> > + }
> > +
> > + /* If request to get default access */
> > + if ( gpfn == ~0ull )
>
> We should have a suitable constant for this, I think, INVALID_MFN looks
> like the one.
>
~0ull is specifically used by the mem_access API for this purpose. If
anywhere, in the cleanup series it might make sense to have a #define added
for it.
>
> > + {
> > + *access = memaccess[p2m->default_access];
> > + return 0;
> > + }
> > +
> > + spin_lock(&p2m->lock);
> > + i = radix_tree_lookup(&p2m->mem_access_settings, gpfn);
> > + spin_unlock(&p2m->lock);
> > +
> > + if ( !i )
> > + {
> > + /*
> > + * No setting was found in the Radix tree. Check if the
> > + * entry exists in the page-tables.
> > + */
> > + paddr_t maddr = p2m_lookup(d, gpfn << PAGE_SHIFT, NULL);
> > + if ( INVALID_PADDR == maddr )
> > + return -ESRCH;
> > +
> > + /* If entry exists then its rwx. */
>
> it's, please
>
Ack.
>
> > + *access = XENMEM_access_rwx;
> > + }
> > + else
> > + {
> > + /* Setting was found in the Radix tree. */
> > + index = radix_tree_ptr_to_int(i);
> > + if ( index >= ARRAY_SIZE(memaccess) )
> > + return -ERANGE;
> > +
> > + *access = memaccess[index];
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /*
> > * Local variables:
> > * mode: C
>
> Ian.
>
Thanks,
Tamas
[-- Attachment #1.2: Type: text/html, Size: 22554 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events.
2015-03-12 15:13 ` Tamas K Lengyel
@ 2015-03-12 15:19 ` Tamas K Lengyel
2015-03-12 15:24 ` Julien Grall
2015-03-12 15:35 ` Ian Campbell
2015-03-12 15:30 ` Ian Campbell
1 sibling, 2 replies; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-12 15:19 UTC (permalink / raw)
To: Ian Campbell
Cc: wei.liu2, Stefano Stabellini, Ian Jackson, Julien Grall,
Tim Deegan, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 950 bytes --]
> out:
> > + if ( flush )
>> > + {
>> > + flush_tlb_domain(d);
>> > + iommu_iotlb_flush(d, sgfn, egfn - sgfn);
>> > + }
>>
>> Is moving the flush out of the loop an independent bug fix? If so please
>> do in a separate commit with a rationale in the commit log. If it is
>> somehow related to the changes here then please mention it in this
>> commit log, since it's a bit subtle.
>>
>
> Right, it's not a bugfix and not required to be outside the loop, I think
> I just moved it because it made sense to me to flush it only once instead
> at every iteration. I'll place it back.
>
Sorry, the flush wasn't actually part of the loop to begin with. I just
moved it under the label out so that the TLB gets flushed when the
memaccess setting hypercall gets preempted. I will just set a separate
label for it before out so that the existing behavior is preserved but the
tlb is still flushed when memaccess is preempted.
Tamas
[-- Attachment #1.2: Type: text/html, Size: 1546 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events.
2015-03-12 15:19 ` Tamas K Lengyel
@ 2015-03-12 15:24 ` Julien Grall
2015-03-12 15:35 ` Ian Campbell
1 sibling, 0 replies; 71+ messages in thread
From: Julien Grall @ 2015-03-12 15:24 UTC (permalink / raw)
To: Tamas K Lengyel, Ian Campbell
Cc: wei.liu2, Stefano Stabellini, Ian Jackson, Tim Deegan, xen-devel,
stefano.stabellini, Jan Beulich, Keir Fraser
On 12/03/15 15:19, Tamas K Lengyel wrote:
>
>> out:
>
> > + if ( flush )
> > + {
> > + flush_tlb_domain(d);
> > + iommu_iotlb_flush(d, sgfn, egfn - sgfn);
> > + }
>
> Is moving the flush out of the loop an independent bug fix? If
> so please
> do in a separate commit with a rationale in the commit log. If it is
> somehow related to the changes here then please mention it in this
> commit log, since it's a bit subtle.
>
>
> Right, it's not a bugfix and not required to be outside the loop, I
> think I just moved it because it made sense to me to flush it only
> once instead at every iteration. I'll place it back.
>
>
> Sorry, the flush wasn't actually part of the loop to begin with. I just
> moved it under the label out so that the TLB gets flushed when the
> memaccess setting hypercall gets preempted. I will just set a separate
> label for it before out so that the existing behavior is preserved but
> the tlb is still flushed when memaccess is preempted.
Even though today it's only require for memaccess, the code move would
benefit all others caller later. Mainly if we decide to support
preemption later...
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events.
2015-03-12 15:19 ` Tamas K Lengyel
2015-03-12 15:24 ` Julien Grall
@ 2015-03-12 15:35 ` Ian Campbell
2015-03-12 16:35 ` Julien Grall
1 sibling, 1 reply; 71+ messages in thread
From: Ian Campbell @ 2015-03-12 15:35 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, Stefano Stabellini, Tim Deegan, Julien Grall,
Ian Jackson, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
On Thu, 2015-03-12 at 16:19 +0100, Tamas K Lengyel wrote:
>
> > out:
> > + if ( flush )
> > + {
> > + flush_tlb_domain(d);
> > + iommu_iotlb_flush(d, sgfn, egfn - sgfn);
> > + }
>
> Is moving the flush out of the loop an independent bug
> fix? If so please
> do in a separate commit with a rationale in the commit
> log. If it is
> somehow related to the changes here then please
> mention it in this
> commit log, since it's a bit subtle.
>
>
> Right, it's not a bugfix and not required to be outside the
> loop, I think I just moved it because it made sense to me to
> flush it only once instead at every iteration. I'll place it
> back.
>
> Sorry, the flush wasn't actually part of the loop to begin with. I
> just moved it under the label out so that the TLB gets flushed when
> the memaccess setting hypercall gets preempted. I will just set a
> separate label for it before out so that the existing behavior is
> preserved but the tlb is still flushed when memaccess is preempted.
I wonder why this isn't needed for the other uses of goto out, i.e. on
the relinquish check.
I'm not convinced this isn't just a straight up bug. Anyone remember any
reasoning why we don't flush on exit if any work has been done?
Ian.
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events.
2015-03-12 15:35 ` Ian Campbell
@ 2015-03-12 16:35 ` Julien Grall
0 siblings, 0 replies; 71+ messages in thread
From: Julien Grall @ 2015-03-12 16:35 UTC (permalink / raw)
To: Ian Campbell, Tamas K Lengyel
Cc: wei.liu2, Stefano Stabellini, Tim Deegan, Ian Jackson, xen-devel,
stefano.stabellini, Jan Beulich, Keir Fraser
Hi Ian,
On 12/03/15 15:35, Ian Campbell wrote:
> On Thu, 2015-03-12 at 16:19 +0100, Tamas K Lengyel wrote:
>>
>>> out:
>> > + if ( flush )
>> > + {
>> > + flush_tlb_domain(d);
>> > + iommu_iotlb_flush(d, sgfn, egfn - sgfn);
>> > + }
>>
>> Is moving the flush out of the loop an independent bug
>> fix? If so please
>> do in a separate commit with a rationale in the commit
>> log. If it is
>> somehow related to the changes here then please
>> mention it in this
>> commit log, since it's a bit subtle.
>>
>>
>> Right, it's not a bugfix and not required to be outside the
>> loop, I think I just moved it because it made sense to me to
>> flush it only once instead at every iteration. I'll place it
>> back.
>
>>
>> Sorry, the flush wasn't actually part of the loop to begin with. I
>> just moved it under the label out so that the TLB gets flushed when
>> the memaccess setting hypercall gets preempted. I will just set a
>> separate label for it before out so that the existing behavior is
>> preserved but the tlb is still flushed when memaccess is preempted.
>
> I wonder why this isn't needed for the other uses of goto out, i.e. on
> the relinquish check.
relinquish is only done when the domain is destroyed. So the flush is
not strictly necessary :).
> I'm not convinced this isn't just a straight up bug. Anyone remember any
> reasoning why we don't flush on exit if any work has been done?
Actually the flush is necessary is 3 cases:
- ALLOCATE
- INSERT
- REMOVE
I don't count RELINQUISH because the guest is not scheduled anymore when
it happens.
REMOVE can never fail. In the case of ALLOCATE/INSERT, we redo the
mapping (REMOVE) which will issue a flush.
So for now we are safe. But I think, in general, the flush would be
better after the out label.
It would avoid missing flushing if one day we decide to implement
preemption on more use-case (such as INSERT/ALLOCATE).
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events.
2015-03-12 15:13 ` Tamas K Lengyel
2015-03-12 15:19 ` Tamas K Lengyel
@ 2015-03-12 15:30 ` Ian Campbell
1 sibling, 0 replies; 71+ messages in thread
From: Ian Campbell @ 2015-03-12 15:30 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, Stefano Stabellini, Tim Deegan, Julien Grall,
Ian Jackson, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
On Thu, 2015-03-12 at 16:13 +0100, Tamas K Lengyel wrote:
> and this function is nearly identical to the x86 one too.
> Nearly, but not completely. IMHO consolidation may be possible on some
> of these bits, but I'm not sure if it would make it any easier to
> follow when the code jumps back and forth between common and arch
> specific parts.
It's worse to have two similar bits of code which are supposed to do the
same thing but can get out of sync or behave subtly different.
Anyway, I'm happy for consolidation to come later.
>
>
> Would be nice not to have this static const thing twice
> in .rodata.
>
>
> What do you mean twice? One is converting from p2m_access to
> XENMEM_access, the other is XENMEM_access to p2m_access.
My mistake, they were similar but not similar enough.
>
> > +#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
> > + };
> > +
> > + /* If no setting was ever set, just return rwx. */
> > + if ( !p2m->mem_access_enabled )
> > + {
> > + *access = XENMEM_access_rwx;
> > + return 0;
> > + }
> > +
> > + /* If request to get default access */
> > + if ( gpfn == ~0ull )
>
> We should have a suitable constant for this, I think,
> INVALID_MFN looks
> like the one.
>
>
> ~0ull is specifically used by the mem_access API for this purpose. If
> anywhere, in the cleanup series it might make sense to have a #define
> added for it.
OK, can be done later then.
Ian.
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events.
2015-03-06 21:24 ` [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events Tamas K Lengyel
2015-03-12 13:35 ` Ian Campbell
@ 2015-03-12 15:13 ` Julien Grall
2015-03-12 15:26 ` Tamas K Lengyel
1 sibling, 1 reply; 71+ messages in thread
From: Julien Grall @ 2015-03-12 15:13 UTC (permalink / raw)
To: Tamas K Lengyel, xen-devel
Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson, tim,
stefano.stabellini, jbeulich, keir
Hi Tamas,
On 06/03/15 21:24, Tamas K Lengyel wrote:
> + /*
> + * Preempt setting mem_access permissions as required by XSA-89,
> + * if it's not the last iteration.
> + */
> + if ( op == MEMACCESS && count )
> + {
> + uint32_t progress = paddr_to_pfn(addr) - sgfn + 1;
> +
> + if ( (egfn - sgfn) > progress && !(progress & mask)
> + && hypercall_preempt_check() )
> + {
> + rc = progress;
> + goto out;
> + }
> + }
> +
Would it be possible to merge the memaccess prempt check with the
relinquish one?
That may require some change in the relinquish version but I think it
would be cleaner.
[..]
> +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
> +{
> + int rc;
> + bool_t violation;
> + xenmem_access_t xma;
> + mem_event_request_t *req;
> + struct vcpu *v = current;
> + struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
> +
> + /* Mem_access is not in use. */
> + if ( !p2m->mem_access_enabled )
> + return true;
true should be used with bool not boot_t.
> +
> + rc = p2m_get_mem_access(v->domain, paddr_to_pfn(gpa), &xma);
> + if ( rc )
> + return true;
Ditto (I won't repeat for the few other place below)
> +
> + /* Now check for mem_access violation. */
> + switch ( xma )
> + {
> + case XENMEM_access_rwx:
> + violation = false;
> + break;
> + case XENMEM_access_rw:
> + violation = npfec.insn_fetch;
> + break;
> + case XENMEM_access_wx:
> + violation = npfec.read_access;
> + break;
> + case XENMEM_access_rx:
> + case XENMEM_access_rx2rw:
> + violation = npfec.write_access;
> + break;
> + case XENMEM_access_x:
> + violation = npfec.read_access || npfec.write_access;
> + break;
> + case XENMEM_access_w:
> + violation = npfec.read_access || npfec.insn_fetch;
> + break;
> + case XENMEM_access_r:
> + violation = npfec.write_access || npfec.insn_fetch;
> + break;
> + default:
> + case XENMEM_access_n:
> + case XENMEM_access_n2rwx:
> + violation = true;
> + break;
> + }
> +
> + if ( !violation )
> + return true;
Ditto
> +
> + /* First, handle rx2rw and n2rwx conversion automatically. */
> + if ( npfec.write_access && xma == XENMEM_access_rx2rw )
> + {
> + rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
> + 0, ~0, XENMEM_access_rw);
> + return false;
Same as true.
[..]
> + if ( !i )
> + {
> + /*
> + * No setting was found in the Radix tree. Check if the
> + * entry exists in the page-tables.
> + */
> + paddr_t maddr = p2m_lookup(d, gpfn << PAGE_SHIFT, NULL);
> + if ( INVALID_PADDR == maddr )
> + return -ESRCH;
> +
> + /* If entry exists then its rwx. */
> + *access = XENMEM_access_rwx;
> + }
> + else
> + {
> + /* Setting was found in the Radix tree. */
> + index = radix_tree_ptr_to_int(i);
> + if ( index >= ARRAY_SIZE(memaccess) )
> + return -ERANGE;
We trust the value stored in the radix tree. So I think this could be
turned into an ASSERT/BUG_ON.
[..]
> @@ -243,12 +245,17 @@ static inline bool_t p2m_mem_event_sanity_check(struct domain *d)
>
> /* Get access type for a pfn
> * If pfn == -1ul, gets the default access type */
> -static inline
> int p2m_get_mem_access(struct domain *d, unsigned long pfn,
> - xenmem_access_t *access)
> -{
> - return -ENOSYS;
> -}
> + xenmem_access_t *access);
> +
p2m_get_mem_access is called by the common code. So it should be moved
in xen/include/xen/p2m-common.h
In general, any function called by common code should be have the
prototype declared in common header.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events.
2015-03-12 15:13 ` Julien Grall
@ 2015-03-12 15:26 ` Tamas K Lengyel
2015-03-12 15:37 ` Julien Grall
0 siblings, 1 reply; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-12 15:26 UTC (permalink / raw)
To: Julien Grall
Cc: wei.liu2, Ian Campbell, Stefano Stabellini, Tim Deegan,
Ian Jackson, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 4770 bytes --]
On Thu, Mar 12, 2015 at 4:13 PM, Julien Grall <julien.grall@linaro.org>
wrote:
> Hi Tamas,
>
> On 06/03/15 21:24, Tamas K Lengyel wrote:
> > + /*
> > + * Preempt setting mem_access permissions as required by XSA-89,
> > + * if it's not the last iteration.
> > + */
> > + if ( op == MEMACCESS && count )
> > + {
> > + uint32_t progress = paddr_to_pfn(addr) - sgfn + 1;
> > +
> > + if ( (egfn - sgfn) > progress && !(progress & mask)
> > + && hypercall_preempt_check() )
> > + {
> > + rc = progress;
> > + goto out;
> > + }
> > + }
> > +
>
> Would it be possible to merge the memaccess prempt check with the
> relinquish one?
>
> That may require some change in the relinquish version but I think it
> would be cleaner.
>
Well, I don't really see an obvious way how the two could be combined.
>
> [..]
>
> > +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct
> npfec npfec)
> > +{
> > + int rc;
> > + bool_t violation;
> > + xenmem_access_t xma;
> > + mem_event_request_t *req;
> > + struct vcpu *v = current;
> > + struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
> > +
> > + /* Mem_access is not in use. */
> > + if ( !p2m->mem_access_enabled )
> > + return true;
>
> true should be used with bool not boot_t.
>
> > +
> > + rc = p2m_get_mem_access(v->domain, paddr_to_pfn(gpa), &xma);
> > + if ( rc )
> > + return true;
>
> Ditto (I won't repeat for the few other place below)
>
There was just a discussion how there is no difference between 0/1 and
false/true other than maybe style. If one is preferred over the other, I'm
fine with either. Is this really an issue?
>
> > +
> > + /* Now check for mem_access violation. */
> > + switch ( xma )
> > + {
> > + case XENMEM_access_rwx:
> > + violation = false;
> > + break;
> > + case XENMEM_access_rw:
> > + violation = npfec.insn_fetch;
> > + break;
> > + case XENMEM_access_wx:
> > + violation = npfec.read_access;
> > + break;
> > + case XENMEM_access_rx:
> > + case XENMEM_access_rx2rw:
> > + violation = npfec.write_access;
> > + break;
> > + case XENMEM_access_x:
> > + violation = npfec.read_access || npfec.write_access;
> > + break;
> > + case XENMEM_access_w:
> > + violation = npfec.read_access || npfec.insn_fetch;
> > + break;
> > + case XENMEM_access_r:
> > + violation = npfec.write_access || npfec.insn_fetch;
> > + break;
> > + default:
> > + case XENMEM_access_n:
> > + case XENMEM_access_n2rwx:
> > + violation = true;
> > + break;
> > + }
> > +
> > + if ( !violation )
> > + return true;
>
> Ditto
>
> > +
> > + /* First, handle rx2rw and n2rwx conversion automatically. */
> > + if ( npfec.write_access && xma == XENMEM_access_rx2rw )
> > + {
> > + rc = p2m_set_mem_access(v->domain, paddr_to_pfn(gpa), 1,
> > + 0, ~0, XENMEM_access_rw);
> > + return false;
>
> Same as true.
>
> [..]
>
> > + if ( !i )
> > + {
> > + /*
> > + * No setting was found in the Radix tree. Check if the
> > + * entry exists in the page-tables.
> > + */
> > + paddr_t maddr = p2m_lookup(d, gpfn << PAGE_SHIFT, NULL);
> > + if ( INVALID_PADDR == maddr )
> > + return -ESRCH;
> > +
> > + /* If entry exists then its rwx. */
> > + *access = XENMEM_access_rwx;
> > + }
> > + else
> > + {
> > + /* Setting was found in the Radix tree. */
> > + index = radix_tree_ptr_to_int(i);
> > + if ( index >= ARRAY_SIZE(memaccess) )
> > + return -ERANGE;
>
> We trust the value stored in the radix tree. So I think this could be
> turned into an ASSERT/BUG_ON.
>
Sure.
>
> [..]
>
> > @@ -243,12 +245,17 @@ static inline bool_t
> p2m_mem_event_sanity_check(struct domain *d)
> >
> > /* Get access type for a pfn
> > * If pfn == -1ul, gets the default access type */
> > -static inline
> > int p2m_get_mem_access(struct domain *d, unsigned long pfn,
> > - xenmem_access_t *access)
> > -{
> > - return -ENOSYS;
> > -}
> > + xenmem_access_t *access);
> > +
>
> p2m_get_mem_access is called by the common code. So it should be moved
> in xen/include/xen/p2m-common.h
>
> In general, any function called by common code should be have the
> prototype declared in common header.
>
Ack, I'll move it into p2m-common.h once the function in ARM is actually
defined in the subsequent patch.
>
> Regards,
>
> --
> Julien Grall
>
Thanks,
Tamas
[-- Attachment #1.2: Type: text/html, Size: 7166 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events.
2015-03-12 15:26 ` Tamas K Lengyel
@ 2015-03-12 15:37 ` Julien Grall
2015-03-12 15:46 ` Ian Campbell
0 siblings, 1 reply; 71+ messages in thread
From: Julien Grall @ 2015-03-12 15:37 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, Ian Campbell, Stefano Stabellini, Tim Deegan,
Ian Jackson, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
On 12/03/15 15:26, Tamas K Lengyel wrote:
>
>
> On Thu, Mar 12, 2015 at 4:13 PM, Julien Grall <julien.grall@linaro.org
> <mailto:julien.grall@linaro.org>> wrote:
>
> Hi Tamas,
>
> On 06/03/15 21:24, Tamas K Lengyel wrote:
> > + /*
> > + * Preempt setting mem_access permissions as required by XSA-89,
> > + * if it's not the last iteration.
> > + */
> > + if ( op == MEMACCESS && count )
> > + {
> > + uint32_t progress = paddr_to_pfn(addr) - sgfn + 1;
> > +
> > + if ( (egfn - sgfn) > progress && !(progress & mask)
> > + && hypercall_preempt_check() )
> > + {
> > + rc = progress;
> > + goto out;
> > + }
> > + }
> > +
>
> Would it be possible to merge the memaccess prempt check with the
> relinquish one?
>
> That may require some change in the relinquish version but I think it
> would be cleaner.
>
>
> Well, I don't really see an obvious way how the two could be combined.
The preemption for relinquish has been chosen arbitrarily so it would be
possible to change it.
>
> [..]
>
> > +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
> > +{
> > + int rc;
> > + bool_t violation;
> > + xenmem_access_t xma;
> > + mem_event_request_t *req;
> > + struct vcpu *v = current;
> > + struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
> > +
> > + /* Mem_access is not in use. */
> > + if ( !p2m->mem_access_enabled )
> > + return true;
>
> true should be used with bool not boot_t.
>
> > +
> > + rc = p2m_get_mem_access(v->domain, paddr_to_pfn(gpa), &xma);
> > + if ( rc )
> > + return true;
>
> Ditto (I won't repeat for the few other place below)
>
>
> There was just a discussion how there is no difference between 0/1 and
> false/true other than maybe style. If one is preferred over the other,
> I'm fine with either. Is this really an issue?
You are mixing 2 different types. bool/false/true are part of stdbool.h
rather than bool_t is part of asm/types.h
By mistake we mix both type in an handful of places in ARM. We should
avoid to introduce more.
FWIW, the headers stdbool.h has been introduced in order to share libelf
with the tools.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events.
2015-03-12 15:37 ` Julien Grall
@ 2015-03-12 15:46 ` Ian Campbell
2015-03-12 16:54 ` Julien Grall
0 siblings, 1 reply; 71+ messages in thread
From: Ian Campbell @ 2015-03-12 15:46 UTC (permalink / raw)
To: Julien Grall
Cc: wei.liu2, Stefano Stabellini, Tim Deegan, Ian Jackson, xen-devel,
stefano.stabellini, Jan Beulich, Keir Fraser, Tamas K Lengyel
On Thu, 2015-03-12 at 15:37 +0000, Julien Grall wrote:
> On 12/03/15 15:26, Tamas K Lengyel wrote:
> >
> >
> > On Thu, Mar 12, 2015 at 4:13 PM, Julien Grall <julien.grall@linaro.org
> > <mailto:julien.grall@linaro.org>> wrote:
> >
> > Hi Tamas,
> >
> > On 06/03/15 21:24, Tamas K Lengyel wrote:
> > > + /*
> > > + * Preempt setting mem_access permissions as required by XSA-89,
> > > + * if it's not the last iteration.
> > > + */
> > > + if ( op == MEMACCESS && count )
> > > + {
> > > + uint32_t progress = paddr_to_pfn(addr) - sgfn + 1;
> > > +
> > > + if ( (egfn - sgfn) > progress && !(progress & mask)
> > > + && hypercall_preempt_check() )
> > > + {
> > > + rc = progress;
> > > + goto out;
> > > + }
> > > + }
> > > +
> >
> > Would it be possible to merge the memaccess prempt check with the
> > relinquish one?
> >
> > That may require some change in the relinquish version but I think it
> > would be cleaner.
> >
> >
> > Well, I don't really see an obvious way how the two could be combined.
>
> The preemption for relinquish has been chosen arbitrarily so it would be
> possible to change it.
>
> >
> > [..]
> >
> > > +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
> > > +{
> > > + int rc;
> > > + bool_t violation;
> > > + xenmem_access_t xma;
> > > + mem_event_request_t *req;
> > > + struct vcpu *v = current;
> > > + struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
> > > +
> > > + /* Mem_access is not in use. */
> > > + if ( !p2m->mem_access_enabled )
> > > + return true;
> >
> > true should be used with bool not boot_t.
> >
> > > +
> > > + rc = p2m_get_mem_access(v->domain, paddr_to_pfn(gpa), &xma);
> > > + if ( rc )
> > > + return true;
> >
> > Ditto (I won't repeat for the few other place below)
> >
> >
> > There was just a discussion how there is no difference between 0/1 and
> > false/true other than maybe style. If one is preferred over the other,
> > I'm fine with either. Is this really an issue?
>
> You are mixing 2 different types. bool/false/true are part of stdbool.h
> rather than bool_t is part of asm/types.h
I don't think this matters one jot.
bool foo = true;
bool_t foo = true;
are both perfectly fine, work as expected (since both are equivalent to
= !0), and are easier to read.
Why do you think this is so important?
Ian.
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events.
2015-03-12 15:46 ` Ian Campbell
@ 2015-03-12 16:54 ` Julien Grall
0 siblings, 0 replies; 71+ messages in thread
From: Julien Grall @ 2015-03-12 16:54 UTC (permalink / raw)
To: Ian Campbell
Cc: wei.liu2, Stefano Stabellini, Tim Deegan, Ian Jackson, xen-devel,
stefano.stabellini, Jan Beulich, Keir Fraser, Tamas K Lengyel
On 12/03/15 15:46, Ian Campbell wrote:
> On Thu, 2015-03-12 at 15:37 +0000, Julien Grall wrote:
>> On 12/03/15 15:26, Tamas K Lengyel wrote:
>>>
>>>
>>> On Thu, Mar 12, 2015 at 4:13 PM, Julien Grall <julien.grall@linaro.org
>>> <mailto:julien.grall@linaro.org>> wrote:
>>>
>>> Hi Tamas,
>>>
>>> On 06/03/15 21:24, Tamas K Lengyel wrote:
>>> > + /*
>>> > + * Preempt setting mem_access permissions as required by XSA-89,
>>> > + * if it's not the last iteration.
>>> > + */
>>> > + if ( op == MEMACCESS && count )
>>> > + {
>>> > + uint32_t progress = paddr_to_pfn(addr) - sgfn + 1;
>>> > +
>>> > + if ( (egfn - sgfn) > progress && !(progress & mask)
>>> > + && hypercall_preempt_check() )
>>> > + {
>>> > + rc = progress;
>>> > + goto out;
>>> > + }
>>> > + }
>>> > +
>>>
>>> Would it be possible to merge the memaccess prempt check with the
>>> relinquish one?
>>>
>>> That may require some change in the relinquish version but I think it
>>> would be cleaner.
>>>
>>>
>>> Well, I don't really see an obvious way how the two could be combined.
>>
>> The preemption for relinquish has been chosen arbitrarily so it would be
>> possible to change it.
>>
>>>
>>> [..]
>>>
>>> > +bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
>>> > +{
>>> > + int rc;
>>> > + bool_t violation;
>>> > + xenmem_access_t xma;
>>> > + mem_event_request_t *req;
>>> > + struct vcpu *v = current;
>>> > + struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
>>> > +
>>> > + /* Mem_access is not in use. */
>>> > + if ( !p2m->mem_access_enabled )
>>> > + return true;
>>>
>>> true should be used with bool not boot_t.
>>>
>>> > +
>>> > + rc = p2m_get_mem_access(v->domain, paddr_to_pfn(gpa), &xma);
>>> > + if ( rc )
>>> > + return true;
>>>
>>> Ditto (I won't repeat for the few other place below)
>>>
>>>
>>> There was just a discussion how there is no difference between 0/1 and
>>> false/true other than maybe style. If one is preferred over the other,
>>> I'm fine with either. Is this really an issue?
>>
>> You are mixing 2 different types. bool/false/true are part of stdbool.h
>> rather than bool_t is part of asm/types.h
>
> I don't think this matters one jot.
>
> bool foo = true;
> bool_t foo = true;
>
> are both perfectly fine, work as expected (since both are equivalent to
> = !0), and are easier to read.
>
> Why do you think this is so important?
Using true/false means we have to include stdbool.h. This file is only
there for library shared with the toolstack. It happens to be included
in p2m.c by mistake...
Anyway, we are taking the liberty for re-using different define for
another purpose. If we really want to use false/true with bool_t we
should at least move them in the same header.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH V13 5/7] xen/arm: Instruction prefetch abort (X) mem_event handling
2015-03-06 21:24 [PATCH V13 0/7] Mem_access for ARM Tamas K Lengyel
` (3 preceding siblings ...)
2015-03-06 21:24 ` [PATCH V13 4/7] xen/arm: Data abort exception (R/W) mem_events Tamas K Lengyel
@ 2015-03-06 21:24 ` Tamas K Lengyel
2015-03-23 14:32 ` Tamas K Lengyel
2015-03-06 21:24 ` [PATCH V13 6/7] xen/arm: Enable mem_access on ARM Tamas K Lengyel
` (2 subsequent siblings)
7 siblings, 1 reply; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-06 21:24 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson,
julien.grall, tim, stefano.stabellini, jbeulich, keir,
Tamas K Lengyel
Add missing structure definition for iabt and update the trap handling
mechanism to only inject the exception if the mem_access checker
decides to do so.
Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
v10: - Minor comment fix for describing s1ptw.
v8: - Revert to arch specific p2m_mem_access_check.
- Retire iabt_fsc enum and use FSC_FLT instead.
- Complete the struct definition of hsr_iabt.
v7: - Use the new common mem_access_check.
v6: - Make npfec a const.
v4: - Don't mark instruction fetch violation as read violation.
- Use new struct npfec to pass violation info.
v2: - Add definition for instruction abort instruction fetch status codes
(enum iabt_ifsc) and only call p2m_mem_access_check for traps triggered
for permission violations.
---
xen/arch/arm/traps.c | 31 +++++++++++++++++++++++++++++--
xen/include/asm-arm/processor.h | 11 +++++++++++
2 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f5aa647..0670145 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1961,8 +1961,35 @@ done:
static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
union hsr hsr)
{
- register_t addr = READ_SYSREG(FAR_EL2);
- inject_iabt_exception(regs, addr, hsr.len);
+ struct hsr_iabt iabt = hsr.iabt;
+ int rc;
+ paddr_t gpa;
+ register_t gva = READ_SYSREG(FAR_EL2);
+
+ rc = gva_to_ipa(gva, &gpa);
+ if ( -EFAULT == rc )
+ return;
+
+ switch ( iabt.ifsc & 0x3f )
+ {
+ case FSC_FLT_PERM ... FSC_FLT_PERM + 3:
+ {
+ const struct npfec npfec = {
+ .insn_fetch = 1,
+ .gla_valid = 1,
+ .kind = iabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
+ };
+
+ rc = p2m_mem_access_check(gpa, gva, npfec);
+
+ /* Trap was triggered by mem_access, work here is done */
+ if ( !rc )
+ return;
+ }
+ break;
+ }
+
+ inject_iabt_exception(regs, gva, hsr.len);
}
static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index cf7ab7c..db07fdd 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -438,6 +438,17 @@ union hsr {
} sysreg; /* HSR_EC_SYSREG */
#endif
+ struct hsr_iabt {
+ unsigned long ifsc:6; /* Instruction fault status code */
+ unsigned long res0:1;
+ unsigned long s1ptw:1; /* Stage 2 fault during stage 1 translation */
+ unsigned long res1:1;
+ unsigned long eat:1; /* External abort type */
+ unsigned long res2:15;
+ unsigned long len:1; /* Instruction length */
+ unsigned long ec:6; /* Exception Class */
+ } iabt; /* HSR_EC_INSTR_ABORT_* */
+
struct hsr_dabt {
unsigned long dfsc:6; /* Data Fault Status Code */
unsigned long write:1; /* Write / not Read */
--
2.1.4
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [PATCH V13 5/7] xen/arm: Instruction prefetch abort (X) mem_event handling
2015-03-06 21:24 ` [PATCH V13 5/7] xen/arm: Instruction prefetch abort (X) mem_event handling Tamas K Lengyel
@ 2015-03-23 14:32 ` Tamas K Lengyel
2015-03-23 15:15 ` Ian Campbell
0 siblings, 1 reply; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-23 14:32 UTC (permalink / raw)
To: xen-devel
Cc: stefano.stabellini, Julien Grall, Ian Jackson, Stefano Stabellini
[-- Attachment #1.1: Type: text/plain, Size: 3882 bytes --]
On Fri, Mar 6, 2015 at 10:24 PM, Tamas K Lengyel <tklengyel@sec.in.tum.de>
wrote:
> Add missing structure definition for iabt and update the trap handling
> mechanism to only inject the exception if the mem_access checker
> decides to do so.
>
> Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> Reviewed-by: Julien Grall <julien.grall@linaro.org>
> ---
> v10: - Minor comment fix for describing s1ptw.
> v8: - Revert to arch specific p2m_mem_access_check.
> - Retire iabt_fsc enum and use FSC_FLT instead.
> - Complete the struct definition of hsr_iabt.
> v7: - Use the new common mem_access_check.
> v6: - Make npfec a const.
> v4: - Don't mark instruction fetch violation as read violation.
> - Use new struct npfec to pass violation info.
> v2: - Add definition for instruction abort instruction fetch status codes
> (enum iabt_ifsc) and only call p2m_mem_access_check for traps
> triggered
> for permission violations.
> ---
> xen/arch/arm/traps.c | 31 +++++++++++++++++++++++++++++--
> xen/include/asm-arm/processor.h | 11 +++++++++++
> 2 files changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index f5aa647..0670145 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1961,8 +1961,35 @@ done:
> static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
> union hsr hsr)
> {
> - register_t addr = READ_SYSREG(FAR_EL2);
> - inject_iabt_exception(regs, addr, hsr.len);
> + struct hsr_iabt iabt = hsr.iabt;
> + int rc;
> + paddr_t gpa;
> + register_t gva = READ_SYSREG(FAR_EL2);
>
>
So I have a question here. The following call to gva_to_ipa will use the
MMU to translate the gva as if it was a data-read access. However, we got
here because of an instruction fetch access. I did a quick check and (at
least some) ARM CPUs have split-TLBs. So technically using gva_to_ipa here
could get us an IPA that wasn't the actual address if the guest pagetable
has since been updated and the TLBs primed. Should the TLB be flushed here
just to be sure we have an accurate translation?
> + rc = gva_to_ipa(gva, &gpa);
> + if ( -EFAULT == rc )
> + return;
> +
> + switch ( iabt.ifsc & 0x3f )
> + {
> + case FSC_FLT_PERM ... FSC_FLT_PERM + 3:
> + {
> + const struct npfec npfec = {
> + .insn_fetch = 1,
> + .gla_valid = 1,
> + .kind = iabt.s1ptw ? npfec_kind_in_gpt : npfec_kind_with_gla
> + };
> +
> + rc = p2m_mem_access_check(gpa, gva, npfec);
> +
> + /* Trap was triggered by mem_access, work here is done */
> + if ( !rc )
> + return;
> + }
> + break;
> + }
> +
> + inject_iabt_exception(regs, gva, hsr.len);
> }
>
> static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
> diff --git a/xen/include/asm-arm/processor.h
> b/xen/include/asm-arm/processor.h
> index cf7ab7c..db07fdd 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -438,6 +438,17 @@ union hsr {
> } sysreg; /* HSR_EC_SYSREG */
> #endif
>
> + struct hsr_iabt {
> + unsigned long ifsc:6; /* Instruction fault status code */
> + unsigned long res0:1;
> + unsigned long s1ptw:1; /* Stage 2 fault during stage 1
> translation */
> + unsigned long res1:1;
> + unsigned long eat:1; /* External abort type */
> + unsigned long res2:15;
> + unsigned long len:1; /* Instruction length */
> + unsigned long ec:6; /* Exception Class */
> + } iabt; /* HSR_EC_INSTR_ABORT_* */
> +
> struct hsr_dabt {
> unsigned long dfsc:6; /* Data Fault Status Code */
> unsigned long write:1; /* Write / not Read */
> --
> 2.1.4
>
>
[-- Attachment #1.2: Type: text/html, Size: 5025 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 5/7] xen/arm: Instruction prefetch abort (X) mem_event handling
2015-03-23 14:32 ` Tamas K Lengyel
@ 2015-03-23 15:15 ` Ian Campbell
2015-03-23 15:18 ` Ian Campbell
0 siblings, 1 reply; 71+ messages in thread
From: Ian Campbell @ 2015-03-23 15:15 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Ian Jackson, Julien Grall, stefano.stabellini, Stefano Stabellini,
xen-devel
On Mon, 2015-03-23 at 15:32 +0100, Tamas K Lengyel wrote:
> + register_t gva = READ_SYSREG(FAR_EL2);
>
>
>
> So I have a question here. The following call to gva_to_ipa will use
> the MMU to translate the gva as if it was a data-read access. However,
> we got here because of an instruction fetch access. I did a quick
> check and (at least some) ARM CPUs have split-TLBs. So technically
> using gva_to_ipa here could get us an IPA that wasn't the actual
> address if the guest pagetable has since been updated and the TLBs
> primed. Should the TLB be flushed here just to be sure we have an
> accurate translation?
Interesting question, I'd need to spend some time with the ARM ARM to
figure out what is allowable here both in terms of seeing stale entries
or even in whether split TLBs are allowed on modern ARM (i.e. I'm not
sure if split-TLBs are allowed on architectures new enough to have virt
extensions or not).
My initial gut reaction is that if the guest has updated a pagetable but
not flushed the I-TLB then it would be permissible for it to see stale
mappings, even on native. The documented procedure for updating a
mapping of text space involves lots of barriers and flushes etc.
_But_ I suppose you are not really worried about the guest doing a PT
update, but rather xenaccess playing games with the stage 2 behind the
guest's back, which might require us to do some TLB shootdowns, and we'd
have to assume both I-TLB and D-TLB since we don't know what the guest
has in those pages.
So there might well be a missing TLB flush somewhere, but it may not be
in this code...
Ian.
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 5/7] xen/arm: Instruction prefetch abort (X) mem_event handling
2015-03-23 15:15 ` Ian Campbell
@ 2015-03-23 15:18 ` Ian Campbell
2015-03-23 15:47 ` Tamas K Lengyel
0 siblings, 1 reply; 71+ messages in thread
From: Ian Campbell @ 2015-03-23 15:18 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Ian Jackson, Julien Grall, stefano.stabellini, Stefano Stabellini,
xen-devel
On Mon, 2015-03-23 at 15:15 +0000, Ian Campbell wrote:
> _But_ I suppose you are not really worried about the guest doing a PT
> update, but rather xenaccess playing games with the stage 2 behind the
> guest's back, which might require us to do some TLB shootdowns, and we'd
> have to assume both I-TLB and D-TLB since we don't know what the guest
> has in those pages.
When we change the p2m we do a tlbi vmalle1is, so regardless of whether
split tlb is a thing we are flushing everything, so we should be good
from a stage 2 PoV.
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 5/7] xen/arm: Instruction prefetch abort (X) mem_event handling
2015-03-23 15:18 ` Ian Campbell
@ 2015-03-23 15:47 ` Tamas K Lengyel
2015-03-23 16:22 ` Ian Campbell
0 siblings, 1 reply; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-23 15:47 UTC (permalink / raw)
To: Ian Campbell
Cc: Ian Jackson, Julien Grall, stefano.stabellini, Stefano Stabellini,
xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 1490 bytes --]
On Mon, Mar 23, 2015 at 4:18 PM, Ian Campbell <ian.campbell@citrix.com>
wrote:
> On Mon, 2015-03-23 at 15:15 +0000, Ian Campbell wrote:
> > _But_ I suppose you are not really worried about the guest doing a PT
> > update, but rather xenaccess playing games with the stage 2 behind the
> > guest's back, which might require us to do some TLB shootdowns, and we'd
> > have to assume both I-TLB and D-TLB since we don't know what the guest
> > has in those pages.
>
> When we change the p2m we do a tlbi vmalle1is, so regardless of whether
> split tlb is a thing we are flushing everything, so we should be good
> from a stage 2 PoV.
>
When we apply p2m changes we do indeed flush the TLB. I do actually worry
about a potential malicious in-guest kernel playing tricks with its
pagetables which may have happened after the p2m changes were applied. The
xen-access permissions are applied based on IPAs. Say I want to be notified
when a specific API is being called, so I walk the guest-pagestables and
set a trap at the page the IPA is one. If the malicious guest kernel wants
to avoid triggering the xen-access notifications, it could theoretically
prime its tables and perform the right accesses so that the iTLB still has
the mapping I wanted to trap with xen-access, but the dTLB has a different
mapping. The instruction abort trap will happen but in Xen I will see the
mapping according to the dTLB, thus the radix-tree lookup fails and the
trap is injected back into the guest.
Tamas
[-- Attachment #1.2: Type: text/html, Size: 1934 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 5/7] xen/arm: Instruction prefetch abort (X) mem_event handling
2015-03-23 15:47 ` Tamas K Lengyel
@ 2015-03-23 16:22 ` Ian Campbell
2015-03-23 16:47 ` Tamas K Lengyel
0 siblings, 1 reply; 71+ messages in thread
From: Ian Campbell @ 2015-03-23 16:22 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: stefano.stabellini, xen-devel, Julien Grall, Ian Jackson,
Stefano Stabellini
On Mon, 2015-03-23 at 16:47 +0100, Tamas K Lengyel wrote:
>
>
> On Mon, Mar 23, 2015 at 4:18 PM, Ian Campbell
> <ian.campbell@citrix.com> wrote:
> On Mon, 2015-03-23 at 15:15 +0000, Ian Campbell wrote:
> > _But_ I suppose you are not really worried about the guest
> doing a PT
> > update, but rather xenaccess playing games with the stage 2
> behind the
> > guest's back, which might require us to do some TLB
> shootdowns, and we'd
> > have to assume both I-TLB and D-TLB since we don't know what
> the guest
> > has in those pages.
>
> When we change the p2m we do a tlbi vmalle1is, so regardless
> of whether
> split tlb is a thing we are flushing everything, so we should
> be good
> from a stage 2 PoV.
>
>
> When we apply p2m changes we do indeed flush the TLB. I do actually
> worry about a potential malicious in-guest kernel playing tricks with
> its pagetables which may have happened after the p2m changes were
> applied. The xen-access permissions are applied based on IPAs. Say I
> want to be notified when a specific API is being called, so I walk the
> guest-pagestables and set a trap at the page the IPA is one. If the
> malicious guest kernel wants to avoid triggering the xen-access
> notifications, it could theoretically prime its tables and perform the
> right accesses so that the iTLB still has the mapping I wanted to trap
> with xen-access, but the dTLB has a different mapping. The instruction
> abort trap will happen but in Xen I will see the mapping according to
> the dTLB, thus the radix-tree lookup fails and the trap is injected
> back into the guest.
But in that case the guest will take a trap, so what has it gained other
than a spurious trap?
I can't see anything in the TLB chapter of the ARMv8 ARM which suggests
split TLBs are possible in AArch64 mode, and the AArch32 refs look like
backwards compat stuff to me.
The ARMv7 ARM does mention the split, but only to say that the
distinction between the ITLB and DTLB maintenance operations is
deprecated and that modern TLB ops do not support the distinction. It's
not 100% unambiguous about whether split TLBs themselves are allowed in
ARMv7 though, and in particular I'm not sure what impact the addition of
the LPAE and virtualisation extensions will have had on these
requirements. I also can't quite tell if split TLB is allowed on VMSA
systems, or if it is PMSA only.
As you might imagine I'm not keen on adding TLB flushes "just in case",
so I would really like to see some good references to the architecture
specs before adding a flush on this path.
Ian.
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 5/7] xen/arm: Instruction prefetch abort (X) mem_event handling
2015-03-23 16:22 ` Ian Campbell
@ 2015-03-23 16:47 ` Tamas K Lengyel
2015-03-24 13:06 ` Tamas K Lengyel
0 siblings, 1 reply; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-23 16:47 UTC (permalink / raw)
To: Ian Campbell
Cc: Stefano Stabellini, Julien Grall, Ian Jackson,
xen-devel@lists.xen.org, Stefano Stabellini, Tamas K Lengyel
On Mon, Mar 23, 2015 at 5:22 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Mon, 2015-03-23 at 16:47 +0100, Tamas K Lengyel wrote:
>>
>>
>> On Mon, Mar 23, 2015 at 4:18 PM, Ian Campbell
>> <ian.campbell@citrix.com> wrote:
>> On Mon, 2015-03-23 at 15:15 +0000, Ian Campbell wrote:
>> > _But_ I suppose you are not really worried about the guest
>> doing a PT
>> > update, but rather xenaccess playing games with the stage 2
>> behind the
>> > guest's back, which might require us to do some TLB
>> shootdowns, and we'd
>> > have to assume both I-TLB and D-TLB since we don't know what
>> the guest
>> > has in those pages.
>>
>> When we change the p2m we do a tlbi vmalle1is, so regardless
>> of whether
>> split tlb is a thing we are flushing everything, so we should
>> be good
>> from a stage 2 PoV.
>>
>>
>> When we apply p2m changes we do indeed flush the TLB. I do actually
>> worry about a potential malicious in-guest kernel playing tricks with
>> its pagetables which may have happened after the p2m changes were
>> applied. The xen-access permissions are applied based on IPAs. Say I
>> want to be notified when a specific API is being called, so I walk the
>> guest-pagestables and set a trap at the page the IPA is one. If the
>> malicious guest kernel wants to avoid triggering the xen-access
>> notifications, it could theoretically prime its tables and perform the
>> right accesses so that the iTLB still has the mapping I wanted to trap
>> with xen-access, but the dTLB has a different mapping. The instruction
>> abort trap will happen but in Xen I will see the mapping according to
>> the dTLB, thus the radix-tree lookup fails and the trap is injected
>> back into the guest.
>
> But in that case the guest will take a trap, so what has it gained other
> than a spurious trap?
Bypassing the notification being sent to the subscriber application
about the trap.
>
> I can't see anything in the TLB chapter of the ARMv8 ARM which suggests
> split TLBs are possible in AArch64 mode, and the AArch32 refs look like
> backwards compat stuff to me.
>
> The ARMv7 ARM does mention the split, but only to say that the
> distinction between the ITLB and DTLB maintenance operations is
> deprecated and that modern TLB ops do not support the distinction. It's
> not 100% unambiguous about whether split TLBs themselves are allowed in
> ARMv7 though, and in particular I'm not sure what impact the addition of
> the LPAE and virtualisation extensions will have had on these
> requirements. I also can't quite tell if split TLB is allowed on VMSA
> systems, or if it is PMSA only.
>
> As you might imagine I'm not keen on adding TLB flushes "just in case",
> so I would really like to see some good references to the architecture
> specs before adding a flush on this path.
>
> Ian.
IMHO a TLB flush in this path is low impact for systems that don't use
xen-access. As far as which hardware supports it, I see the following
for A53 (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0500f/BIIJFHJD.html):
10-entry fully-associative instruction micro TLB.
10-entry fully-associative data micro TLB.
For A57 (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0500f/BIIJFHJD.html):
48-entry fully-associative L1 instruction TLB.
32-entry fully-associative L1 data TLB for data load and store pipelines.
Tamas
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 5/7] xen/arm: Instruction prefetch abort (X) mem_event handling
2015-03-23 16:47 ` Tamas K Lengyel
@ 2015-03-24 13:06 ` Tamas K Lengyel
2015-03-26 10:50 ` Ian Campbell
0 siblings, 1 reply; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-24 13:06 UTC (permalink / raw)
To: Ian Campbell
Cc: Stefano Stabellini, Julien Grall, Ian Jackson,
xen-devel@lists.xen.org, Stefano Stabellini, Tamas K Lengyel
[-- Attachment #1.1: Type: text/plain, Size: 4760 bytes --]
On Mon, Mar 23, 2015 at 5:47 PM, Tamas K Lengyel <tamas.lengyel@zentific.com
> wrote:
> On Mon, Mar 23, 2015 at 5:22 PM, Ian Campbell <ian.campbell@citrix.com>
> wrote:
> > On Mon, 2015-03-23 at 16:47 +0100, Tamas K Lengyel wrote:
> >>
> >>
> >> On Mon, Mar 23, 2015 at 4:18 PM, Ian Campbell
> >> <ian.campbell@citrix.com> wrote:
> >> On Mon, 2015-03-23 at 15:15 +0000, Ian Campbell wrote:
> >> > _But_ I suppose you are not really worried about the guest
> >> doing a PT
> >> > update, but rather xenaccess playing games with the stage 2
> >> behind the
> >> > guest's back, which might require us to do some TLB
> >> shootdowns, and we'd
> >> > have to assume both I-TLB and D-TLB since we don't know what
> >> the guest
> >> > has in those pages.
> >>
> >> When we change the p2m we do a tlbi vmalle1is, so regardless
> >> of whether
> >> split tlb is a thing we are flushing everything, so we should
> >> be good
> >> from a stage 2 PoV.
> >>
> >>
> >> When we apply p2m changes we do indeed flush the TLB. I do actually
> >> worry about a potential malicious in-guest kernel playing tricks with
> >> its pagetables which may have happened after the p2m changes were
> >> applied. The xen-access permissions are applied based on IPAs. Say I
> >> want to be notified when a specific API is being called, so I walk the
> >> guest-pagestables and set a trap at the page the IPA is one. If the
> >> malicious guest kernel wants to avoid triggering the xen-access
> >> notifications, it could theoretically prime its tables and perform the
> >> right accesses so that the iTLB still has the mapping I wanted to trap
> >> with xen-access, but the dTLB has a different mapping. The instruction
> >> abort trap will happen but in Xen I will see the mapping according to
> >> the dTLB, thus the radix-tree lookup fails and the trap is injected
> >> back into the guest.
> >
> > But in that case the guest will take a trap, so what has it gained other
> > than a spurious trap?
>
> Bypassing the notification being sent to the subscriber application
> about the trap.
>
> >
> > I can't see anything in the TLB chapter of the ARMv8 ARM which suggests
> > split TLBs are possible in AArch64 mode, and the AArch32 refs look like
> > backwards compat stuff to me.
> >
> > The ARMv7 ARM does mention the split, but only to say that the
> > distinction between the ITLB and DTLB maintenance operations is
> > deprecated and that modern TLB ops do not support the distinction. It's
> > not 100% unambiguous about whether split TLBs themselves are allowed in
> > ARMv7 though, and in particular I'm not sure what impact the addition of
> > the LPAE and virtualisation extensions will have had on these
> > requirements. I also can't quite tell if split TLB is allowed on VMSA
> > systems, or if it is PMSA only.
> >
> > As you might imagine I'm not keen on adding TLB flushes "just in case",
> > so I would really like to see some good references to the architecture
> > specs before adding a flush on this path.
> >
> > Ian.
>
> IMHO a TLB flush in this path is low impact for systems that don't use
> xen-access. As far as which hardware supports it, I see the following
> for A53 (
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0500f/BIIJFHJD.html
> ):
>
> 10-entry fully-associative instruction micro TLB.
> 10-entry fully-associative data micro TLB.
>
> For A57 (
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0500f/BIIJFHJD.html
> ):
>
> 48-entry fully-associative L1 instruction TLB.
> 32-entry fully-associative L1 data TLB for data load and store pipelines.
>
> Tamas
>
Ian,
according to the ARM ARM v8 split TLB is possible, see section TLB matching
(page 1826): "In some cases, the TLB can hold two mappings for the same
address". In fact, it seems like some hardware can even detect such cases
and cause a Data-abort or Prefetch abort with error code TLB Conflict (see
section TLB conflict aborts, page 1827). They did indeed deprecate the
separate maintenance operations but that doesn't really effect the problem
I'm describing.
It's unfortunate we can't make the MMU get us the translation as if doing a
prefetch, only as a data access. It's also unfortunate that during a
Permission fault we don't get the IPA associated with the fault, only the
GVA. I looked at the KVM code and they seem to query HPFAR_EL2 if the fault
is during s1ptw, but otherwise they do exactly what we do here.
So, IMHO doing a TLB flush here would prevent a primed DTLB becoming a
problem. Not sure if that helps if the ITLB is primed however as we would
have no way to replicate the translation that is cached..
Tamas
[-- Attachment #1.2: Type: text/html, Size: 6270 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 5/7] xen/arm: Instruction prefetch abort (X) mem_event handling
2015-03-24 13:06 ` Tamas K Lengyel
@ 2015-03-26 10:50 ` Ian Campbell
2015-03-26 11:24 ` Tamas K Lengyel
0 siblings, 1 reply; 71+ messages in thread
From: Ian Campbell @ 2015-03-26 10:50 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Stefano Stabellini, xen-devel@lists.xen.org, Julien Grall,
Ian Jackson, Stefano Stabellini
On Tue, 2015-03-24 at 14:06 +0100, Tamas K Lengyel wrote:
> according to the ARM ARM v8 split TLB is possible, see section TLB
> matching (page 1826): "In some cases, the TLB can hold two mappings
> for the same address". In fact, it seems like some hardware can even
> detect such cases and cause a Data-abort or Prefetch abort with error
> code TLB Conflict (see section TLB conflict aborts, page 1827). They
> did indeed deprecate the separate maintenance operations but that
> doesn't really effect the problem I'm describing.
(FWIW this is page 1754 in revision DDI0487A.d of the doc)
I'm not sure that the "In some cases" wording is talking about split
TLBs, but rather a potential occurrence with in a single TLB when
invalidation is not properly performed. If there were 2 TLBs I would
expect them to operate independently, so there would be no possibility
of a conflict.
> It's unfortunate we can't make the MMU get us the translation as if
> doing a prefetch, only as a data access.
Part of me thinks that if split TLBs were possible then an instruction
to do this would have been provided...
I'm going to ask around and see if I can find a definitive answer on
this.
> It's also unfortunate that during a Permission fault we don't get the
> IPA associated with the fault, only the GVA.
FWIW I think this is because an allowable implementation is to only
cache the final PA+perms result of Stage 1+2 rather than caching the two
stages separately (which is also allowed), so it's possible that the h/w
wouldn't have the IPA in its hand.
> I looked at the KVM code and they seem to query HPFAR_EL2 if the
> fault is during s1ptw, but otherwise they do exactly what we do here.
Yes, reading their comment there lead me to the bit of the ARM ARM and I
think there may be improvements we could make to the non-xenaccess case
at least, I was confused about HPFAR_EL2 va FAR_EL2 earlier.
> So, IMHO doing a TLB flush here would prevent a primed DTLB becoming a
> problem. Not sure if that helps if the ITLB is primed however as we
> would have no way to replicate the translation that is cached..
Ian.
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 5/7] xen/arm: Instruction prefetch abort (X) mem_event handling
2015-03-26 10:50 ` Ian Campbell
@ 2015-03-26 11:24 ` Tamas K Lengyel
2015-03-26 11:53 ` Ian Campbell
0 siblings, 1 reply; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-26 11:24 UTC (permalink / raw)
To: Ian Campbell
Cc: Stefano Stabellini, Julien Grall, Ian Jackson,
xen-devel@lists.xen.org, Stefano Stabellini, Tamas K Lengyel
On Thu, Mar 26, 2015 at 11:50 AM, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Tue, 2015-03-24 at 14:06 +0100, Tamas K Lengyel wrote:
>
>> according to the ARM ARM v8 split TLB is possible, see section TLB
>> matching (page 1826): "In some cases, the TLB can hold two mappings
>> for the same address". In fact, it seems like some hardware can even
>> detect such cases and cause a Data-abort or Prefetch abort with error
>> code TLB Conflict (see section TLB conflict aborts, page 1827). They
>> did indeed deprecate the separate maintenance operations but that
>> doesn't really effect the problem I'm describing.
>
> (FWIW this is page 1754 in revision DDI0487A.d of the doc)
Sorry, I was going by the PDF page # not their own.
>
> I'm not sure that the "In some cases" wording is talking about split
> TLBs, but rather a potential occurrence with in a single TLB when
> invalidation is not properly performed. If there were 2 TLBs I would
> expect them to operate independently, so there would be no possibility
> of a conflict.
It is certainly not clear what it refers to but logically thinking
about it, two entries within the same sub-TLB would not be possible to
be created since the look-up that happens for the address would have
succeeded already, thus bypassing the path that inserts a new entry.
The only scenario where I can imagine this happen (and make sense) is
with the split-TLB where one entry is in the ITLB the other in the
DTLB, and where cache maintenance operations were bypassed either by
accident or on purpose. Of course, who knows what the hardware is
actually doing, if it is possible to have two entries for the same
address within the DTLB for example that would sound very strange to
me.
>
>> It's unfortunate we can't make the MMU get us the translation as if
>> doing a prefetch, only as a data access.
>
> Part of me thinks that if split TLBs were possible then an instruction
> to do this would have been provided...
>
> I'm going to ask around and see if I can find a definitive answer on
> this.
Maybe, but who knows? Design flaws do happen.. Keep me posted if you
find anything useful =)
>
>> It's also unfortunate that during a Permission fault we don't get the
>> IPA associated with the fault, only the GVA.
>
> FWIW I think this is because an allowable implementation is to only
> cache the final PA+perms result of Stage 1+2 rather than caching the two
> stages separately (which is also allowed), so it's possible that the h/w
> wouldn't have the IPA in its hand.
Hm, right that would make sense. This wasn't exactly clear on the x86
either, what does the TLB hold.. separate stage1/stage2 entries, or a
single entry for stage1+2? On the other hand, x86 always gets us the
IPA no matter what so my bet is on separate entries there.
>
>> I looked at the KVM code and they seem to query HPFAR_EL2 if the
>> fault is during s1ptw, but otherwise they do exactly what we do here.
>
> Yes, reading their comment there lead me to the bit of the ARM ARM and I
> think there may be improvements we could make to the non-xenaccess case
> at least, I was confused about HPFAR_EL2 va FAR_EL2 earlier.
IMHO it's very minor performance tuning and for data access violations
the cache probably already has the same translation so I wouldn't be
surprised if the two would cost the same amount of cycles. For
prefetch it's useful because we don't have to rely on the cache being
valid. Furthermore, I yet to see a single event being delivered that
happened during s1ptw. For some reason my domain's normal operations
don't seem to trigger any, even if I create new processes etc..
>
>> So, IMHO doing a TLB flush here would prevent a primed DTLB becoming a
>> problem. Not sure if that helps if the ITLB is primed however as we
>> would have no way to replicate the translation that is cached..
>
> Ian.
Thanks,
Tamas
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 5/7] xen/arm: Instruction prefetch abort (X) mem_event handling
2015-03-26 11:24 ` Tamas K Lengyel
@ 2015-03-26 11:53 ` Ian Campbell
0 siblings, 0 replies; 71+ messages in thread
From: Ian Campbell @ 2015-03-26 11:53 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Stefano Stabellini, Julien Grall, Ian Jackson,
xen-devel@lists.xen.org, Stefano Stabellini, Tamas K Lengyel
On Thu, 2015-03-26 at 12:24 +0100, Tamas K Lengyel wrote:
> >> I looked at the KVM code and they seem to query HPFAR_EL2 if the
> >> fault is during s1ptw, but otherwise they do exactly what we do here.
> >
> > Yes, reading their comment there lead me to the bit of the ARM ARM and I
> > think there may be improvements we could make to the non-xenaccess case
> > at least, I was confused about HPFAR_EL2 va FAR_EL2 earlier.
>
> IMHO it's very minor performance tuning and for data access violations
> the cache probably already has the same translation so I wouldn't be
> surprised if the two would cost the same amount of cycles. For
> prefetch it's useful because we don't have to rely on the cache being
> valid.
I'm thinking along the lines of:
if (HPFAR_EL2 is valid)
ipa = read HPFAR_EL2
else
ipa = gva_to_ipa of FAR_EL2 (via info.gva)
Then handle ipa as usual, including potentially an ipa->pa/pte lookup
when needed, which is safe because we guarantee that we don't play TLB
tricks to try and game ourselves.
The bit I've not yet convinced myself of is whether "HPFAR_EL2 is valid"
is true for all the potentially problematic cases. TFM says (G4.13.5):
For any Access flag fault or Translation fault, and also for any
Permission fault on the stage 2 translation of a memory access
made as part of a translation table walk for a stage 1
translation, the
HPFAR holds the IPA that caused the fault. Otherwise, the HPFAR
is UNKNOWN .
So that excludes Address Size and Access flag, but I think we don't care
about those, or at least not in a way which would require us to do a
lookup.
Table G4-33 also looks very informative too BTW.
Anyway, that all sounds pretty promising, we'd need to do a bit more
deciding of the FSC than we do today but that's not too hard.
> Furthermore, I yet to see a single event being delivered that
> happened during s1ptw. For some reason my domain's normal operations
> don't seem to trigger any, even if I create new processes etc..
You would need to make a stage 1 page table entry be non-present or at
least not readable to hit that, I suppose you have tried doing so via
xenaccess e.g. setting all of guest to some suitable type? Very strange
not to then see a s1ptw fault.
The other way would be xenpaging, which we don't do on ARM.
Ian.
^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH V13 6/7] xen/arm: Enable mem_access on ARM
2015-03-06 21:24 [PATCH V13 0/7] Mem_access for ARM Tamas K Lengyel
` (4 preceding siblings ...)
2015-03-06 21:24 ` [PATCH V13 5/7] xen/arm: Instruction prefetch abort (X) mem_event handling Tamas K Lengyel
@ 2015-03-06 21:24 ` Tamas K Lengyel
2015-03-12 13:36 ` Ian Campbell
2015-03-12 15:19 ` Julien Grall
2015-03-06 21:24 ` [PATCH V13 7/7] tools/libxc: Allocate magic page for mem access " Tamas K Lengyel
2015-03-12 11:30 ` [PATCH V13 0/7] Mem_access for ARM Ian Campbell
7 siblings, 2 replies; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-06 21:24 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson,
julien.grall, tim, stefano.stabellini, jbeulich, keir,
Tamas K Lengyel
Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
---
config/arm32.mk | 1 +
config/arm64.mk | 1 +
2 files changed, 2 insertions(+)
diff --git a/config/arm32.mk b/config/arm32.mk
index 268ca9c..cd97e42 100644
--- a/config/arm32.mk
+++ b/config/arm32.mk
@@ -14,6 +14,7 @@ HAS_EXYNOS4210 := y
HAS_OMAP := y
HAS_SCIF := y
HAS_NS16550 := y
+HAS_MEM_ACCESS := y
# Use only if calling $(LD) directly.
LDFLAGS_DIRECT += -EL
diff --git a/config/arm64.mk b/config/arm64.mk
index 6eafda2..d2fd242 100644
--- a/config/arm64.mk
+++ b/config/arm64.mk
@@ -8,6 +8,7 @@ CFLAGS += #-marm -march= -mcpu= etc
HAS_PL011 := y
HAS_NS16550 := y
+HAS_MEM_ACCESS := y
# Use only if calling $(LD) directly.
LDFLAGS_DIRECT += -EL
--
2.1.4
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [PATCH V13 6/7] xen/arm: Enable mem_access on ARM
2015-03-06 21:24 ` [PATCH V13 6/7] xen/arm: Enable mem_access on ARM Tamas K Lengyel
@ 2015-03-12 13:36 ` Ian Campbell
2015-03-12 15:19 ` Julien Grall
1 sibling, 0 replies; 71+ messages in thread
From: Ian Campbell @ 2015-03-12 13:36 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, stefano.stabellini, ian.jackson, julien.grall, tim,
xen-devel, stefano.stabellini, jbeulich, keir
On Fri, 2015-03-06 at 22:24 +0100, Tamas K Lengyel wrote:
> Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> config/arm32.mk | 1 +
> config/arm64.mk | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/config/arm32.mk b/config/arm32.mk
> index 268ca9c..cd97e42 100644
> --- a/config/arm32.mk
> +++ b/config/arm32.mk
> @@ -14,6 +14,7 @@ HAS_EXYNOS4210 := y
> HAS_OMAP := y
> HAS_SCIF := y
> HAS_NS16550 := y
> +HAS_MEM_ACCESS := y
>
> # Use only if calling $(LD) directly.
> LDFLAGS_DIRECT += -EL
> diff --git a/config/arm64.mk b/config/arm64.mk
> index 6eafda2..d2fd242 100644
> --- a/config/arm64.mk
> +++ b/config/arm64.mk
> @@ -8,6 +8,7 @@ CFLAGS += #-marm -march= -mcpu= etc
>
> HAS_PL011 := y
> HAS_NS16550 := y
> +HAS_MEM_ACCESS := y
>
> # Use only if calling $(LD) directly.
> LDFLAGS_DIRECT += -EL
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 6/7] xen/arm: Enable mem_access on ARM
2015-03-06 21:24 ` [PATCH V13 6/7] xen/arm: Enable mem_access on ARM Tamas K Lengyel
2015-03-12 13:36 ` Ian Campbell
@ 2015-03-12 15:19 ` Julien Grall
2015-03-12 15:43 ` Tamas K Lengyel
1 sibling, 1 reply; 71+ messages in thread
From: Julien Grall @ 2015-03-12 15:19 UTC (permalink / raw)
To: Tamas K Lengyel, xen-devel
Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson, tim,
stefano.stabellini, jbeulich, keir
Hi Tamas,
On 06/03/15 21:24, Tamas K Lengyel wrote:
> Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
It's strange to enable MEM_ACCESS before having the toolstack part
support memaccess (even though it's tiny).
Anyway, I guess it's fine:
Reviewed-by: Julien Grall <julien.grall@linaro.org>
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 6/7] xen/arm: Enable mem_access on ARM
2015-03-12 15:19 ` Julien Grall
@ 2015-03-12 15:43 ` Tamas K Lengyel
0 siblings, 0 replies; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-12 15:43 UTC (permalink / raw)
To: Julien Grall
Cc: wei.liu2, Ian Campbell, Stefano Stabellini, Tim Deegan,
Ian Jackson, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 554 bytes --]
On Thu, Mar 12, 2015 at 4:19 PM, Julien Grall <julien.grall@linaro.org>
wrote:
> Hi Tamas,
>
> On 06/03/15 21:24, Tamas K Lengyel wrote:
> > Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
>
> It's strange to enable MEM_ACCESS before having the toolstack part
> support memaccess (even though it's tiny).
Made the series a bit more easier to follow, first half hypervisor side,
second half toolstack.
>
> Anyway, I guess it's fine:
>
> Reviewed-by: Julien Grall <julien.grall@linaro.org>
>
> Regards,
>
> --
> Julien Grall
>
Thanks,
Tamas
[-- Attachment #1.2: Type: text/html, Size: 1296 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH V13 7/7] tools/libxc: Allocate magic page for mem access on ARM
2015-03-06 21:24 [PATCH V13 0/7] Mem_access for ARM Tamas K Lengyel
` (5 preceding siblings ...)
2015-03-06 21:24 ` [PATCH V13 6/7] xen/arm: Enable mem_access on ARM Tamas K Lengyel
@ 2015-03-06 21:24 ` Tamas K Lengyel
2015-03-12 13:36 ` Ian Campbell
2015-03-12 11:30 ` [PATCH V13 0/7] Mem_access for ARM Ian Campbell
7 siblings, 1 reply; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-06 21:24 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, ian.campbell, stefano.stabellini, ian.jackson,
julien.grall, tim, stefano.stabellini, jbeulich, keir,
Tamas K Lengyel
Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
tools/libxc/xc_dom_arm.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index c7feca7..aaf835c 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -26,9 +26,10 @@
#include "xg_private.h"
#include "xc_dom.h"
-#define NR_MAGIC_PAGES 2
+#define NR_MAGIC_PAGES 3
#define CONSOLE_PFN_OFFSET 0
#define XENSTORE_PFN_OFFSET 1
+#define MEMACCESS_PFN_OFFSET 2
#define LPAE_SHIFT 9
@@ -87,10 +88,13 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
+ xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET);
xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
dom->console_pfn);
xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
dom->xenstore_pfn);
+ xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_ACCESS_RING_PFN,
+ base + MEMACCESS_PFN_OFFSET);
/* allocated by toolstack */
xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_EVTCHN,
dom->console_evtchn);
--
2.1.4
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [PATCH V13 7/7] tools/libxc: Allocate magic page for mem access on ARM
2015-03-06 21:24 ` [PATCH V13 7/7] tools/libxc: Allocate magic page for mem access " Tamas K Lengyel
@ 2015-03-12 13:36 ` Ian Campbell
0 siblings, 0 replies; 71+ messages in thread
From: Ian Campbell @ 2015-03-12 13:36 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, stefano.stabellini, ian.jackson, julien.grall, tim,
xen-devel, stefano.stabellini, jbeulich, keir
On Fri, 2015-03-06 at 22:24 +0100, Tamas K Lengyel wrote:
> Signed-off-by: Tamas K Lengyel <tklengyel@sec.in.tum.de>
> Reviewed-by: Julien Grall <julien.grall@linaro.org>
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> tools/libxc/xc_dom_arm.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index c7feca7..aaf835c 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -26,9 +26,10 @@
> #include "xg_private.h"
> #include "xc_dom.h"
>
> -#define NR_MAGIC_PAGES 2
> +#define NR_MAGIC_PAGES 3
> #define CONSOLE_PFN_OFFSET 0
> #define XENSTORE_PFN_OFFSET 1
> +#define MEMACCESS_PFN_OFFSET 2
>
> #define LPAE_SHIFT 9
>
> @@ -87,10 +88,13 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
>
> xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
> xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
> + xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET);
> xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
> dom->console_pfn);
> xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
> dom->xenstore_pfn);
> + xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_ACCESS_RING_PFN,
> + base + MEMACCESS_PFN_OFFSET);
> /* allocated by toolstack */
> xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_EVTCHN,
> dom->console_evtchn);
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 0/7] Mem_access for ARM
2015-03-06 21:24 [PATCH V13 0/7] Mem_access for ARM Tamas K Lengyel
` (6 preceding siblings ...)
2015-03-06 21:24 ` [PATCH V13 7/7] tools/libxc: Allocate magic page for mem access " Tamas K Lengyel
@ 2015-03-12 11:30 ` Ian Campbell
2015-03-12 12:24 ` Tamas K Lengyel
7 siblings, 1 reply; 71+ messages in thread
From: Ian Campbell @ 2015-03-12 11:30 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, stefano.stabellini, ian.jackson, julien.grall, tim,
xen-devel, stefano.stabellini, jbeulich, keir
On Fri, 2015-03-06 at 22:24 +0100, Tamas K Lengyel wrote:
> The ARM virtualization extension provides 2-stage paging, a similar mechanisms
> to Intel's EPT, which can be used to trace the memory accesses performed by
> the guest systems. This series sets up the necessary infrastructure in the
> ARM code to deliver the event on R/W/X traps. Finally, we turn on the
> compilation of mem_access and mem_event on ARM and perform the necessary
> changes to the tools side.
>
> This version of the series is based on master and each patch in the series has
> been compile tested on both ARM and x86.
>
> The following benchmarks have been completed on an Arndale board before and
> after applying this entire series in dom0, which had 2 vCPUs available
> pinned. No regressions are evident based on these benchmarks.
>
> | f0ffd60 | AFTER
> --------------------------------------------
> hackbench | 1.604 | 1.618
> hackbench -pT | 1.170 | 1.126
> hardinfo CPU Blowfish | 43.252 | 43.323
> hardinfo CPU CryptoHash | 40.935 | 40.893
> hardinfo CPU Fibonacci | 10.358 | 10.360
> hardinfo CPU N-Queens | 25.773 | 25.894
> hardinfo FPU FFT | 20.332 | 20.325
> hardinfo FPU Raytracing | 73.017 | 73.104
Thanks for these, they are very reassuring.
More for curiosities sake than anything else: Do you have any figures
for the overhead of enabling memaccess?
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH V13 0/7] Mem_access for ARM
2015-03-12 11:30 ` [PATCH V13 0/7] Mem_access for ARM Ian Campbell
@ 2015-03-12 12:24 ` Tamas K Lengyel
2015-03-12 13:53 ` Ian Campbell
0 siblings, 1 reply; 71+ messages in thread
From: Tamas K Lengyel @ 2015-03-12 12:24 UTC (permalink / raw)
To: Ian Campbell
Cc: wei.liu2, Stefano Stabellini, Ian Jackson, Julien Grall,
Tim Deegan, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 1700 bytes --]
On Thu, Mar 12, 2015 at 12:30 PM, Ian Campbell <ian.campbell@citrix.com>
wrote:
> On Fri, 2015-03-06 at 22:24 +0100, Tamas K Lengyel wrote:
> > The ARM virtualization extension provides 2-stage paging, a similar
> mechanisms
> > to Intel's EPT, which can be used to trace the memory accesses performed
> by
> > the guest systems. This series sets up the necessary infrastructure in
> the
> > ARM code to deliver the event on R/W/X traps. Finally, we turn on the
> > compilation of mem_access and mem_event on ARM and perform the necessary
> > changes to the tools side.
> >
> > This version of the series is based on master and each patch in the
> series has
> > been compile tested on both ARM and x86.
> >
> > The following benchmarks have been completed on an Arndale board before
> and
> > after applying this entire series in dom0, which had 2 vCPUs available
> > pinned. No regressions are evident based on these benchmarks.
> >
> > | f0ffd60 | AFTER
> > --------------------------------------------
> > hackbench | 1.604 | 1.618
> > hackbench -pT | 1.170 | 1.126
> > hardinfo CPU Blowfish | 43.252 | 43.323
> > hardinfo CPU CryptoHash | 40.935 | 40.893
> > hardinfo CPU Fibonacci | 10.358 | 10.360
> > hardinfo CPU N-Queens | 25.773 | 25.894
> > hardinfo FPU FFT | 20.332 | 20.325
> > hardinfo FPU Raytracing | 73.017 | 73.104
>
> Thanks for these, they are very reassuring.
>
> More for curiosities sake than anything else: Do you have any figures
> for the overhead of enabling memaccess?
>
I can gather some figures if needed but it is very use-case specific
depending on which pages are being trapped and how.
Tamas
[-- Attachment #1.2: Type: text/html, Size: 2271 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH V13 0/7] Mem_access for ARM
2015-03-12 12:24 ` Tamas K Lengyel
@ 2015-03-12 13:53 ` Ian Campbell
0 siblings, 0 replies; 71+ messages in thread
From: Ian Campbell @ 2015-03-12 13:53 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: wei.liu2, Stefano Stabellini, Ian Jackson, Julien Grall,
Tim Deegan, xen-devel, stefano.stabellini, Jan Beulich,
Keir Fraser
On Thu, 2015-03-12 at 13:24 +0100, Tamas K Lengyel wrote:
>
>
> On Thu, Mar 12, 2015 at 12:30 PM, Ian Campbell
> More for curiosities sake than anything else: Do you have any figures
> for the overhead of enabling memaccess?
> I can gather some figures if needed but it is very use-case specific
> depending on which pages are being trapped and how.
That makes sense, was just curious so no need to go do extra work.
Ian.
^ permalink raw reply [flat|nested] 71+ messages in thread