Re: [Xen-devel] [PATCH v3] boot allocator: Use arch helper for virt_to_mfn on DIRECTMAP

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

Hi Jan,

  Thanks for the review.

On Mon, Mar 27, 2017 at 12:50 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 27.03.17 at 09:10, <vijay.kilari@xxxxxxxxx> wrote:
>> @@ -254,7 +262,6 @@ static inline int gvirt_to_maddr(vaddr_t va, paddr_t 
>> *pa, unsigned int flags)
>>  #define virt_to_mfn(va)   (virt_to_maddr(va) >> PAGE_SHIFT)
>>  #define mfn_to_virt(mfn)  (maddr_to_virt((paddr_t)(mfn) << PAGE_SHIFT))
>>
>> -
>>  /* Convert between Xen-heap virtual addresses and page-info structures. */
>
> If I was an ARM maintainer, I'd object to such a stray change (even
> if generally it looks good to me to remove double blank lines).

Hmm. That got creeped from from previous commit change.. I will take care.

>
>> @@ -374,6 +375,17 @@ perms_strictly_increased(uint32_t old_flags, uint32_t 
>> new_flags)
>>      return ((of | (of ^ nf)) == nf);
>>  }
>>
>> +/*
>> + * x86 maps DIRECTMAP_VIRT to physical memory. Get the mfn for directmap
>> + * memory region.
>> + */
>> +static inline bool_t arch_mfn_below_directmap_max_mfn(unsigned long mfn)
>
> I'm pretty convinced it has been pointed out to you that we use
> plain bool nowadays. Also the function name looks overly long to
> me. How about arch_mfn_in_directmap()?

This name is fine with me.

>
>> +{
>> +    unsigned long eva = min(DIRECTMAP_VIRT_END, HYPERVISOR_VIRT_END);
>> +
>> +    return (mfn <= (virt_to_mfn(eva - 1) + 1)) ? true : false;
>
> There's absolutely no need for conditional expressions like this. The
> result of the comparison is fine as is for a function with a boolean
> result (and that was already the case back when we were still using
> bool_t).
OK
>
> Jan
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel