| Message ID | 1503515375-3030-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 23.08.17 at 21:09, <andrew.cooper3@citrix.com> wrote: > * These functions work in terms of linear addresses, not virtual addresses. > Update the comments and parameter names. > * Drop unnecessary inlines. > * Drop vcpu parameter from guest_get_eff_kern_l1e(). Its sole caller passes > current, and its callee strictly operates on current. I'm not entirely convinced of this part, as I think the intention was to save the re-latching of "current", but anyway: > * Switch guest_get_eff_kern_l1e()'s parameter from void * to l1_pgentry_t > *. > Both its caller and callee already use the correct type already. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
On Wed, Aug 23, 2017 at 08:09:35PM +0100, Andrew Cooper wrote: > * These functions work in terms of linear addresses, not virtual addresses. > Update the comments and parameter names. > * Drop unnecessary inlines. > * Drop vcpu parameter from guest_get_eff_kern_l1e(). Its sole caller passes > current, and its callee strictly operates on current. > * Switch guest_get_eff_kern_l1e()'s parameter from void * to l1_pgentry_t *. > Both its caller and callee already use the correct type already. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
On 24/08/17 09:44, Jan Beulich wrote: > >>> On 23.08.17 at 21:09, <andrew.cooper3@citrix.com> wrote: >> * These functions work in terms of linear addresses, not virtual addresses. >> Update the comments and parameter names. >> * Drop unnecessary inlines. >> * Drop vcpu parameter from guest_get_eff_kern_l1e(). Its sole caller passes >> current, and its callee strictly operates on current. > I'm not entirely convinced of this part, as I think the intention was > to save the re-latching of "current", but anyway: It'll be inlined anyway, given a sole caller. We could have struct vcpu *curr as a parameter if you prefer, but having just a plain v gives the false impression it can be used with something other than current. ~Andrew > >> * Switch guest_get_eff_kern_l1e()'s parameter from void * to l1_pgentry_t >> *. >> Both its caller and callee already use the correct type already. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> >
>>> On 24.08.17 at 13:13, <andrew.cooper3@citrix.com> wrote: > On 24/08/17 09:44, Jan Beulich wrote: >> >>> On 23.08.17 at 21:09, <andrew.cooper3@citrix.com> wrote: >>> * These functions work in terms of linear addresses, not virtual addresses. >>> Update the comments and parameter names. >>> * Drop unnecessary inlines. >>> * Drop vcpu parameter from guest_get_eff_kern_l1e(). Its sole caller passes >>> current, and its callee strictly operates on current. >> I'm not entirely convinced of this part, as I think the intention was >> to save the re-latching of "current", but anyway: > > It'll be inlined anyway, given a sole caller. But the inlining won't help with the re-fetching; the compiler can't know it can re-use the earlier fetched value. > We could have struct vcpu *curr as a parameter if you prefer, but having > just a plain v gives the false impression it can be used with something > other than current. That would be an option, yes, but as said, the problem is minor enough for the patch to be good as is. Jan
On 24/08/17 13:35, Jan Beulich wrote: >>>> On 24.08.17 at 13:13, <andrew.cooper3@citrix.com> wrote: >> On 24/08/17 09:44, Jan Beulich wrote: >>> >>> On 23.08.17 at 21:09, <andrew.cooper3@citrix.com> wrote: >>>> * These functions work in terms of linear addresses, not virtual addresses. >>>> Update the comments and parameter names. >>>> * Drop unnecessary inlines. >>>> * Drop vcpu parameter from guest_get_eff_kern_l1e(). Its sole caller passes >>>> current, and its callee strictly operates on current. >>> I'm not entirely convinced of this part, as I think the intention was >>> to save the re-latching of "current", but anyway: >> It'll be inlined anyway, given a sole caller. > But the inlining won't help with the re-fetching; the compiler can't > know it can re-use the earlier fetched value. On that side of things, in the past I tried experimenting with attribute pure and const for things behind current. Such annotations should allow subexpression elimination and reordering, even across inlining boundaries, but I failed to get any change in generated code. I don't know if it was because I was annotating incorrectly, or GCC decided that elimination wasn't worth doing, but it might be something worth re-investigating. ~Andrew
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 3262499..8993e6d 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -561,15 +561,15 @@ static inline void guest_unmap_l1e(void *p) unmap_domain_page(p); } -/* Read a PV guest's l1e that maps this virtual address. */ -static inline void guest_get_eff_l1e(unsigned long addr, l1_pgentry_t *eff_l1e) +/* Read a PV guest's l1e that maps this linear address. */ +static void guest_get_eff_l1e(unsigned long linear, l1_pgentry_t *eff_l1e) { ASSERT(!paging_mode_translate(current->domain)); ASSERT(!paging_mode_external(current->domain)); - if ( unlikely(!__addr_ok(addr)) || + if ( unlikely(!__addr_ok(linear)) || __copy_from_user(eff_l1e, - &__linear_l1_table[l1_linear_offset(addr)], + &__linear_l1_table[l1_linear_offset(linear)], sizeof(l1_pgentry_t)) ) *eff_l1e = l1e_empty(); } @@ -578,18 +578,18 @@ static inline void guest_get_eff_l1e(unsigned long addr, l1_pgentry_t *eff_l1e) * Read the guest's l1e that maps this address, from the kernel-mode * page tables. */ -static inline void guest_get_eff_kern_l1e(struct vcpu *v, unsigned long addr, - void *eff_l1e) +static void guest_get_eff_kern_l1e(unsigned long linear, l1_pgentry_t *eff_l1e) { - const bool user_mode = !(v->arch.flags & TF_kernel_mode); + struct vcpu *curr = current; + const bool user_mode = !(curr->arch.flags & TF_kernel_mode); if ( user_mode ) - toggle_guest_mode(v); + toggle_guest_mode(curr); - guest_get_eff_l1e(addr, eff_l1e); + guest_get_eff_l1e(linear, eff_l1e); if ( user_mode ) - toggle_guest_mode(v); + toggle_guest_mode(curr); } static inline void page_set_tlbflush_timestamp(struct page_info *page) @@ -676,7 +676,7 @@ int map_ldt_shadow_page(unsigned int off) if ( is_pv_32bit_domain(d) ) gva = (u32)gva; - guest_get_eff_kern_l1e(v, gva, &l1e); + guest_get_eff_kern_l1e(gva, &l1e); if ( unlikely(!(l1e_get_flags(l1e) & _PAGE_PRESENT)) ) return 0;
* These functions work in terms of linear addresses, not virtual addresses. Update the comments and parameter names. * Drop unnecessary inlines. * Drop vcpu parameter from guest_get_eff_kern_l1e(). Its sole caller passes current, and its callee strictly operates on current. * Switch guest_get_eff_kern_l1e()'s parameter from void * to l1_pgentry_t *. Both its caller and callee already use the correct type already. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wei.liu2@citrix.com> --- xen/arch/x86/mm.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)