| Message ID | 56D820A402000078000D8BAC@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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
>>> 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
>>> 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
--- 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__ */