Re: [Xen-devel] [PATCH v3 10/16] x86/mm: put nested p2m code under CONFIG_HVM

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 09/13/2018 04:46 PM, Wei Liu wrote:
> On Thu, Sep 06, 2018 at 05:20:53PM +0100, George Dunlap wrote:
>> On 09/04/2018 05:15 PM, Wei Liu wrote:
>>> These functions are only useful for nested hvm, which isn't enabled
>>> when CONFIG_HVM is false.
>>>
>>> Enclose relevant code and fields in CONFIG_HVM. Guard np2m_schedule
>>> with nestedhvm_enabled.
>>>
>>> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
>>> ---
>>>  xen/arch/x86/domain.c        |  6 ++++--
>>>  xen/arch/x86/mm/p2m.c        | 18 ++++++++++++++----
>>>  xen/include/asm-x86/domain.h |  2 ++
>>>  xen/include/asm-x86/p2m.h    |  2 ++
>>>  4 files changed, 22 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>>> index 313ebb3221..7c945a2428 100644
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -1691,7 +1691,8 @@ void context_switch(struct vcpu *prev, struct vcpu 
>>> *next)
>>>      {
>>>          _update_runstate_area(prev);
>>>          vpmu_switch_from(prev);
>>> -        np2m_schedule(NP2M_SCHEDLE_OUT);
>>> +        if ( nestedhvm_enabled(prevd) )
>>> +            np2m_schedule(NP2M_SCHEDLE_OUT);
>>>      }
>>>  
>>>      if ( is_hvm_domain(prevd) && !list_empty(&prev->arch.hvm.tm_list) )
>>> @@ -1758,7 +1759,8 @@ void context_switch(struct vcpu *prev, struct vcpu 
>>> *next)
>>>  
>>>          /* Must be done with interrupts enabled */
>>>          vpmu_switch_to(next);
>>> -        np2m_schedule(NP2M_SCHEDLE_IN);
>>> +        if ( nestedhvm_enabled(nextd) )
>>> +            np2m_schedule(NP2M_SCHEDLE_IN);
>>
>> There's already a nestedhvm_enabled() check first thing in
>> np2m_schedule().  How does adding this check help the CONFIG_HVM cause?
> 
> np2m_schedule will be gone entirely when !HVM.  Add nestedhvm_enabled
> check here, which always evaluates to false when !HVM, makes compiler
> able to eliminate the call to np2m_schedule when linking.
> 
>>
>> And why not #ifdef out this call, as well as the np2m_schedule()
>> functions entirely?
> 
> The end result is the same. I'm happy to use either.

The end result may usually be the same in the case of !CONFIG_HVM, but
in the case of CONFIG_HVM, you'll have added a completely redundant
conditional.  More importantly,  if you use #ifdef, then it will be
immediately obvious to anyone reading the code that np2m_schedule isn't
called if !CONFIG_HVM.

Unless there are better reasons I'm not aware of, I'd prefer #ifdef here.

Thanks,
 -George

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