>>> On 15.03.18 at 16:03, <julien.gr...@arm.com> wrote:
> 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.
Well, as said - it's probably too much to ask for this patch anyway.
But no, the compiler cannot optimize it, at least not in the x86
case. Yet we have far more cases where both PV and HVM cases
are handled during guest copy, when one of the two is clearly
dead code.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
