* [RFC PATCH 1/2] mem_access: Fix npfec.kind propagation @ 2018-09-26 16:47 George Dunlap 2018-09-26 16:47 ` [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT George Dunlap ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: George Dunlap @ 2018-09-26 16:47 UTC (permalink / raw) To: xen-devel Cc: Razvan Cojocaru, Andrew Cooper, Tim Deegan, George Dunlap, Jan Beulich, Tamas K Lengyel, Alexandru Isaila The name of the "with_gla" flag is confusing; it has nothing to do with the existence or lack thereof of a faulting GLA, but rather where the fault originated. The npfec.kind value is always valid, and should thus be propagated, regardless of whether gla_valid is set or not. In particular, gla_valid will never be set on AMD systems; but npfec.kind will still be valid and should still be propagated. Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> Signed-off-by: George Dunlap <george.dunlap@citrix.com> --- CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Jan Beulich <jbeulich@suse.com> CC: Tim Deegan <tim@xen.org> CC: Tamas K Lengyel <tamas.lengyel@zentific.com> CC: Razvan Cojocaru <rcojocaru@bitdefender.com> --- xen/arch/x86/mm/mem_access.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c index d9e64fcbb9..699a315076 100644 --- a/xen/arch/x86/mm/mem_access.c +++ b/xen/arch/x86/mm/mem_access.c @@ -232,12 +232,12 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla, { req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID; req->u.mem_access.gla = gla; - - if ( npfec.kind == npfec_kind_with_gla ) - req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA; - else if ( npfec.kind == npfec_kind_in_gpt ) - req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT; } + + if ( npfec.kind == npfec_kind_with_gla ) + req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA; + else if ( npfec.kind == npfec_kind_in_gpt ) + req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT; req->u.mem_access.flags |= npfec.read_access ? MEM_ACCESS_R : 0; req->u.mem_access.flags |= npfec.write_access ? MEM_ACCESS_W : 0; req->u.mem_access.flags |= npfec.insn_fetch ? MEM_ACCESS_X : 0; -- 2.19.0 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT 2018-09-26 16:47 [RFC PATCH 1/2] mem_access: Fix npfec.kind propagation George Dunlap @ 2018-09-26 16:47 ` George Dunlap 2018-09-26 17:22 ` Andrew Cooper ` (3 more replies) 2018-09-26 16:51 ` [RFC PATCH 1/2] mem_access: Fix npfec.kind propagation Tamas Lengyel 2018-09-26 17:00 ` Andrew Cooper 2 siblings, 4 replies; 16+ messages in thread From: George Dunlap @ 2018-09-26 16:47 UTC (permalink / raw) To: xen-devel Cc: Razvan Cojocaru, Andrew Cooper, Tim Deegan, George Dunlap, Paul Durrant, Jan Beulich, Tamas K Lengyel, Isaila Alexandru From: Isaila Alexandru <aisaila@bitdefender.com> This patch adds access control for NPT mode. There aren’t enough extra bits to store the access rights in the NPT p2m table, so we add a radix tree to store extra information. For efficiency: - Only allocate this radix tree when we first store "non-default" extra information - Remove entires which match the default extra information rather than continuing to store them - For superpages, only store an entry for the first gfn in the superpage. Use the order of the p2m entry being read to determine the proper place to look in the radix table. Modify p2m_type_to_flags() to accept and interpret an access value, parallel to the ept code. Add a set_default_access() method to the p2m-pt and p2m-ept versions of the p2m rather than setting it directly, to deal with different default permitted access values. Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> Signed-off-by: George Dunlap <george.dunlap@citrix.com> --- NB, this is compile-tested only. cc'ing Paul because this is functionality he may want at some point in the future. I'm not sure why we only allow 'int' to be stored in the radix tree, but that throws away 30-some bits we could otherwise use. We might consider revising this if we run out of bits here. CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Jan Beulich <jbeulich@suse.com> CC: Tim Deegan <tim@xen.org> CC: Tamas K Lengyel <tamas.lengyel@zentific.com> CC: Paul Durrant <paul.durrant@citrix.com> CC: Razvan Cojocaru <rcojocaru@bitdefender.com> --- xen/arch/x86/mm/mem_access.c | 5 +- xen/arch/x86/mm/p2m-ept.c | 9 ++ xen/arch/x86/mm/p2m-pt.c | 173 ++++++++++++++++++++++++++++--- xen/arch/x86/mm/p2m.c | 6 ++ xen/arch/x86/monitor.c | 1 + xen/include/asm-x86/mem_access.h | 2 +- xen/include/asm-x86/p2m.h | 18 ++++ 7 files changed, 192 insertions(+), 22 deletions(-) diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c index 699a315076..0f96c43f88 100644 --- a/xen/arch/x86/mm/mem_access.c +++ b/xen/arch/x86/mm/mem_access.c @@ -389,10 +389,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, /* If request to set default access. */ if ( gfn_eq(gfn, INVALID_GFN) ) - { - p2m->default_access = a; - return 0; - } + return p2m->set_default_access(p2m, a); p2m_lock(p2m); if ( ap2m ) diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index 1ff4f14ae4..37bdbcf186 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -667,6 +667,14 @@ bool_t ept_handle_misconfig(uint64_t gpa) return spurious ? (rc >= 0) : (rc > 0); } +int ept_set_default_access(struct p2m_domain *p2m, p2m_access_t p2ma) +{ + /* All access is permitted */ + p2m->default_access = p2ma; + + return 0; +} + /* * ept_set_entry() computes 'need_modify_vtd_table' for itself, * by observing whether any gfn->mfn translations are modified. @@ -1255,6 +1263,7 @@ int ept_p2m_init(struct p2m_domain *p2m) p2m->change_entry_type_global = ept_change_entry_type_global; p2m->change_entry_type_range = ept_change_entry_type_range; p2m->memory_type_changed = ept_memory_type_changed; + p2m->set_default_access = ept_set_default_access; p2m->audit_p2m = NULL; p2m->tlb_flush = ept_tlb_flush; diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c index 40bfc76a6f..cd8b8c9187 100644 --- a/xen/arch/x86/mm/p2m-pt.c +++ b/xen/arch/x86/mm/p2m-pt.c @@ -68,7 +68,8 @@ static unsigned long p2m_type_to_flags(const struct p2m_domain *p2m, p2m_type_t t, mfn_t mfn, - unsigned int level) + unsigned int level, + p2m_access_t access) { unsigned long flags; /* @@ -87,23 +88,28 @@ static unsigned long p2m_type_to_flags(const struct p2m_domain *p2m, case p2m_ram_paged: case p2m_ram_paging_in: default: - return flags | _PAGE_NX_BIT; + flags |= _PAGE_NX_BIT; + break; case p2m_grant_map_ro: - return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT; + flags |= P2M_BASE_FLAGS | _PAGE_NX_BIT; + break; case p2m_ioreq_server: flags |= P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT; if ( p2m->ioreq.flags & XEN_DMOP_IOREQ_MEM_ACCESS_WRITE ) - return flags & ~_PAGE_RW; - return flags; + flags &= ~_PAGE_RW; + break; case p2m_ram_ro: case p2m_ram_logdirty: case p2m_ram_shared: - return flags | P2M_BASE_FLAGS; + flags |= P2M_BASE_FLAGS; + break; case p2m_ram_rw: - return flags | P2M_BASE_FLAGS | _PAGE_RW; + flags |= P2M_BASE_FLAGS | _PAGE_RW; + break; case p2m_grant_map_rw: case p2m_map_foreign: - return flags | P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT; + flags |= P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT; + break; case p2m_mmio_direct: if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn)) ) flags |= _PAGE_RW; @@ -112,8 +118,32 @@ static unsigned long p2m_type_to_flags(const struct p2m_domain *p2m, flags |= _PAGE_PWT; ASSERT(!level); } - return flags | P2M_BASE_FLAGS | _PAGE_PCD; + flags |= P2M_BASE_FLAGS | _PAGE_PCD; + break; } + + switch ( access ) + { + case p2m_access_r: + flags |= _PAGE_NX_BIT; + flags &= ~_PAGE_RW; + break; + case p2m_access_rw: + flags |= _PAGE_NX_BIT; + break; + case p2m_access_rx: + case p2m_access_rx2rw: + flags &= ~(_PAGE_NX_BIT | _PAGE_RW); + break; + case p2m_access_x: + flags &= ~_PAGE_RW; + break; + case p2m_access_rwx: + default: + break; + } + + return flags; } @@ -174,6 +204,48 @@ static void p2m_add_iommu_flags(l1_pgentry_t *p2m_entry, l1e_add_flags(*p2m_entry, iommu_nlevel_to_flags(nlevel, flags)); } +const p2m_extra_t pt_extra_default = { .access = p2m_access_rwx }; + +static p2m_extra_t p2m_pt_get_extra(struct p2m_domain *p2m, unsigned long gfn, + unsigned level) +{ + void *ptr; + unsigned long mask = ~0UL << (level * PAGETABLE_ORDER); + + if ( !p2m->extra ) + return pt_extra_default; + + ptr = radix_tree_lookup(p2m->extra, gfn & mask); + if ( !ptr ) + return pt_extra_default; + else + return (p2m_extra_t)radix_tree_ptr_to_int(ptr); +} + +static void p2m_pt_set_extra(struct p2m_domain *p2m, unsigned long gfn, + p2m_extra_t extra) +{ + int rc; + + if ( extra.extra == pt_extra_default.extra ) + { + if ( p2m->extra ) + radix_tree_delete(p2m->extra, gfn); + return; + } + + ASSERT(p2m->extra); + + rc = radix_tree_insert(p2m->extra, gfn, + radix_tree_int_to_ptr(extra.extra)); + if ( rc == -EEXIST ) + /* If a setting already exists, change it to the new one. */ + radix_tree_replace_slot( + radix_tree_lookup_slot( + p2m->extra, gfn), + radix_tree_int_to_ptr(extra.extra)); +} + /* Returns: 0 for success, -errno for failure */ static int p2m_next_level(struct p2m_domain *p2m, void **table, @@ -210,6 +282,7 @@ p2m_next_level(struct p2m_domain *p2m, void **table, mfn_t mfn; l1_pgentry_t *l1_entry; unsigned int i; + p2m_extra_t extra; switch ( level ) { @@ -247,8 +320,14 @@ p2m_next_level(struct p2m_domain *p2m, void **table, for ( i = 0; i < (1u << PAGETABLE_ORDER); i++ ) { - new_entry = l1e_from_pfn(pfn | (i << ((level - 1) * PAGETABLE_ORDER)), - flags); + unsigned long frame_offset = (i << ((level - 1) * PAGETABLE_ORDER)); + new_entry = l1e_from_pfn(pfn | frame_offset, flags); + + /* Shattering a p2m mapping requires duplicating extra info as well */ + if ( i == 0 ) + extra = p2m_pt_get_extra(p2m, gfn, level); + else + p2m_pt_set_extra(p2m, gfn + frame_offset, extra); p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, level); } @@ -420,8 +499,10 @@ static int do_recalc(struct p2m_domain *p2m, unsigned long gfn) if ( nt != ot ) { unsigned long mfn = l1e_get_pfn(e); + p2m_extra_t extra = p2m_pt_get_extra(p2m, gfn, level); unsigned long flags = p2m_type_to_flags(p2m, nt, - _mfn(mfn), level); + _mfn(mfn), level, + extra.access); if ( level ) { @@ -471,6 +552,32 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa) return rc; } +/* Returns: 0 for success, -errno for failure */ +static int p2m_pt_check_access(p2m_access_t p2ma) +{ + switch ( p2ma ) + { + case p2m_access_n: + case p2m_access_w: + case p2m_access_wx: + case p2m_access_n2rwx: + return -EINVAL; + default: + break; + } + return 0; +} + +static int p2m_pt_set_default_access(struct p2m_domain *p2m, + p2m_access_t p2ma) { + int rc = p2m_pt_check_access(p2ma); + + if ( !rc ) + p2m->default_access = p2ma; + + return rc; +} + /* Returns: 0 for success, -errno for failure */ static int p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn, @@ -500,9 +607,13 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn, */ unsigned int flags, iommu_old_flags = 0; unsigned long old_mfn = mfn_x(INVALID_MFN); + p2m_extra_t extra = pt_extra_default; ASSERT(sve != 0); + if ( (rc = p2m_pt_check_access(p2ma)) ) + return rc; + if ( tb_init_done ) { struct { @@ -532,6 +643,24 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn, if ( rc < 0 ) return rc; + if ( p2ma != extra.access ) + extra.access = p2ma; + + /* + * If we need 'extra' bits and we haven't allocated space for it + * yet, do so + */ + if ( (extra.extra != pt_extra_default.extra) && !p2m->extra ) + { + p2m->extra = xmalloc(struct radix_tree_root); + + if( !p2m->extra ) + { + return -ENOMEM; + } + radix_tree_init(p2m->extra); + } + table = map_domain_page(pagetable_get_mfn(p2m_get_pagetable(p2m))); rc = p2m_next_level(p2m, &table, &gfn_remainder, gfn, L4_PAGETABLE_SHIFT - PAGE_SHIFT, @@ -569,7 +698,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn, ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct); l3e_content = mfn_valid(mfn) || p2m_allows_invalid_mfn(p2mt) ? p2m_l3e_from_pfn(mfn_x(mfn), - p2m_type_to_flags(p2m, p2mt, mfn, 2)) + p2m_type_to_flags(p2m, p2mt, mfn, 2, p2ma)) : l3e_empty(); entry_content.l1 = l3e_content.l3; @@ -608,7 +737,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn, if ( mfn_valid(mfn) || p2m_allows_invalid_mfn(p2mt) ) entry_content = p2m_l1e_from_pfn(mfn_x(mfn), - p2m_type_to_flags(p2m, p2mt, mfn, 0)); + p2m_type_to_flags(p2m, p2mt, mfn, 0, p2ma)); else entry_content = l1e_empty(); @@ -661,7 +790,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn, ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct); l2e_content = mfn_valid(mfn) || p2m_allows_invalid_mfn(p2mt) ? p2m_l2e_from_pfn(mfn_x(mfn), - p2m_type_to_flags(p2m, p2mt, mfn, 1)) + p2m_type_to_flags(p2m, p2mt, mfn, 1, p2ma)) : l2e_empty(); entry_content.l1 = l2e_content.l2; @@ -672,6 +801,13 @@ p2m_pt_set_entry(struct p2m_domain *p2m, gfn_t gfn_, mfn_t mfn, /* NB: paging_write_p2m_entry() handles tlb flushes properly */ } + /* + * For storing of 'extra' information, only set the first gfn in + * any range; use the p2m table itself to record the order covered + * by that entry. + */ + p2m_pt_set_extra(p2m, gfn, extra); + /* Track the highest gfn for which we have ever had a valid mapping */ if ( p2mt != p2m_invalid && (gfn + (1UL << page_order) - 1 > p2m->max_mapped_pfn) ) @@ -749,8 +885,7 @@ p2m_pt_get_entry(struct p2m_domain *p2m, gfn_t gfn_, * XXX Once we start explicitly registering MMIO regions in the p2m * XXX we will return p2m_invalid for unmapped gfns */ *t = p2m_mmio_dm; - /* Not implemented except with EPT */ - *a = p2m_access_rwx; + *a = p2m_access_n; if ( gfn > p2m->max_mapped_pfn ) { @@ -813,6 +948,7 @@ pod_retry_l3: l1_table_offset(addr)); *t = p2m_recalc_type(recalc || _needs_recalc(flags), p2m_flags_to_type(flags), p2m, gfn); + *a = p2m_pt_get_extra(p2m, gfn, 2).access; unmap_domain_page(l3e); ASSERT(mfn_valid(mfn) || !p2m_is_ram(*t)); @@ -852,6 +988,7 @@ pod_retry_l2: mfn = _mfn(l2e_get_pfn(*l2e) + l1_table_offset(addr)); *t = p2m_recalc_type(recalc || _needs_recalc(flags), p2m_flags_to_type(flags), p2m, gfn); + *a = p2m_pt_get_extra(p2m, gfn, 1).access; unmap_domain_page(l2e); ASSERT(mfn_valid(mfn) || !p2m_is_ram(*t)); @@ -888,6 +1025,7 @@ pod_retry_l1: } mfn = l1e_get_mfn(*l1e); *t = p2m_recalc_type(recalc || _needs_recalc(flags), l1t, p2m, gfn); + *a = p2m_pt_get_extra(p2m, gfn, 0).access; unmap_domain_page(l1e); ASSERT(mfn_valid(mfn) || !p2m_is_ram(*t) || p2m_is_paging(*t)); @@ -1129,6 +1267,7 @@ void p2m_pt_init(struct p2m_domain *p2m) p2m->change_entry_type_global = p2m_pt_change_entry_type_global; p2m->change_entry_type_range = p2m_pt_change_entry_type_range; p2m->write_p2m_entry = paging_write_p2m_entry; + p2m->set_default_access = p2m_pt_set_default_access; #if P2M_AUDIT p2m->audit_p2m = p2m_pt_audit_p2m; #else diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index ed2e8daf58..706a8ec8bb 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -682,6 +682,12 @@ void p2m_teardown(struct p2m_domain *p2m) d = p2m->domain; + if ( p2m->extra ) + { + radix_tree_destroy(p2m->extra, NULL); + xfree(p2m->extra); + } + p2m_lock(p2m); ASSERT(atomic_read(&d->shr_pages) == 0); p2m->phys_table = pagetable_null(); diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index 3c42e21906..2e6b1e75e4 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -20,6 +20,7 @@ */ #include <asm/monitor.h> +#include <asm/p2m.h> #include <public/vm_event.h> int arch_monitor_init_domain(struct domain *d) diff --git a/xen/include/asm-x86/mem_access.h b/xen/include/asm-x86/mem_access.h index 4043c9fb4d..34f2c07088 100644 --- a/xen/include/asm-x86/mem_access.h +++ b/xen/include/asm-x86/mem_access.h @@ -46,7 +46,7 @@ bool p2m_mem_access_emulate_check(struct vcpu *v, /* Sanity check for mem_access hardware support */ static inline bool p2m_mem_access_sanity_check(struct domain *d) { - return is_hvm_domain(d) && cpu_has_vmx && hap_enabled(d); + return is_hvm_domain(d) && hap_enabled(d); } #endif /*__ASM_X86_MEM_ACCESS_H__ */ diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h index be3b6fcaf0..a5e1193dcc 100644 --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -185,6 +185,16 @@ typedef enum { p2m_alternate, } p2m_class_t; +typedef union { + struct { + u32 access : 4, /* p2m_access_t */ + _available : 25, + _reserved : 3; /* Upper two bits reserved */ + }; + int extra; +} p2m_extra_t; + + /* Per-p2m-table state */ struct p2m_domain { /* Lock that protects updates to the p2m */ @@ -270,6 +280,8 @@ struct p2m_domain { unsigned long gfn, l1_pgentry_t *p, l1_pgentry_t new, unsigned int level); long (*audit_p2m)(struct p2m_domain *p2m); + int (*set_default_access)(struct p2m_domain *p2m, + p2m_access_t p2ma); /* * P2M updates may require TLBs to be flushed (invalidated). @@ -292,6 +304,12 @@ struct p2m_domain { * retyped get this access type. See definition of p2m_access_t. */ p2m_access_t default_access; + /* + * Radix tree to store extra information which doesn't fit in the + * p2m's PTE + */ + struct radix_tree_root *extra; + /* If true, and an access fault comes in and there is no vm_event listener, * pause domain. Otherwise, remove access restrictions. */ bool_t access_required; -- 2.19.0 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT 2018-09-26 16:47 ` [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT George Dunlap @ 2018-09-26 17:22 ` Andrew Cooper 2018-09-27 10:37 ` George Dunlap 2018-09-27 9:38 ` Isaila Alexandru ` (2 subsequent siblings) 3 siblings, 1 reply; 16+ messages in thread From: Andrew Cooper @ 2018-09-26 17:22 UTC (permalink / raw) To: George Dunlap, xen-devel Cc: Razvan Cojocaru, Tamas K Lengyel, Tim Deegan, Paul Durrant, Jan Beulich, Isaila Alexandru On 26/09/18 17:47, George Dunlap wrote: > From: Isaila Alexandru <aisaila@bitdefender.com> > > This patch adds access control for NPT mode. > > There aren’t enough extra bits to store the access rights in the NPT p2m > table, so we add a radix tree to store extra information. I'm sorry to re-open this argument, but why? ISTR there being some argument based on pagetable sharing with the IOMMU, but that doesn't work at the moment and can't reasonably be made to work. For one, attempting to use pt sharing will break as soon as you try and DMA to a mapped grant. I'm disinclined to let a broken vestigial feature get in the way of real improvements. Beyond that, an NPT PTE has basically the same number of software available bits as an EPT PTE. Am I missing anything? > > For efficiency: > - Only allocate this radix tree when we first store "non-default" > extra information > > - Remove entires which match the default extra information rather > than continuing to store them > > - For superpages, only store an entry for the first gfn in the > superpage. Use the order of the p2m entry being read to determine > the proper place to look in the radix table. > > Modify p2m_type_to_flags() to accept and interpret an access value, > parallel to the ept code. > > Add a set_default_access() method to the p2m-pt and p2m-ept versions > of the p2m rather than setting it directly, to deal with different > default permitted access values. > > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > Signed-off-by: George Dunlap <george.dunlap@citrix.com> > --- > NB, this is compile-tested only. > > cc'ing Paul because this is functionality he may want at some point in > the future. > > I'm not sure why we only allow 'int' to be stored in the radix tree, > but that throws away 30-some bits we could otherwise use. We might > consider revising this if we run out of bits here. Probably because we have a old fork of the Linux radix tree functionality, rather than any specific reason to waste 30-some bits. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT 2018-09-26 17:22 ` Andrew Cooper @ 2018-09-27 10:37 ` George Dunlap 2018-09-27 10:53 ` Paul Durrant 2019-01-09 9:30 ` Alexandru Stefan ISAILA 0 siblings, 2 replies; 16+ messages in thread From: George Dunlap @ 2018-09-27 10:37 UTC (permalink / raw) To: Andrew Cooper, xen-devel Cc: Suravee Suthikulpanit, Razvan Cojocaru, Tamas K Lengyel, Tim Deegan, Paul Durrant, Jan Beulich, Isaila Alexandru, Boris Ostrovsky, Brian Woods On 09/26/2018 06:22 PM, Andrew Cooper wrote: > On 26/09/18 17:47, George Dunlap wrote: >> From: Isaila Alexandru <aisaila@bitdefender.com> >> >> This patch adds access control for NPT mode. >> >> There aren’t enough extra bits to store the access rights in the NPT p2m >> table, so we add a radix tree to store extra information. > > I'm sorry to re-open this argument, but why? > > ISTR there being some argument based on pagetable sharing with the > IOMMU, but that doesn't work at the moment and can't reasonably be made > to work. For one, attempting to use pt sharing will break as soon as > you try and DMA to a mapped grant. > > I'm disinclined to let a broken vestigial feature get in the way of real > improvements. > > Beyond that, an NPT PTE has basically the same number of software > available bits as an EPT PTE. > > Am I missing anything? Wow -- looks like IOMMU/p2m sharing has been disabled unconditionally since 2014. If nobody has complained since then, that seems like a good enough reason to me to rip it out. Suravee / Brian / Boris -- any opinions? The main reason to go with the 'extra bits' solution rather than the 'rip out iommu/p2m sharing' solution is because people have been prognosticating for years that we would be running out of bits and need more at some point in the future. I thought Paul, for instance, might have a use for the extra bits. But I'm happy to wait until such time as we need it and then fish this patch out of the mail archives. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT 2018-09-27 10:37 ` George Dunlap @ 2018-09-27 10:53 ` Paul Durrant 2019-01-09 9:30 ` Alexandru Stefan ISAILA 1 sibling, 0 replies; 16+ messages in thread From: Paul Durrant @ 2018-09-27 10:53 UTC (permalink / raw) To: George Dunlap, Andrew Cooper, xen-devel Cc: Jan Beulich, Razvan Cojocaru, Tamas K Lengyel, Tim (Xen.org), Suravee Suthikulpanit, Isaila Alexandru, Boris Ostrovsky, Brian Woods > -----Original Message----- > From: George Dunlap [mailto:george.dunlap@citrix.com] > Sent: 27 September 2018 11:38 > To: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen- > devel@lists.xenproject.org > Cc: Isaila Alexandru <aisaila@bitdefender.com>; Jan Beulich > <jbeulich@suse.com>; Tim (Xen.org) <tim@xen.org>; Tamas K Lengyel > <tamas.lengyel@zentific.com>; Paul Durrant <Paul.Durrant@citrix.com>; > Razvan Cojocaru <rcojocaru@bitdefender.com>; Suravee Suthikulpanit > <suravee.suthikulpanit@amd.com>; Brian Woods <brian.woods@amd.com>; Boris > Ostrovsky <boris.ostrovsky@oracle.com> > Subject: Re: [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT > > On 09/26/2018 06:22 PM, Andrew Cooper wrote: > > On 26/09/18 17:47, George Dunlap wrote: > >> From: Isaila Alexandru <aisaila@bitdefender.com> > >> > >> This patch adds access control for NPT mode. > >> > >> There aren’t enough extra bits to store the access rights in the NPT > p2m > >> table, so we add a radix tree to store extra information. > > > > I'm sorry to re-open this argument, but why? > > > > ISTR there being some argument based on pagetable sharing with the > > IOMMU, but that doesn't work at the moment and can't reasonably be made > > to work. For one, attempting to use pt sharing will break as soon as > > you try and DMA to a mapped grant. > > > > I'm disinclined to let a broken vestigial feature get in the way of real > > improvements. > > > > Beyond that, an NPT PTE has basically the same number of software > > available bits as an EPT PTE. > > > > Am I missing anything? > > Wow -- looks like IOMMU/p2m sharing has been disabled unconditionally > since 2014. If nobody has complained since then, that seems like a good > enough reason to me to rip it out. > > Suravee / Brian / Boris -- any opinions? > > The main reason to go with the 'extra bits' solution rather than the > 'rip out iommu/p2m sharing' solution is because people have been > prognosticating for years that we would be running out of bits and need > more at some point in the future. I thought Paul, for instance, might > have a use for the extra bits. But I'm happy to wait until such time as > we need it and then fish this patch out of the mail archives. > The main angle I had was to have a more generic page-to-type mapping such that it would be suitable to allow steering of accesses to certain pages to distinct IOREQ servers. Paul > -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT 2018-09-27 10:37 ` George Dunlap 2018-09-27 10:53 ` Paul Durrant @ 2019-01-09 9:30 ` Alexandru Stefan ISAILA 1 sibling, 0 replies; 16+ messages in thread From: Alexandru Stefan ISAILA @ 2019-01-09 9:30 UTC (permalink / raw) To: George Dunlap, Andrew Cooper, xen-devel, Suravee Suthikulpanit, Brian Woods, Boris Ostrovsky Cc: Tamas K Lengyel, Paul Durrant, Tim Deegan, Razvan Cojocaru, Jan Beulich Ping Suravee / Brian / Boris any ideas on this topic are appreciated. Regards, Alex On 27.09.2018 13:37, George Dunlap wrote: > On 09/26/2018 06:22 PM, Andrew Cooper wrote: >> On 26/09/18 17:47, George Dunlap wrote: >>> From: Isaila Alexandru <aisaila@bitdefender.com> >>> >>> This patch adds access control for NPT mode. >>> >>> There aren’t enough extra bits to store the access rights in the NPT p2m >>> table, so we add a radix tree to store extra information. >> >> I'm sorry to re-open this argument, but why? >> >> ISTR there being some argument based on pagetable sharing with the >> IOMMU, but that doesn't work at the moment and can't reasonably be made >> to work. For one, attempting to use pt sharing will break as soon as >> you try and DMA to a mapped grant. >> >> I'm disinclined to let a broken vestigial feature get in the way of real >> improvements. >> >> Beyond that, an NPT PTE has basically the same number of software >> available bits as an EPT PTE. >> >> Am I missing anything? > > Wow -- looks like IOMMU/p2m sharing has been disabled unconditionally > since 2014. If nobody has complained since then, that seems like a good > enough reason to me to rip it out. > > Suravee / Brian / Boris -- any opinions? > > The main reason to go with the 'extra bits' solution rather than the > 'rip out iommu/p2m sharing' solution is because people have been > prognosticating for years that we would be running out of bits and need > more at some point in the future. I thought Paul, for instance, might > have a use for the extra bits. But I'm happy to wait until such time as > we need it and then fish this patch out of the mail archives. > > -George > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT 2018-09-26 16:47 ` [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT George Dunlap 2018-09-26 17:22 ` Andrew Cooper @ 2018-09-27 9:38 ` Isaila Alexandru 2019-06-13 10:56 ` [Xen-devel] " Alexandru Stefan ISAILA 2019-06-13 15:19 ` Tamas Lengyel 3 siblings, 0 replies; 16+ messages in thread From: Isaila Alexandru @ 2018-09-27 9:38 UTC (permalink / raw) To: George Dunlap, xen-devel Cc: Razvan Cojocaru, Andrew Cooper, Tim Deegan, Paul Durrant, Jan Beulich, Tamas K Lengyel On Wed, 2018-09-26 at 17:47 +0100, George Dunlap wrote: > From: Isaila Alexandru <aisaila@bitdefender.com> > > This patch adds access control for NPT mode. > > There aren’t enough extra bits to store the access rights in the NPT > p2m > table, so we add a radix tree to store extra information. > > For efficiency: > - Only allocate this radix tree when we first store "non-default" > extra information > > - Remove entires which match the default extra information rather > than continuing to store them > > - For superpages, only store an entry for the first gfn in the > superpage. Use the order of the p2m entry being read to determine > the proper place to look in the radix table. > > Modify p2m_type_to_flags() to accept and interpret an access value, > parallel to the ept code. > > Add a set_default_access() method to the p2m-pt and p2m-ept versions > of the p2m rather than setting it directly, to deal with different > default permitted access values. > > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > Signed-off-by: George Dunlap <george.dunlap@citrix.com> > --- > NB, this is compile-tested only. I've tested this with xen-access and it works as expected > > diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c > index 3c42e21906..2e6b1e75e4 100644 > --- a/xen/arch/x86/monitor.c > +++ b/xen/arch/x86/monitor.c > @@ -20,6 +20,7 @@ > */ > > #include <asm/monitor.h> > +#include <asm/p2m.h> Is this intended? Regards, Alex _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT 2018-09-26 16:47 ` [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT George Dunlap 2018-09-26 17:22 ` Andrew Cooper 2018-09-27 9:38 ` Isaila Alexandru @ 2019-06-13 10:56 ` Alexandru Stefan ISAILA 2019-06-17 10:48 ` George Dunlap 2019-06-13 15:19 ` Tamas Lengyel 3 siblings, 1 reply; 16+ messages in thread From: Alexandru Stefan ISAILA @ 2019-06-13 10:56 UTC (permalink / raw) To: George Dunlap, xen-devel Cc: Razvan Cojocaru, Andrew Cooper, Tim Deegan, Paul Durrant, Jan Beulich, Tamas K Lengyel On 26.09.2018 19:47, George Dunlap wrote: > From: Isaila Alexandru <aisaila@bitdefender.com> > > This patch adds access control for NPT mode. > > There aren’t enough extra bits to store the access rights in the NPT p2m > table, so we add a radix tree to store extra information. > > For efficiency: > - Only allocate this radix tree when we first store "non-default" > extra information > > - Remove entires which match the default extra information rather > than continuing to store them > > - For superpages, only store an entry for the first gfn in the > superpage. Use the order of the p2m entry being read to determine > the proper place to look in the radix table. > > Modify p2m_type_to_flags() to accept and interpret an access value, > parallel to the ept code. > > Add a set_default_access() method to the p2m-pt and p2m-ept versions > of the p2m rather than setting it directly, to deal with different > default permitted access values. > > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > Signed-off-by: George Dunlap <george.dunlap@citrix.com> > --- > NB, this is compile-tested only. > > cc'ing Paul because this is functionality he may want at some point in > the future. > > I'm not sure why we only allow 'int' to be stored in the radix tree, > but that throws away 30-some bits we could otherwise use. We might > consider revising this if we run out of bits here. > > CC: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Jan Beulich <jbeulich@suse.com> > CC: Tim Deegan <tim@xen.org> > CC: Tamas K Lengyel <tamas.lengyel@zentific.com> > CC: Paul Durrant <paul.durrant@citrix.com> > CC: Razvan Cojocaru <rcojocaru@bitdefender.com> Hi all, I know it's been some time from the start of this patch but can this move forward? Any thoughts or acks are appreciated. Thanks, Alex _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT 2019-06-13 10:56 ` [Xen-devel] " Alexandru Stefan ISAILA @ 2019-06-17 10:48 ` George Dunlap 2019-06-17 11:58 ` Andrew Cooper 0 siblings, 1 reply; 16+ messages in thread From: George Dunlap @ 2019-06-17 10:48 UTC (permalink / raw) To: Alexandru Stefan ISAILA, xen-devel Cc: Razvan Cojocaru, Andrew Cooper, Tim Deegan, Paul Durrant, Jan Beulich, Tamas K Lengyel On 6/13/19 11:56 AM, Alexandru Stefan ISAILA wrote: > > > On 26.09.2018 19:47, George Dunlap wrote: >> From: Isaila Alexandru <aisaila@bitdefender.com> >> >> This patch adds access control for NPT mode. >> >> There aren’t enough extra bits to store the access rights in the NPT p2m >> table, so we add a radix tree to store extra information. >> >> For efficiency: >> - Only allocate this radix tree when we first store "non-default" >> extra information >> >> - Remove entires which match the default extra information rather >> than continuing to store them >> >> - For superpages, only store an entry for the first gfn in the >> superpage. Use the order of the p2m entry being read to determine >> the proper place to look in the radix table. >> >> Modify p2m_type_to_flags() to accept and interpret an access value, >> parallel to the ept code. >> >> Add a set_default_access() method to the p2m-pt and p2m-ept versions >> of the p2m rather than setting it directly, to deal with different >> default permitted access values. >> >> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> >> Signed-off-by: George Dunlap <george.dunlap@citrix.com> >> --- >> NB, this is compile-tested only. >> >> cc'ing Paul because this is functionality he may want at some point in >> the future. >> >> I'm not sure why we only allow 'int' to be stored in the radix tree, >> but that throws away 30-some bits we could otherwise use. We might >> consider revising this if we run out of bits here. >> >> CC: Andrew Cooper <andrew.cooper3@citrix.com> >> CC: Jan Beulich <jbeulich@suse.com> >> CC: Tim Deegan <tim@xen.org> >> CC: Tamas K Lengyel <tamas.lengyel@zentific.com> >> CC: Paul Durrant <paul.durrant@citrix.com> >> CC: Razvan Cojocaru <rcojocaru@bitdefender.com> > > Hi all, > > I know it's been some time from the start of this patch but can this > move forward? Any thoughts or acks are appreciated. Right, well where we left this, the situation was that on AMD hardware with IOMMU / p2m sharing, there aren't enough bits. The two general fixes we have so far are: 1. Add a parallel tree for extra bits (this patch) 2. Rip out IOMMU / p2m sharing for AMD. #2 has the advantage that we don't need an entirely separate tree, as well as getting rid of code that has (apparently) been completely dead for 5 years. #1 has the advantage that we're set up for having a much larger number of IOREQ servers in the future. Nobody objected to #2. Without looking deeply into it, it seems like it might be a good idea, but I can't be sure without seeing what it would actually look like. The easiest way to press the point then would be to post a patch series ripping it out. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT 2019-06-17 10:48 ` George Dunlap @ 2019-06-17 11:58 ` Andrew Cooper 0 siblings, 0 replies; 16+ messages in thread From: Andrew Cooper @ 2019-06-17 11:58 UTC (permalink / raw) To: George Dunlap, Alexandru Stefan ISAILA, xen-devel Cc: Tamas K Lengyel, Paul Durrant, Tim Deegan, Razvan Cojocaru, Jan Beulich On 17/06/2019 11:48, George Dunlap wrote: > On 6/13/19 11:56 AM, Alexandru Stefan ISAILA wrote: >> >> On 26.09.2018 19:47, George Dunlap wrote: >>> From: Isaila Alexandru <aisaila@bitdefender.com> >>> >>> This patch adds access control for NPT mode. >>> >>> There aren’t enough extra bits to store the access rights in the NPT p2m >>> table, so we add a radix tree to store extra information. >>> >>> For efficiency: >>> - Only allocate this radix tree when we first store "non-default" >>> extra information >>> >>> - Remove entires which match the default extra information rather >>> than continuing to store them >>> >>> - For superpages, only store an entry for the first gfn in the >>> superpage. Use the order of the p2m entry being read to determine >>> the proper place to look in the radix table. >>> >>> Modify p2m_type_to_flags() to accept and interpret an access value, >>> parallel to the ept code. >>> >>> Add a set_default_access() method to the p2m-pt and p2m-ept versions >>> of the p2m rather than setting it directly, to deal with different >>> default permitted access values. >>> >>> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> >>> Signed-off-by: George Dunlap <george.dunlap@citrix.com> >>> --- >>> NB, this is compile-tested only. >>> >>> cc'ing Paul because this is functionality he may want at some point in >>> the future. >>> >>> I'm not sure why we only allow 'int' to be stored in the radix tree, >>> but that throws away 30-some bits we could otherwise use. We might >>> consider revising this if we run out of bits here. >>> >>> CC: Andrew Cooper <andrew.cooper3@citrix.com> >>> CC: Jan Beulich <jbeulich@suse.com> >>> CC: Tim Deegan <tim@xen.org> >>> CC: Tamas K Lengyel <tamas.lengyel@zentific.com> >>> CC: Paul Durrant <paul.durrant@citrix.com> >>> CC: Razvan Cojocaru <rcojocaru@bitdefender.com> >> Hi all, >> >> I know it's been some time from the start of this patch but can this >> move forward? Any thoughts or acks are appreciated. > Right, well where we left this, the situation was that on AMD hardware > with IOMMU / p2m sharing, there aren't enough bits. > > The two general fixes we have so far are: > 1. Add a parallel tree for extra bits (this patch) > 2. Rip out IOMMU / p2m sharing for AMD. > > #2 has the advantage that we don't need an entirely separate tree, as > well as getting rid of code that has (apparently) been completely dead > for 5 years. #1 has the advantage that we're set up for having a much > larger number of IOREQ servers in the future. > > Nobody objected to #2. Without looking deeply into it, it seems like it > might be a good idea, but I can't be sure without seeing what it would > actually look like. > > The easiest way to press the point then would be to post a patch series > ripping it out. IOMMU / p2m sharing on AMD doesn't work, and isn't available any more (despite the code looking suspiciously like it is usable). There was a bugfix to allow DMA mapping of granted pages, which is a prerequisite for PVH support, which requires using a nonzero p2m type, and is therefore incompatible with IOMMU/p2m sharing. I don't see any feasible way to bring p2m-sharing back into a working state on AMD. As a result, we'd be much better ripping out the dead code and more formally acknowledging that it is a dead feature, after which the existing p2m type/access infrastructure should work compatibly with the Intel implementation. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT 2018-09-26 16:47 ` [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT George Dunlap ` (2 preceding siblings ...) 2019-06-13 10:56 ` [Xen-devel] " Alexandru Stefan ISAILA @ 2019-06-13 15:19 ` Tamas Lengyel 2019-06-13 15:21 ` Razvan Cojocaru 3 siblings, 1 reply; 16+ messages in thread From: Tamas Lengyel @ 2019-06-13 15:19 UTC (permalink / raw) To: George Dunlap Cc: Razvan Cojocaru, Andrew Cooper, Tim Deegan, Paul Durrant, Jan Beulich, Isaila Alexandru, Xen-devel On Wed, Sep 26, 2018 at 10:49 AM George Dunlap <george.dunlap@citrix.com> wrote: > > From: Isaila Alexandru <aisaila@bitdefender.com> > > This patch adds access control for NPT mode. > > There aren’t enough extra bits to store the access rights in the NPT p2m > table, so we add a radix tree to store extra information. > > For efficiency: > - Only allocate this radix tree when we first store "non-default" > extra information > > - Remove entires which match the default extra information rather > than continuing to store them > > - For superpages, only store an entry for the first gfn in the > superpage. Use the order of the p2m entry being read to determine > the proper place to look in the radix table. > > Modify p2m_type_to_flags() to accept and interpret an access value, > parallel to the ept code. > > Add a set_default_access() method to the p2m-pt and p2m-ept versions > of the p2m rather than setting it directly, to deal with different > default permitted access values. > > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > Signed-off-by: George Dunlap <george.dunlap@citrix.com> The mem_access/monitor bits are fairly trivial: Acked-by: Tamas K Lengyel <tamas@tklengyel.com> > --- > NB, this is compile-tested only. Are you planning to do some actual testing? I would highly recommend that we see real test results before this is merged to verify functionality. Thanks, Tamas _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Xen-devel] [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT 2019-06-13 15:19 ` Tamas Lengyel @ 2019-06-13 15:21 ` Razvan Cojocaru 0 siblings, 0 replies; 16+ messages in thread From: Razvan Cojocaru @ 2019-06-13 15:21 UTC (permalink / raw) To: Tamas Lengyel, George Dunlap Cc: Andrew Cooper, Tim Deegan, Paul Durrant, Jan Beulich, Isaila Alexandru, Xen-devel On 6/13/19 6:19 PM, Tamas Lengyel wrote: > On Wed, Sep 26, 2018 at 10:49 AM George Dunlap <george.dunlap@citrix.com> wrote: >> >> From: Isaila Alexandru <aisaila@bitdefender.com> >> >> This patch adds access control for NPT mode. >> >> There aren’t enough extra bits to store the access rights in the NPT p2m >> table, so we add a radix tree to store extra information. >> >> For efficiency: >> - Only allocate this radix tree when we first store "non-default" >> extra information >> >> - Remove entires which match the default extra information rather >> than continuing to store them >> >> - For superpages, only store an entry for the first gfn in the >> superpage. Use the order of the p2m entry being read to determine >> the proper place to look in the radix table. >> >> Modify p2m_type_to_flags() to accept and interpret an access value, >> parallel to the ept code. >> >> Add a set_default_access() method to the p2m-pt and p2m-ept versions >> of the p2m rather than setting it directly, to deal with different >> default permitted access values. >> >> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> >> Signed-off-by: George Dunlap <george.dunlap@citrix.com> > > The mem_access/monitor bits are fairly trivial: > > Acked-by: Tamas K Lengyel <tamas@tklengyel.com> > >> --- >> NB, this is compile-tested only. > > Are you planning to do some actual testing? I would highly recommend > that we see real test results before this is merged to verify > functionality. We did do some testing with xen-access at the time, but limited testing with the actual full-blown introspection agent (because not all the needed pieces align yet). Things did appear to work as intended. Thanks, Razvan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/2] mem_access: Fix npfec.kind propagation 2018-09-26 16:47 [RFC PATCH 1/2] mem_access: Fix npfec.kind propagation George Dunlap 2018-09-26 16:47 ` [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT George Dunlap @ 2018-09-26 16:51 ` Tamas Lengyel 2018-09-26 17:00 ` Andrew Cooper 2 siblings, 0 replies; 16+ messages in thread From: Tamas Lengyel @ 2018-09-26 16:51 UTC (permalink / raw) To: George Dunlap Cc: Razvan Cojocaru, Andrew Cooper, Tim Deegan, Jan Beulich, Alexandru Isaila, Xen-devel On Wed, Sep 26, 2018 at 10:49 AM George Dunlap <george.dunlap@citrix.com> wrote: > > The name of the "with_gla" flag is confusing; it has nothing to do > with the existence or lack thereof of a faulting GLA, but rather where > the fault originated. The npfec.kind value is always valid, and > should thus be propagated, regardless of whether gla_valid is set or > not. > > In particular, gla_valid will never be set on AMD systems; but > npfec.kind will still be valid and should still be propagated. > > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > Signed-off-by: George Dunlap <george.dunlap@citrix.com> Acked-by: Tamas K Lengyel <tamas@tklengyel.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/2] mem_access: Fix npfec.kind propagation 2018-09-26 16:47 [RFC PATCH 1/2] mem_access: Fix npfec.kind propagation George Dunlap 2018-09-26 16:47 ` [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT George Dunlap 2018-09-26 16:51 ` [RFC PATCH 1/2] mem_access: Fix npfec.kind propagation Tamas Lengyel @ 2018-09-26 17:00 ` Andrew Cooper 2018-09-27 7:04 ` Jan Beulich 2 siblings, 1 reply; 16+ messages in thread From: Andrew Cooper @ 2018-09-26 17:00 UTC (permalink / raw) To: George Dunlap, xen-devel Cc: Alexandru Isaila, Tamas K Lengyel, Tim Deegan, Razvan Cojocaru, Jan Beulich On 26/09/18 17:47, George Dunlap wrote: > The name of the "with_gla" flag is confusing; it has nothing to do > with the existence or lack thereof of a faulting GLA, but rather where > the fault originated. The npfec.kind value is always valid, and > should thus be propagated, regardless of whether gla_valid is set or > not. > > In particular, gla_valid will never be set on AMD systems; but > npfec.kind will still be valid and should still be propagated. > > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > Signed-off-by: George Dunlap <george.dunlap@citrix.com> > --- > CC: Andrew Cooper <andrew.cooper3@citrix.com> > CC: Jan Beulich <jbeulich@suse.com> > CC: Tim Deegan <tim@xen.org> > CC: Tamas K Lengyel <tamas.lengyel@zentific.com> > CC: Razvan Cojocaru <rcojocaru@bitdefender.com> > --- > xen/arch/x86/mm/mem_access.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c > index d9e64fcbb9..699a315076 100644 > --- a/xen/arch/x86/mm/mem_access.c > +++ b/xen/arch/x86/mm/mem_access.c > @@ -232,12 +232,12 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla, > { > req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID; > req->u.mem_access.gla = gla; > - > - if ( npfec.kind == npfec_kind_with_gla ) > - req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA; > - else if ( npfec.kind == npfec_kind_in_gpt ) > - req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT; > } > + > + if ( npfec.kind == npfec_kind_with_gla ) > + req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA; > + else if ( npfec.kind == npfec_kind_in_gpt ) > + req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT; Nit. Newline here please, as it is not logically related with the block below. ~Andrew > req->u.mem_access.flags |= npfec.read_access ? MEM_ACCESS_R : 0; > req->u.mem_access.flags |= npfec.write_access ? MEM_ACCESS_W : 0; > req->u.mem_access.flags |= npfec.insn_fetch ? MEM_ACCESS_X : 0; _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/2] mem_access: Fix npfec.kind propagation 2018-09-26 17:00 ` Andrew Cooper @ 2018-09-27 7:04 ` Jan Beulich 2018-09-27 8:46 ` George Dunlap 0 siblings, 1 reply; 16+ messages in thread From: Jan Beulich @ 2018-09-27 7:04 UTC (permalink / raw) To: george.dunlap Cc: Razvan Cojocaru, Andrew Cooper, Tim Deegan, Tamas K Lengyel, aisaila, xen-devel >>> On 26.09.18 at 19:00, <andrew.cooper3@citrix.com> wrote: > On 26/09/18 17:47, George Dunlap wrote: >> --- a/xen/arch/x86/mm/mem_access.c >> +++ b/xen/arch/x86/mm/mem_access.c >> @@ -232,12 +232,12 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla, >> { >> req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID; >> req->u.mem_access.gla = gla; >> - >> - if ( npfec.kind == npfec_kind_with_gla ) >> - req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA; >> - else if ( npfec.kind == npfec_kind_in_gpt ) >> - req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT; >> } >> + >> + if ( npfec.kind == npfec_kind_with_gla ) >> + req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA; >> + else if ( npfec.kind == npfec_kind_in_gpt ) >> + req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT; > > Nit. Newline here please, as it is not logically related with the block > below. And, despite it being just two comparisons, perhaps better to make it a switch() at the same time? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 1/2] mem_access: Fix npfec.kind propagation 2018-09-27 7:04 ` Jan Beulich @ 2018-09-27 8:46 ` George Dunlap 0 siblings, 0 replies; 16+ messages in thread From: George Dunlap @ 2018-09-27 8:46 UTC (permalink / raw) To: Jan Beulich Cc: Razvan Cojocaru, Andrew Cooper, Tim Deegan, Tamas K Lengyel, aisaila, xen-devel On 09/27/2018 08:04 AM, Jan Beulich wrote: >>>> On 26.09.18 at 19:00, <andrew.cooper3@citrix.com> wrote: >> On 26/09/18 17:47, George Dunlap wrote: >>> --- a/xen/arch/x86/mm/mem_access.c >>> +++ b/xen/arch/x86/mm/mem_access.c >>> @@ -232,12 +232,12 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla, >>> { >>> req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID; >>> req->u.mem_access.gla = gla; >>> - >>> - if ( npfec.kind == npfec_kind_with_gla ) >>> - req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA; >>> - else if ( npfec.kind == npfec_kind_in_gpt ) >>> - req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT; >>> } >>> + >>> + if ( npfec.kind == npfec_kind_with_gla ) >>> + req->u.mem_access.flags |= MEM_ACCESS_FAULT_WITH_GLA; >>> + else if ( npfec.kind == npfec_kind_in_gpt ) >>> + req->u.mem_access.flags |= MEM_ACCESS_FAULT_IN_GPT; >> >> Nit. Newline here please, as it is not logically related with the block >> below. > > And, despite it being just two comparisons, perhaps better to > make it a switch() at the same time? Sure -- this is logically separate from the follow-up patch, so I'll re-send it as a singleton with the comments. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-06-17 11:59 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-26 16:47 [RFC PATCH 1/2] mem_access: Fix npfec.kind propagation George Dunlap 2018-09-26 16:47 ` [RFC PATCH 2/2] x86/mm: Add mem access rights to NPT George Dunlap 2018-09-26 17:22 ` Andrew Cooper 2018-09-27 10:37 ` George Dunlap 2018-09-27 10:53 ` Paul Durrant 2019-01-09 9:30 ` Alexandru Stefan ISAILA 2018-09-27 9:38 ` Isaila Alexandru 2019-06-13 10:56 ` [Xen-devel] " Alexandru Stefan ISAILA 2019-06-17 10:48 ` George Dunlap 2019-06-17 11:58 ` Andrew Cooper 2019-06-13 15:19 ` Tamas Lengyel 2019-06-13 15:21 ` Razvan Cojocaru 2018-09-26 16:51 ` [RFC PATCH 1/2] mem_access: Fix npfec.kind propagation Tamas Lengyel 2018-09-26 17:00 ` Andrew Cooper 2018-09-27 7:04 ` Jan Beulich 2018-09-27 8:46 ` George Dunlap
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).