* [PATCH 0/2] x86: more XSA-154 follow-ups @ 2016-03-03 9:54 Jan Beulich 2016-03-03 10:31 ` [PATCH 1/2] x86: use "unsigned int" for cache attribute values Jan Beulich 2016-03-03 10:31 ` [PATCH 2/2] x86/HVM: cache attribute pinning adjustments Jan Beulich 0 siblings, 2 replies; 10+ messages in thread From: Jan Beulich @ 2016-03-03 9:54 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Keir Fraser 1: use "unsigned int" for cache attribute values 2: HVM: cache attribute pinning adjustments Signed-off-by: Jan Beulich <jbeulich@suse.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] x86: use "unsigned int" for cache attribute values 2016-03-03 9:54 [PATCH 0/2] x86: more XSA-154 follow-ups Jan Beulich @ 2016-03-03 10:31 ` Jan Beulich 2016-03-03 10:36 ` Andrew Cooper 2016-03-03 10:31 ` [PATCH 2/2] x86/HVM: cache attribute pinning adjustments Jan Beulich 1 sibling, 1 reply; 10+ messages in thread From: Jan Beulich @ 2016-03-03 10:31 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Keir Fraser [-- Attachment #1: Type: text/plain, Size: 1551 bytes --] ... where suitable. But note that the type of "cacheattr" in get_page_from_l1e() specifically needs to remain "unsigned long". Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -802,7 +802,7 @@ int is_iomem_page(unsigned long mfn) return (page_get_owner(page) == dom_io); } -static int update_xen_mappings(unsigned long mfn, unsigned long cacheattr) +static int update_xen_mappings(unsigned long mfn, unsigned int cacheattr) { int err = 0; bool_t alias = mfn >= PFN_DOWN(xen_phys_start) && @@ -2136,7 +2136,7 @@ static int mod_l4_entry(l4_pgentry_t *pl static int cleanup_page_cacheattr(struct page_info *page) { - uint32_t cacheattr = + unsigned int cacheattr = (page->count_info & PGC_cacheattr_mask) >> PGC_cacheattr_base; if ( likely(cacheattr == 0) ) --- a/xen/include/asm-x86/page.h +++ b/xen/include/asm-x86/page.h @@ -348,11 +348,11 @@ void free_xen_pagetable(void *v); l1_pgentry_t *virt_to_xen_l1e(unsigned long v); /* Convert between PAT/PCD/PWT embedded in PTE flags and 3-bit cacheattr. */ -static inline uint32_t pte_flags_to_cacheattr(uint32_t flags) +static inline unsigned int pte_flags_to_cacheattr(unsigned int flags) { return ((flags >> 5) & 4) | ((flags >> 3) & 3); } -static inline uint32_t cacheattr_to_pte_flags(uint32_t cacheattr) +static inline unsigned int cacheattr_to_pte_flags(unsigned int cacheattr) { return ((cacheattr & 4) << 5) | ((cacheattr & 3) << 3); } [-- Attachment #2: x86-uxm-param-type.patch --] [-- Type: text/plain, Size: 1599 bytes --] x86: use "unsigned int" for cache attribute values ... where suitable. But note that the type of "cacheattr" in get_page_from_l1e() specifically needs to remain "unsigned long". Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -802,7 +802,7 @@ int is_iomem_page(unsigned long mfn) return (page_get_owner(page) == dom_io); } -static int update_xen_mappings(unsigned long mfn, unsigned long cacheattr) +static int update_xen_mappings(unsigned long mfn, unsigned int cacheattr) { int err = 0; bool_t alias = mfn >= PFN_DOWN(xen_phys_start) && @@ -2136,7 +2136,7 @@ static int mod_l4_entry(l4_pgentry_t *pl static int cleanup_page_cacheattr(struct page_info *page) { - uint32_t cacheattr = + unsigned int cacheattr = (page->count_info & PGC_cacheattr_mask) >> PGC_cacheattr_base; if ( likely(cacheattr == 0) ) --- a/xen/include/asm-x86/page.h +++ b/xen/include/asm-x86/page.h @@ -348,11 +348,11 @@ void free_xen_pagetable(void *v); l1_pgentry_t *virt_to_xen_l1e(unsigned long v); /* Convert between PAT/PCD/PWT embedded in PTE flags and 3-bit cacheattr. */ -static inline uint32_t pte_flags_to_cacheattr(uint32_t flags) +static inline unsigned int pte_flags_to_cacheattr(unsigned int flags) { return ((flags >> 5) & 4) | ((flags >> 3) & 3); } -static inline uint32_t cacheattr_to_pte_flags(uint32_t cacheattr) +static inline unsigned int cacheattr_to_pte_flags(unsigned int cacheattr) { return ((cacheattr & 4) << 5) | ((cacheattr & 3) << 3); } [-- Attachment #3: 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] 10+ messages in thread
* Re: [PATCH 1/2] x86: use "unsigned int" for cache attribute values 2016-03-03 10:31 ` [PATCH 1/2] x86: use "unsigned int" for cache attribute values Jan Beulich @ 2016-03-03 10:36 ` Andrew Cooper 0 siblings, 0 replies; 10+ messages in thread From: Andrew Cooper @ 2016-03-03 10:36 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: Keir Fraser On 03/03/16 10:31, Jan Beulich wrote: > ... where suitable. But note that the type of "cacheattr" in > get_page_from_l1e() specifically needs to remain "unsigned long". > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] x86/HVM: cache attribute pinning adjustments 2016-03-03 9:54 [PATCH 0/2] x86: more XSA-154 follow-ups Jan Beulich 2016-03-03 10:31 ` [PATCH 1/2] x86: use "unsigned int" for cache attribute values Jan Beulich @ 2016-03-03 10:31 ` Jan Beulich 2016-03-03 11:03 ` Andrew Cooper 1 sibling, 1 reply; 10+ messages in thread From: Jan Beulich @ 2016-03-03 10:31 UTC (permalink / raw) To: xen-devel; +Cc: Andrew Cooper, Keir Fraser, Wei Liu, Tim Deegan [-- Attachment #1: Type: text/plain, Size: 9184 bytes --] - call hvm_get_mem_pinned_cacheattr() for RAM ranges only (requires some re-ordering in epte_get_entry_emt(), to fully handle all MMIO aspects first) - it's documented to be intended for RAM only - remove unnecessary indirection for hvm_get_mem_pinned_cacheattr()'s return of the type - make hvm_set_mem_pinned_cacheattr() return an error on bad domain kind or obviously bad GFN range - also avoid cache flush on EPT when removing a UC- range - other code structure adjustments without intended functional change Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/mtrr.c +++ b/xen/arch/x86/hvm/mtrr.c @@ -521,14 +521,12 @@ struct hvm_mem_pinned_cacheattr_range { static DEFINE_RCU_READ_LOCK(pinned_cacheattr_rcu_lock); -void hvm_init_cacheattr_region_list( - struct domain *d) +void hvm_init_cacheattr_region_list(struct domain *d) { INIT_LIST_HEAD(&d->arch.hvm_domain.pinned_cacheattr_ranges); } -void hvm_destroy_cacheattr_region_list( - struct domain *d) +void hvm_destroy_cacheattr_region_list(struct domain *d) { struct list_head *head = &d->arch.hvm_domain.pinned_cacheattr_ranges; struct hvm_mem_pinned_cacheattr_range *range; @@ -543,20 +541,14 @@ void hvm_destroy_cacheattr_region_list( } } -int hvm_get_mem_pinned_cacheattr( - struct domain *d, - uint64_t guest_fn, - unsigned int order, - uint32_t *type) +int hvm_get_mem_pinned_cacheattr(struct domain *d, uint64_t guest_fn, + unsigned int order) { struct hvm_mem_pinned_cacheattr_range *range; uint64_t mask = ~(uint64_t)0 << order; - int rc = 0; + int rc = -ENXIO; - *type = ~0; - - if ( !is_hvm_domain(d) ) - return 0; + ASSERT(has_hvm_container_domain(d)); rcu_read_lock(&pinned_cacheattr_rcu_lock); list_for_each_entry_rcu ( range, @@ -566,14 +558,13 @@ int hvm_get_mem_pinned_cacheattr( if ( ((guest_fn & mask) >= range->start) && ((guest_fn | ~mask) <= range->end) ) { - *type = range->type; - rc = 1; + rc = range->type; break; } if ( ((guest_fn & mask) <= range->end) && (range->start <= (guest_fn | ~mask)) ) { - rc = -1; + rc = -EADDRNOTAVAIL; break; } } @@ -587,20 +578,21 @@ static void free_pinned_cacheattr_entry( xfree(container_of(rcu, struct hvm_mem_pinned_cacheattr_range, rcu)); } -int32_t hvm_set_mem_pinned_cacheattr( - struct domain *d, - uint64_t gfn_start, - uint64_t gfn_end, - uint32_t type) +int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start, + uint64_t gfn_end, uint32_t type) { struct hvm_mem_pinned_cacheattr_range *range; int rc = 1; - if ( !is_hvm_domain(d) || gfn_end < gfn_start ) - return 0; + if ( !is_hvm_domain(d) ) + return -EOPNOTSUPP; + + if ( gfn_end < gfn_start || (gfn_start | gfn_end) >> paddr_bits ) + return -EINVAL; - if ( type == XEN_DOMCTL_DELETE_MEM_CACHEATTR ) + switch ( type ) { + case XEN_DOMCTL_DELETE_MEM_CACHEATTR: /* Remove the requested range. */ rcu_read_lock(&pinned_cacheattr_rcu_lock); list_for_each_entry_rcu ( range, @@ -613,22 +605,37 @@ int32_t hvm_set_mem_pinned_cacheattr( type = range->type; call_rcu(&range->rcu, free_pinned_cacheattr_entry); p2m_memory_type_changed(d); - if ( type != PAT_TYPE_UNCACHABLE ) + switch ( type ) + { + case PAT_TYPE_UC_MINUS: + /* + * For EPT we can also avoid the flush in this case; + * see epte_get_entry_emt(). + */ + if ( hap_enabled(d) && cpu_has_vmx ) + case PAT_TYPE_UNCACHABLE: + break; + /* fall through */ + default: flush_all(FLUSH_CACHE); + break; + } return 0; } rcu_read_unlock(&pinned_cacheattr_rcu_lock); return -ENOENT; - } - if ( !((type == PAT_TYPE_UNCACHABLE) || - (type == PAT_TYPE_WRCOMB) || - (type == PAT_TYPE_WRTHROUGH) || - (type == PAT_TYPE_WRPROT) || - (type == PAT_TYPE_WRBACK) || - (type == PAT_TYPE_UC_MINUS)) || - !is_hvm_domain(d) ) + case PAT_TYPE_UC_MINUS: + case PAT_TYPE_UNCACHABLE: + case PAT_TYPE_WRBACK: + case PAT_TYPE_WRCOMB: + case PAT_TYPE_WRPROT: + case PAT_TYPE_WRTHROUGH: + break; + + default: return -EINVAL; + } rcu_read_lock(&pinned_cacheattr_rcu_lock); list_for_each_entry_rcu ( range, @@ -762,7 +769,6 @@ int epte_get_entry_emt(struct domain *d, unsigned int order, uint8_t *ipat, bool_t direct_mmio) { int gmtrr_mtype, hmtrr_mtype; - uint32_t type; struct vcpu *v = current; *ipat = 0; @@ -782,30 +788,28 @@ int epte_get_entry_emt(struct domain *d, mfn_x(mfn) + (1UL << order) - 1) ) return -1; - switch ( hvm_get_mem_pinned_cacheattr(d, gfn, order, &type) ) + if ( direct_mmio ) { - case 1: + if ( (mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >> order ) + return MTRR_TYPE_UNCACHABLE; + if ( order ) + return -1; *ipat = 1; - return type != PAT_TYPE_UC_MINUS ? type : PAT_TYPE_UNCACHABLE; - case -1: - return -1; + return MTRR_TYPE_WRBACK; } - if ( !need_iommu(d) && !cache_flush_permitted(d) ) + gmtrr_mtype = hvm_get_mem_pinned_cacheattr(d, gfn, order); + if ( gmtrr_mtype >= 0 ) { - ASSERT(!direct_mmio || - !((mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >> - order)); *ipat = 1; - return MTRR_TYPE_WRBACK; + return gmtrr_mtype != PAT_TYPE_UC_MINUS ? gmtrr_mtype + : MTRR_TYPE_UNCACHABLE; } + if ( gmtrr_mtype == -EADDRNOTAVAIL ) + return -1; - if ( direct_mmio ) + if ( !need_iommu(d) && !cache_flush_permitted(d) ) { - if ( (mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >> order ) - return MTRR_TYPE_UNCACHABLE; - if ( order ) - return -1; *ipat = 1; return MTRR_TYPE_WRBACK; } --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -607,7 +607,7 @@ _sh_propagate(struct vcpu *v, if ( (level == 1) && is_hvm_domain(d) && !is_xen_heap_mfn(mfn_x(target_mfn)) ) { - unsigned int type; + int type; ASSERT(!(sflags & (_PAGE_PAT | _PAGE_PCD | _PAGE_PWT))); @@ -618,7 +618,9 @@ _sh_propagate(struct vcpu *v, * 3) if disables snoop control, compute the PAT index with * gMTRR and gPAT. */ - if ( hvm_get_mem_pinned_cacheattr(d, gfn_x(target_gfn), 0, &type) ) + if ( !mmio_mfn && + (type = hvm_get_mem_pinned_cacheattr(d, gfn_x(target_gfn), + 0)) >= 0 ) sflags |= pat_type_2_pte_flags(type); else if ( d->arch.hvm_domain.is_in_uc_mode ) sflags |= pat_type_2_pte_flags(PAT_TYPE_UNCACHABLE); --- a/xen/include/asm-x86/hvm/cacheattr.h +++ b/xen/include/asm-x86/hvm/cacheattr.h @@ -1,29 +1,23 @@ #ifndef __HVM_CACHEATTR_H__ #define __HVM_CACHEATTR_H__ -void hvm_init_cacheattr_region_list( - struct domain *d); -void hvm_destroy_cacheattr_region_list( - struct domain *d); +#include <xen/types.h> + +struct domain; +void hvm_init_cacheattr_region_list(struct domain *d); +void hvm_destroy_cacheattr_region_list(struct domain *d); /* * To see guest_fn is in the pinned range or not, - * if yes, return 1, and set type to value in this range - * if no, return 0, setting type to ~0 - * if ambiguous, return -1, setting type to ~0 (possible only for order > 0) + * if yes, return the (non-negative) type + * if no or ambiguous, return a negative error code */ -int hvm_get_mem_pinned_cacheattr( - struct domain *d, - uint64_t guest_fn, - unsigned int order, - uint32_t *type); +int hvm_get_mem_pinned_cacheattr(struct domain *d, uint64_t guest_fn, + unsigned int order); /* Set pinned caching type for a domain. */ -int32_t hvm_set_mem_pinned_cacheattr( - struct domain *d, - uint64_t gfn_start, - uint64_t gfn_end, - uint32_t type); +int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start, + uint64_t gfn_end, uint32_t type); #endif /* __HVM_CACHEATTR_H__ */ [-- Attachment #2: x86-HVM-mem-pinned-cacheattr.patch --] [-- Type: text/plain, Size: 9228 bytes --] x86/HVM: cache attribute pinning adjustments - call hvm_get_mem_pinned_cacheattr() for RAM ranges only (requires some re-ordering in epte_get_entry_emt(), to fully handle all MMIO aspects first) - it's documented to be intended for RAM only - remove unnecessary indirection for hvm_get_mem_pinned_cacheattr()'s return of the type - make hvm_set_mem_pinned_cacheattr() return an error on bad domain kind or obviously bad GFN range - also avoid cache flush on EPT when removing a UC- range - other code structure adjustments without intended functional change Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/hvm/mtrr.c +++ b/xen/arch/x86/hvm/mtrr.c @@ -521,14 +521,12 @@ struct hvm_mem_pinned_cacheattr_range { static DEFINE_RCU_READ_LOCK(pinned_cacheattr_rcu_lock); -void hvm_init_cacheattr_region_list( - struct domain *d) +void hvm_init_cacheattr_region_list(struct domain *d) { INIT_LIST_HEAD(&d->arch.hvm_domain.pinned_cacheattr_ranges); } -void hvm_destroy_cacheattr_region_list( - struct domain *d) +void hvm_destroy_cacheattr_region_list(struct domain *d) { struct list_head *head = &d->arch.hvm_domain.pinned_cacheattr_ranges; struct hvm_mem_pinned_cacheattr_range *range; @@ -543,20 +541,14 @@ void hvm_destroy_cacheattr_region_list( } } -int hvm_get_mem_pinned_cacheattr( - struct domain *d, - uint64_t guest_fn, - unsigned int order, - uint32_t *type) +int hvm_get_mem_pinned_cacheattr(struct domain *d, uint64_t guest_fn, + unsigned int order) { struct hvm_mem_pinned_cacheattr_range *range; uint64_t mask = ~(uint64_t)0 << order; - int rc = 0; + int rc = -ENXIO; - *type = ~0; - - if ( !is_hvm_domain(d) ) - return 0; + ASSERT(has_hvm_container_domain(d)); rcu_read_lock(&pinned_cacheattr_rcu_lock); list_for_each_entry_rcu ( range, @@ -566,14 +558,13 @@ int hvm_get_mem_pinned_cacheattr( if ( ((guest_fn & mask) >= range->start) && ((guest_fn | ~mask) <= range->end) ) { - *type = range->type; - rc = 1; + rc = range->type; break; } if ( ((guest_fn & mask) <= range->end) && (range->start <= (guest_fn | ~mask)) ) { - rc = -1; + rc = -EADDRNOTAVAIL; break; } } @@ -587,20 +578,21 @@ static void free_pinned_cacheattr_entry( xfree(container_of(rcu, struct hvm_mem_pinned_cacheattr_range, rcu)); } -int32_t hvm_set_mem_pinned_cacheattr( - struct domain *d, - uint64_t gfn_start, - uint64_t gfn_end, - uint32_t type) +int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start, + uint64_t gfn_end, uint32_t type) { struct hvm_mem_pinned_cacheattr_range *range; int rc = 1; - if ( !is_hvm_domain(d) || gfn_end < gfn_start ) - return 0; + if ( !is_hvm_domain(d) ) + return -EOPNOTSUPP; + + if ( gfn_end < gfn_start || (gfn_start | gfn_end) >> paddr_bits ) + return -EINVAL; - if ( type == XEN_DOMCTL_DELETE_MEM_CACHEATTR ) + switch ( type ) { + case XEN_DOMCTL_DELETE_MEM_CACHEATTR: /* Remove the requested range. */ rcu_read_lock(&pinned_cacheattr_rcu_lock); list_for_each_entry_rcu ( range, @@ -613,22 +605,37 @@ int32_t hvm_set_mem_pinned_cacheattr( type = range->type; call_rcu(&range->rcu, free_pinned_cacheattr_entry); p2m_memory_type_changed(d); - if ( type != PAT_TYPE_UNCACHABLE ) + switch ( type ) + { + case PAT_TYPE_UC_MINUS: + /* + * For EPT we can also avoid the flush in this case; + * see epte_get_entry_emt(). + */ + if ( hap_enabled(d) && cpu_has_vmx ) + case PAT_TYPE_UNCACHABLE: + break; + /* fall through */ + default: flush_all(FLUSH_CACHE); + break; + } return 0; } rcu_read_unlock(&pinned_cacheattr_rcu_lock); return -ENOENT; - } - if ( !((type == PAT_TYPE_UNCACHABLE) || - (type == PAT_TYPE_WRCOMB) || - (type == PAT_TYPE_WRTHROUGH) || - (type == PAT_TYPE_WRPROT) || - (type == PAT_TYPE_WRBACK) || - (type == PAT_TYPE_UC_MINUS)) || - !is_hvm_domain(d) ) + case PAT_TYPE_UC_MINUS: + case PAT_TYPE_UNCACHABLE: + case PAT_TYPE_WRBACK: + case PAT_TYPE_WRCOMB: + case PAT_TYPE_WRPROT: + case PAT_TYPE_WRTHROUGH: + break; + + default: return -EINVAL; + } rcu_read_lock(&pinned_cacheattr_rcu_lock); list_for_each_entry_rcu ( range, @@ -762,7 +769,6 @@ int epte_get_entry_emt(struct domain *d, unsigned int order, uint8_t *ipat, bool_t direct_mmio) { int gmtrr_mtype, hmtrr_mtype; - uint32_t type; struct vcpu *v = current; *ipat = 0; @@ -782,30 +788,28 @@ int epte_get_entry_emt(struct domain *d, mfn_x(mfn) + (1UL << order) - 1) ) return -1; - switch ( hvm_get_mem_pinned_cacheattr(d, gfn, order, &type) ) + if ( direct_mmio ) { - case 1: + if ( (mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >> order ) + return MTRR_TYPE_UNCACHABLE; + if ( order ) + return -1; *ipat = 1; - return type != PAT_TYPE_UC_MINUS ? type : PAT_TYPE_UNCACHABLE; - case -1: - return -1; + return MTRR_TYPE_WRBACK; } - if ( !need_iommu(d) && !cache_flush_permitted(d) ) + gmtrr_mtype = hvm_get_mem_pinned_cacheattr(d, gfn, order); + if ( gmtrr_mtype >= 0 ) { - ASSERT(!direct_mmio || - !((mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >> - order)); *ipat = 1; - return MTRR_TYPE_WRBACK; + return gmtrr_mtype != PAT_TYPE_UC_MINUS ? gmtrr_mtype + : MTRR_TYPE_UNCACHABLE; } + if ( gmtrr_mtype == -EADDRNOTAVAIL ) + return -1; - if ( direct_mmio ) + if ( !need_iommu(d) && !cache_flush_permitted(d) ) { - if ( (mfn_x(mfn) ^ d->arch.hvm_domain.vmx.apic_access_mfn) >> order ) - return MTRR_TYPE_UNCACHABLE; - if ( order ) - return -1; *ipat = 1; return MTRR_TYPE_WRBACK; } --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -607,7 +607,7 @@ _sh_propagate(struct vcpu *v, if ( (level == 1) && is_hvm_domain(d) && !is_xen_heap_mfn(mfn_x(target_mfn)) ) { - unsigned int type; + int type; ASSERT(!(sflags & (_PAGE_PAT | _PAGE_PCD | _PAGE_PWT))); @@ -618,7 +618,9 @@ _sh_propagate(struct vcpu *v, * 3) if disables snoop control, compute the PAT index with * gMTRR and gPAT. */ - if ( hvm_get_mem_pinned_cacheattr(d, gfn_x(target_gfn), 0, &type) ) + if ( !mmio_mfn && + (type = hvm_get_mem_pinned_cacheattr(d, gfn_x(target_gfn), + 0)) >= 0 ) sflags |= pat_type_2_pte_flags(type); else if ( d->arch.hvm_domain.is_in_uc_mode ) sflags |= pat_type_2_pte_flags(PAT_TYPE_UNCACHABLE); --- a/xen/include/asm-x86/hvm/cacheattr.h +++ b/xen/include/asm-x86/hvm/cacheattr.h @@ -1,29 +1,23 @@ #ifndef __HVM_CACHEATTR_H__ #define __HVM_CACHEATTR_H__ -void hvm_init_cacheattr_region_list( - struct domain *d); -void hvm_destroy_cacheattr_region_list( - struct domain *d); +#include <xen/types.h> + +struct domain; +void hvm_init_cacheattr_region_list(struct domain *d); +void hvm_destroy_cacheattr_region_list(struct domain *d); /* * To see guest_fn is in the pinned range or not, - * if yes, return 1, and set type to value in this range - * if no, return 0, setting type to ~0 - * if ambiguous, return -1, setting type to ~0 (possible only for order > 0) + * if yes, return the (non-negative) type + * if no or ambiguous, return a negative error code */ -int hvm_get_mem_pinned_cacheattr( - struct domain *d, - uint64_t guest_fn, - unsigned int order, - uint32_t *type); +int hvm_get_mem_pinned_cacheattr(struct domain *d, uint64_t guest_fn, + unsigned int order); /* Set pinned caching type for a domain. */ -int32_t hvm_set_mem_pinned_cacheattr( - struct domain *d, - uint64_t gfn_start, - uint64_t gfn_end, - uint32_t type); +int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start, + uint64_t gfn_end, uint32_t type); #endif /* __HVM_CACHEATTR_H__ */ [-- Attachment #3: 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] 10+ messages in thread
* Re: [PATCH 2/2] x86/HVM: cache attribute pinning adjustments 2016-03-03 10:31 ` [PATCH 2/2] x86/HVM: cache attribute pinning adjustments Jan Beulich @ 2016-03-03 11:03 ` Andrew Cooper 2016-03-03 12:10 ` Wei Liu 2016-03-03 12:41 ` Jan Beulich 0 siblings, 2 replies; 10+ messages in thread From: Andrew Cooper @ 2016-03-03 11:03 UTC (permalink / raw) To: Jan Beulich, xen-devel; +Cc: Keir Fraser, Wei Liu, Tim Deegan On 03/03/16 10:31, Jan Beulich wrote: > - call hvm_get_mem_pinned_cacheattr() for RAM ranges only (requires > some re-ordering in epte_get_entry_emt(), to fully handle all MMIO > aspects first) - it's documented to be intended for RAM only > - remove unnecessary indirection for hvm_get_mem_pinned_cacheattr()'s > return of the type > - make hvm_set_mem_pinned_cacheattr() return an error on bad domain > kind or obviously bad GFN range > - also avoid cache flush on EPT when removing a UC- range > - other code structure adjustments without intended functional change Reviewing would be far easier if these code structure changes were in a different patch. In particular, half the changes to epte_get_entry_emt() appear to just be reordering the direct_mmio check in order to drop an ASSERT(), but I am not quite sure, given the complexity of the change. > @@ -587,20 +578,21 @@ static void free_pinned_cacheattr_entry( > xfree(container_of(rcu, struct hvm_mem_pinned_cacheattr_range, rcu)); > } > > -int32_t hvm_set_mem_pinned_cacheattr( > - struct domain *d, > - uint64_t gfn_start, > - uint64_t gfn_end, > - uint32_t type) > +int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start, > + uint64_t gfn_end, uint32_t type) > { > struct hvm_mem_pinned_cacheattr_range *range; > int rc = 1; > > - if ( !is_hvm_domain(d) || gfn_end < gfn_start ) > - return 0; > + if ( !is_hvm_domain(d) ) > + return -EOPNOTSUPP; You introduce an asymmetry between set and get here, both in terms of the checks (hvm vs hvm_container), and assert vs plain failure. Why is this? I would suggest ASSERT(is_hvm_domain(d)) in both cases. > --- a/xen/include/asm-x86/hvm/cacheattr.h > +++ b/xen/include/asm-x86/hvm/cacheattr.h > @@ -1,29 +1,23 @@ > #ifndef __HVM_CACHEATTR_H__ > #define __HVM_CACHEATTR_H__ > > -void hvm_init_cacheattr_region_list( > - struct domain *d); > -void hvm_destroy_cacheattr_region_list( > - struct domain *d); > +#include <xen/types.h> > + > +struct domain; > +void hvm_init_cacheattr_region_list(struct domain *d); > +void hvm_destroy_cacheattr_region_list(struct domain *d); > > /* > * To see guest_fn is in the pinned range or not, > - * if yes, return 1, and set type to value in this range > - * if no, return 0, setting type to ~0 > - * if ambiguous, return -1, setting type to ~0 (possible only for order > 0) > + * if yes, return the (non-negative) type > + * if no or ambiguous, return a negative error code > */ > -int hvm_get_mem_pinned_cacheattr( > - struct domain *d, > - uint64_t guest_fn, > - unsigned int order, > - uint32_t *type); > +int hvm_get_mem_pinned_cacheattr(struct domain *d, uint64_t guest_fn, > + unsigned int order); fn, being the usual abbreviation for function, makes this confusing to read. As it is already changing, could we change the parameter name to gfn or guest_frame to be more consistent? (Perhaps even start using gfn_t if you are feeing keen). ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] x86/HVM: cache attribute pinning adjustments 2016-03-03 11:03 ` Andrew Cooper @ 2016-03-03 12:10 ` Wei Liu 2016-03-03 12:12 ` Andrew Cooper 2016-03-03 12:41 ` Jan Beulich 1 sibling, 1 reply; 10+ messages in thread From: Wei Liu @ 2016-03-03 12:10 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel, Keir Fraser, Wei Liu, Jan Beulich, Tim Deegan On Thu, Mar 03, 2016 at 11:03:43AM +0000, Andrew Cooper wrote: [...] > > @@ -587,20 +578,21 @@ static void free_pinned_cacheattr_entry( > > xfree(container_of(rcu, struct hvm_mem_pinned_cacheattr_range, rcu)); > > } > > > > -int32_t hvm_set_mem_pinned_cacheattr( > > - struct domain *d, > > - uint64_t gfn_start, > > - uint64_t gfn_end, > > - uint32_t type) > > +int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start, > > + uint64_t gfn_end, uint32_t type) > > { > > struct hvm_mem_pinned_cacheattr_range *range; > > int rc = 1; > > > > - if ( !is_hvm_domain(d) || gfn_end < gfn_start ) > > - return 0; > > + if ( !is_hvm_domain(d) ) > > + return -EOPNOTSUPP; > > You introduce an asymmetry between set and get here, both in terms of > the checks (hvm vs hvm_container), and assert vs plain failure. Why is > this? > > I would suggest ASSERT(is_hvm_domain(d)) in both cases. > I don't think we can have ASSERT() in the set function because it might be called by untrusted entity. On the other hand, the get function can only be used by hypervisor so the ASSERT should be fine. Wei. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] x86/HVM: cache attribute pinning adjustments 2016-03-03 12:10 ` Wei Liu @ 2016-03-03 12:12 ` Andrew Cooper 2016-03-03 12:19 ` Wei Liu 2016-03-03 12:46 ` Jan Beulich 0 siblings, 2 replies; 10+ messages in thread From: Andrew Cooper @ 2016-03-03 12:12 UTC (permalink / raw) To: Wei Liu; +Cc: xen-devel, Keir Fraser, Jan Beulich, Tim Deegan On 03/03/16 12:10, Wei Liu wrote: > On Thu, Mar 03, 2016 at 11:03:43AM +0000, Andrew Cooper wrote: > [...] >>> @@ -587,20 +578,21 @@ static void free_pinned_cacheattr_entry( >>> xfree(container_of(rcu, struct hvm_mem_pinned_cacheattr_range, rcu)); >>> } >>> >>> -int32_t hvm_set_mem_pinned_cacheattr( >>> - struct domain *d, >>> - uint64_t gfn_start, >>> - uint64_t gfn_end, >>> - uint32_t type) >>> +int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start, >>> + uint64_t gfn_end, uint32_t type) >>> { >>> struct hvm_mem_pinned_cacheattr_range *range; >>> int rc = 1; >>> >>> - if ( !is_hvm_domain(d) || gfn_end < gfn_start ) >>> - return 0; >>> + if ( !is_hvm_domain(d) ) >>> + return -EOPNOTSUPP; >> You introduce an asymmetry between set and get here, both in terms of >> the checks (hvm vs hvm_container), and assert vs plain failure. Why is >> this? >> >> I would suggest ASSERT(is_hvm_domain(d)) in both cases. >> > I don't think we can have ASSERT() in the set function because it might > be called by untrusted entity. On the other hand, the get function can > only be used by hypervisor so the ASSERT should be fine. The hypercall handler should sanitise the untrusted caller before we get into this function. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] x86/HVM: cache attribute pinning adjustments 2016-03-03 12:12 ` Andrew Cooper @ 2016-03-03 12:19 ` Wei Liu 2016-03-03 12:46 ` Jan Beulich 1 sibling, 0 replies; 10+ messages in thread From: Wei Liu @ 2016-03-03 12:19 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel, Keir Fraser, Wei Liu, Jan Beulich, Tim Deegan On Thu, Mar 03, 2016 at 12:12:40PM +0000, Andrew Cooper wrote: > On 03/03/16 12:10, Wei Liu wrote: > > On Thu, Mar 03, 2016 at 11:03:43AM +0000, Andrew Cooper wrote: > > [...] > >>> @@ -587,20 +578,21 @@ static void free_pinned_cacheattr_entry( > >>> xfree(container_of(rcu, struct hvm_mem_pinned_cacheattr_range, rcu)); > >>> } > >>> > >>> -int32_t hvm_set_mem_pinned_cacheattr( > >>> - struct domain *d, > >>> - uint64_t gfn_start, > >>> - uint64_t gfn_end, > >>> - uint32_t type) > >>> +int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start, > >>> + uint64_t gfn_end, uint32_t type) > >>> { > >>> struct hvm_mem_pinned_cacheattr_range *range; > >>> int rc = 1; > >>> > >>> - if ( !is_hvm_domain(d) || gfn_end < gfn_start ) > >>> - return 0; > >>> + if ( !is_hvm_domain(d) ) > >>> + return -EOPNOTSUPP; > >> You introduce an asymmetry between set and get here, both in terms of > >> the checks (hvm vs hvm_container), and assert vs plain failure. Why is > >> this? > >> > >> I would suggest ASSERT(is_hvm_domain(d)) in both cases. > >> > > I don't think we can have ASSERT() in the set function because it might > > be called by untrusted entity. On the other hand, the get function can > > only be used by hypervisor so the ASSERT should be fine. > > The hypercall handler should sanitise the untrusted caller before we get > into this function. > I don't disagree. It is just that this seems undone at the moment -- I don't see xsm hook in the callsite of this function in domctl. Wei. > ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] x86/HVM: cache attribute pinning adjustments 2016-03-03 12:12 ` Andrew Cooper 2016-03-03 12:19 ` Wei Liu @ 2016-03-03 12:46 ` Jan Beulich 1 sibling, 0 replies; 10+ messages in thread From: Jan Beulich @ 2016-03-03 12:46 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel, Keir Fraser, Wei Liu, Tim Deegan >>> On 03.03.16 at 13:12, <andrew.cooper3@citrix.com> wrote: > On 03/03/16 12:10, Wei Liu wrote: >> On Thu, Mar 03, 2016 at 11:03:43AM +0000, Andrew Cooper wrote: >> [...] >>>> @@ -587,20 +578,21 @@ static void free_pinned_cacheattr_entry( >>>> xfree(container_of(rcu, struct hvm_mem_pinned_cacheattr_range, rcu)); >>>> } >>>> >>>> -int32_t hvm_set_mem_pinned_cacheattr( >>>> - struct domain *d, >>>> - uint64_t gfn_start, >>>> - uint64_t gfn_end, >>>> - uint32_t type) >>>> +int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start, >>>> + uint64_t gfn_end, uint32_t type) >>>> { >>>> struct hvm_mem_pinned_cacheattr_range *range; >>>> int rc = 1; >>>> >>>> - if ( !is_hvm_domain(d) || gfn_end < gfn_start ) >>>> - return 0; >>>> + if ( !is_hvm_domain(d) ) >>>> + return -EOPNOTSUPP; >>> You introduce an asymmetry between set and get here, both in terms of >>> the checks (hvm vs hvm_container), and assert vs plain failure. Why is >>> this? >>> >>> I would suggest ASSERT(is_hvm_domain(d)) in both cases. >>> >> I don't think we can have ASSERT() in the set function because it might >> be called by untrusted entity. On the other hand, the get function can >> only be used by hypervisor so the ASSERT should be fine. > > The hypercall handler should sanitise the untrusted caller before we get > into this function. I don't think this would be a good idea: Once we extend this to also allow such ranges for PVH, keeping the change in policy to the source file / function actually carrying this out seems quite a bit more logical. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] x86/HVM: cache attribute pinning adjustments 2016-03-03 11:03 ` Andrew Cooper 2016-03-03 12:10 ` Wei Liu @ 2016-03-03 12:41 ` Jan Beulich 1 sibling, 0 replies; 10+ messages in thread From: Jan Beulich @ 2016-03-03 12:41 UTC (permalink / raw) To: Andrew Cooper; +Cc: xen-devel, Keir Fraser, Wei Liu, Tim Deegan >>> On 03.03.16 at 12:03, <andrew.cooper3@citrix.com> wrote: > On 03/03/16 10:31, Jan Beulich wrote: >> - call hvm_get_mem_pinned_cacheattr() for RAM ranges only (requires >> some re-ordering in epte_get_entry_emt(), to fully handle all MMIO >> aspects first) - it's documented to be intended for RAM only >> - remove unnecessary indirection for hvm_get_mem_pinned_cacheattr()'s >> return of the type >> - make hvm_set_mem_pinned_cacheattr() return an error on bad domain >> kind or obviously bad GFN range >> - also avoid cache flush on EPT when removing a UC- range >> - other code structure adjustments without intended functional change > > Reviewing would be far easier if these code structure changes were in a > different patch. In particular, half the changes to > epte_get_entry_emt() appear to just be reordering the direct_mmio check > in order to drop an ASSERT(), but I am not quite sure, given the > complexity of the change. I'll see about splitting them, albeit that's not going to help the hard to read change to epte_get_entry_emt() (in which removal of the ASSERT() isn't the goal, but a nice side effect). >> @@ -587,20 +578,21 @@ static void free_pinned_cacheattr_entry( >> xfree(container_of(rcu, struct hvm_mem_pinned_cacheattr_range, rcu)); >> } >> >> -int32_t hvm_set_mem_pinned_cacheattr( >> - struct domain *d, >> - uint64_t gfn_start, >> - uint64_t gfn_end, >> - uint32_t type) >> +int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start, >> + uint64_t gfn_end, uint32_t type) >> { >> struct hvm_mem_pinned_cacheattr_range *range; >> int rc = 1; >> >> - if ( !is_hvm_domain(d) || gfn_end < gfn_start ) >> - return 0; >> + if ( !is_hvm_domain(d) ) >> + return -EOPNOTSUPP; > > You introduce an asymmetry between set and get here, both in terms of > the checks (hvm vs hvm_container), and assert vs plain failure. Why is > this? > > I would suggest ASSERT(is_hvm_domain(d)) in both cases. No, in the "set" case we mustn't assert, or else the caller needs to change too. And switching to the container variant in "get" and converting to an assert at the same time is intended too: No caller calls this for a PV domain, and at least ept_get_entry_emt() may call this for a PVH one (just that currently "set" won't allow such ranges to be introduced for them - since I don't know the reason for this, I didn't want to change the behavior in this regard). >> --- a/xen/include/asm-x86/hvm/cacheattr.h >> +++ b/xen/include/asm-x86/hvm/cacheattr.h >> @@ -1,29 +1,23 @@ >> #ifndef __HVM_CACHEATTR_H__ >> #define __HVM_CACHEATTR_H__ >> >> -void hvm_init_cacheattr_region_list( >> - struct domain *d); >> -void hvm_destroy_cacheattr_region_list( >> - struct domain *d); >> +#include <xen/types.h> >> + >> +struct domain; >> +void hvm_init_cacheattr_region_list(struct domain *d); >> +void hvm_destroy_cacheattr_region_list(struct domain *d); >> >> /* >> * To see guest_fn is in the pinned range or not, >> - * if yes, return 1, and set type to value in this range >> - * if no, return 0, setting type to ~0 >> - * if ambiguous, return -1, setting type to ~0 (possible only for order > 0) >> + * if yes, return the (non-negative) type >> + * if no or ambiguous, return a negative error code >> */ >> -int hvm_get_mem_pinned_cacheattr( >> - struct domain *d, >> - uint64_t guest_fn, >> - unsigned int order, >> - uint32_t *type); >> +int hvm_get_mem_pinned_cacheattr(struct domain *d, uint64_t guest_fn, >> + unsigned int order); > > fn, being the usual abbreviation for function, makes this confusing to > read. As it is already changing, could we change the parameter name to > gfn or guest_frame to be more consistent? (Perhaps even start using > gfn_t if you are feeing keen). Well, yes, but that will mean yet more difficult to review a patch (and to be honest with things like this I don't really feel like splitting things up into too fine grained pieces). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-03-03 12:46 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-03-03 9:54 [PATCH 0/2] x86: more XSA-154 follow-ups Jan Beulich 2016-03-03 10:31 ` [PATCH 1/2] x86: use "unsigned int" for cache attribute values Jan Beulich 2016-03-03 10:36 ` Andrew Cooper 2016-03-03 10:31 ` [PATCH 2/2] x86/HVM: cache attribute pinning adjustments Jan Beulich 2016-03-03 11:03 ` Andrew Cooper 2016-03-03 12:10 ` Wei Liu 2016-03-03 12:12 ` Andrew Cooper 2016-03-03 12:19 ` Wei Liu 2016-03-03 12:46 ` Jan Beulich 2016-03-03 12:41 ` Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.