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

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

On 06/10/17 11:55, Paul Durrant wrote:
>>>> 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?
>>
> 
> It makes the hunk look odd though. I think you should leave the comment alone in this patch, even if you do think it superfluous.

Please get agree with Andrew... Here his comment on the previous version:

"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 :)."

> 
>> [...]
>>
>>>> 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.
> 
> And again, you are modifying the code so why not modify it such that it is coded appropriately, as you have in other places in this patch?

I will see what I can do when I will have time to spend on clean-up...

-- 
Julien Grall

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