Re: [PATCH v2 9/9] xen: Convert __page_to_mfn and __mfn_to_page to use typesafe MFN

[Julien Grall <julien.grall@linaro.org>]

Hi Paul,

On 06/10/17 10:11, Paul Durrant wrote:
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index 0410b1e86b..1e7a0c6c40 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -38,12 +38,6 @@ static unsigned int __read_mostly max_vmid =
>> MAX_VMID_8_BIT;
>>
>>   #define P2M_ROOT_PAGES    (1<<P2M_ROOT_ORDER)
>>
>> -/* Override macros from asm/mm.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))
>> -
>>   unsigned int __read_mostly p2m_ipa_bits;
>>
>>   /* Helpers to lookup the properties of each level */
>> @@ -98,7 +92,7 @@ void dump_p2m_lookup(struct domain *d, paddr_t
>> addr)
>>       printk("dom%d IPA 0x%"PRIpaddr"\n", d->domain_id, addr);
>>
>>       printk("P2M @ %p mfn:0x%lx\n",
>> -           p2m->root, __page_to_mfn(p2m->root));
>> +           p2m->root, mfn_x(page_to_mfn(p2m->root)));
> 
> The format specifier should really be using PRI_mfn now. Same goes for others below.

Similarly we could do much more clean-up in each chunk. So where do I 
stop? That's why I wrote down in this comment I will not handle all the 
clean-up...

>> diff --git a/xen/arch/x86/pv/descriptor-tables.c
>> b/xen/arch/x86/pv/descriptor-tables.c
>> index 81973af124..f2b20f9910 100644
>> --- a/xen/arch/x86/pv/descriptor-tables.c
>> +++ b/xen/arch/x86/pv/descriptor-tables.c
>> @@ -25,16 +25,6 @@
>>   #include <asm/p2m.h>
>>   #include <asm/pv/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))
>> -
>> -/*******************
>> - * Descriptor Tables
>> - */
>> -
> 
> Is the comment wrong?

[...]

>> diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-
>> op.c
>> index dd90713acf..9ccbd021ef 100644
>> --- 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
>> - */
>> -
> 
> What's wrong with the comment?

The file is dedicated to I/O emulation support as said in the header and 
the name. I can understand why it was there given there was macros 
defined not related to I/O. Now they are dropped, why would you need a 
comment to separate includes and the code?

[...]

>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>> index 86506f3747..b85394d1f9 100644
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -811,7 +811,7 @@ int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
>>
>>               gdprintk(XENLOG_WARNING,
>>                        "Bad GMFN %lx (MFN %lx) to MSR %08x\n",
>> -                     gmfn, page ? page_to_mfn(page) : -1UL, base);
>> +                     gmfn, page ? mfn_x(page_to_mfn(page)) : -1UL, base);
> 
> Would this not be better as mfn_x(page ? page_to_mfn(page) : INVALID_MFN), as you have done elsewhere?

See above.

-- 
Julien Grall

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