Re: [PATCH v4 16/16] xen: Convert page_to_mfn and mfn_to_page to use typesafe MFN

[Julien Grall <julien.grall@arm.com>]

Hi Jan,

On 03/02/2018 04:08 PM, Jan Beulich wrote:
>>>> On 21.02.18 at 15:02, <julien.grall@arm.com> wrote:
>> --- a/xen/arch/x86/pv/emul-priv-op.c
>> +++ b/xen/arch/x86/pv/emul-priv-op.c
>> @@ -43,16 +43,6 @@
>>   #include "emulate.h"
>>   #include "mm.h"
>>   
>> -/* Override macros from asm/page.h to make them work with mfn_t */
>> -#undef mfn_to_page
>> -#define mfn_to_page(mfn) __mfn_to_page(mfn_x(mfn))
>> -#undef page_to_mfn
>> -#define page_to_mfn(pg) _mfn(__page_to_mfn(pg))
>> -
>> -/***********************
>> - * I/O emulation support
>> - */
> 
> Why does this comment go away?

 From an earlier review, Andrew said:

"If you're making this change, please take out the Descriptor Tables
comment like you do with I/O below, because the entire file is dedicated
to descriptor table support and it will save me one item on a cleanup
patch :)."

The descriptor one got remove by 634afe43ac "x86/pv: Rename 
invalidate_shadow_ldt() to pv_destroy_ldt()". So it is not part of this
patch anymore.

> 
>> @@ -478,10 +478,10 @@ extern paddr_t mem_hotplug;
>>   #define SHARED_M2P(_e)           ((_e) == SHARED_M2P_ENTRY)
>>   
>>   #define compat_machine_to_phys_mapping ((unsigned int
>> *)RDWR_COMPAT_MPT_VIRT_START)
>> -#define _set_gpfn_from_mfn(mfn, pfn) ({                        \
>> -    struct domain *d = page_get_owner(__mfn_to_page(mfn));     \
>> -    unsigned long entry = (d && (d == dom_cow)) ?              \
>> -        SHARED_M2P_ENTRY : (pfn);                              \
>> +#define _set_gpfn_from_mfn(mfn, pfn) ({                         \
>> +    struct domain *d = page_get_owner(mfn_to_page(_mfn(mfn)));    \
>> +    unsigned long entry = (d && (d == dom_cow)) ?               \
>> +        SHARED_M2P_ENTRY : (pfn);                               \
> 
> Please don't break the alignment of the backslashes here. It also looks
> like three of the four lines could be left alone altogether.

I am not sure why I modified the 3 other lines. I fixed it.

> 
>> @@ -157,10 +157,10 @@ static inline l4_pgentry_t l4e_from_paddr(paddr_t pa, unsigned int flags)
>>   #define l4e_from_intpte(intpte)    ((l4_pgentry_t) { (intpte_t)(intpte) })
>>   
>>   /* Construct a pte from a page pointer and access flags. */
>> -#define l1e_from_page(page, flags) l1e_from_pfn(__page_to_mfn(page), (flags))
>> -#define l2e_from_page(page, flags) l2e_from_pfn(__page_to_mfn(page), (flags))
>> -#define l3e_from_page(page, flags) l3e_from_pfn(__page_to_mfn(page), (flags))
>> -#define l4e_from_page(page, flags) l4e_from_pfn(__page_to_mfn(page), (flags))
>> +#define l1e_from_page(page, flags) l1e_from_mfn(page_to_mfn(page), (flags))
>> +#define l2e_from_page(page, flags) l2e_from_mfn(page_to_mfn(page), (flags))
>> +#define l3e_from_page(page, flags) l3e_from_mfn(page_to_mfn(page), (flags))
>> +#define l4e_from_page(page, flags) l4e_from_mfn(page_to_mfn(page), (flags))
> 
> Would again have been nice if you got rid of the extra parentheses
> here at the same time.

I admit, I don't spend my time trying to find the possible cleanup in 
the x86 code. I just do mechanical change and when I get bored I do a 
bit more.

> 
>> @@ -240,12 +240,12 @@ void copy_page_sse2(void *, const void *);
>>   #define __mfn_to_virt(mfn)  (maddr_to_virt((paddr_t)(mfn) << PAGE_SHIFT))
>>   
>>   /* Convert between machine frame numbers and page-info structures. */
>> -#define __mfn_to_page(mfn)  (frame_table + pfn_to_pdx(mfn))
>> -#define __page_to_mfn(pg)   pdx_to_pfn((unsigned long)((pg) - frame_table))
>> +#define mfn_to_page(mfn)    (frame_table + mfn_to_pdx(mfn))
>> +#define page_to_mfn(pg)     pdx_to_mfn((unsigned long)((pg) - frame_table))
>>   
>>   /* Convert between machine addresses and page-info structures. */
>> -#define __maddr_to_page(ma) __mfn_to_page((ma) >> PAGE_SHIFT)
>> -#define __page_to_maddr(pg) ((paddr_t)__page_to_mfn(pg) << PAGE_SHIFT)
>> +#define __maddr_to_page(ma) mfn_to_page(maddr_to_mfn(ma))
>> +#define __page_to_maddr(pg) (mfn_to_maddr(page_to_mfn(pg)))
> 
> Same here.
> 
> With at least the first two items taken care of, relevant x86 pieces
> Acked-by: Jan Beulich <jbeulich@suse.com>

I don't plan to address the first one as Andrew were happy with it.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel