Hi Jan, On 15/03/18 08:06, Jan Beulich wrote:
On 14.03.18 at 19:20, <julien.gr...@arm.com> wrote:@@ -95,11 +101,17 @@ static unsigned int max_order(const struct domain *d) return min(order, MAX_ORDER + 0U); }+/* Helper to copy a typesafe MFN to guest */+#define copy_mfn_to_guest(hnd, off, mfn) \ + ({ \ + xen_pfn_t mfn_ = mfn_x(mfn); \ + __copy_to_guest_offset(hnd, off, &mfn_, 1); \ + })However much I dislike introduction of new name space violations, I think following the global naming principle here is more important: As a function not validating the address range, this should have two leading underscores. Also - was there a reason for this not to be an inline function?
I thought the handle was different type at each call site. I was wrong, so turned it to static inline.
The other thing I notice only now is perhaps a little much too ask for a mostly mechanical change like this one: All uses of this sit inside !paging_mode_translate() checks, hence these could do nothing on ARM and resolve to __copy_to_user() on x86 (with the type checking suitably lifted to here).
I am quite reluctant to turn this function as nop for Arm. This is common code and should not assume the implementation of paging_mode_translate. Furthermore, I can't see the real benefits as the compiler will optimize out it.
@@ -132,8 +144,9 @@ static void increase_reservation(struct memop_args *a) if ( !paging_mode_translate(d) && !guest_handle_is_null(a->extent_list) ) { - mfn = page_to_mfn(page); - if ( unlikely(__copy_to_guest_offset(a->extent_list, i, &mfn, 1)) ) + mfn_t mfn = page_to_mfn(page); + + if ( unlikely(copy_mfn_to_guest(a->extent_list, i, mfn)) )Can't you avoid the intermediate variable altogether now?
I didn't do it because the line was getting too long and quite difficult to read if you split it.
Also, technically the compiler will be clever enough to optimize the code. So I don't see the benefits of removing the variable.
Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
