Re: [PATCH 05/34] xen: is_hvm_domain should evaluate to 0 when !CONFIG_HVM

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 20/08/2018 10:06, Wei Liu wrote:
> On Sun, Aug 19, 2018 at 05:48:17PM +0100, Andrew Cooper wrote:
>> On 17/08/2018 16:12, Wei Liu wrote:
>>> Since it is defined in common header file, introduce CONFIG_HVM to
>>> Arm to avoid breakage.
>>>
>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>> ---
>>>  xen/arch/arm/Kconfig    | 3 +++
>>>  xen/include/xen/sched.h | 6 ++++++
>>>  2 files changed, 9 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>>> index 586bc62..c0e969e 100644
>>> --- a/xen/arch/arm/Kconfig
>>> +++ b/xen/arch/arm/Kconfig
>>> @@ -52,6 +52,9 @@ config HAS_ITS
>>>          prompt "GICv3 ITS MSI controller support" if EXPERT = "y"
>>>          depends on GICV3 && !NEW_VGIC
>>>  
>>> +config HVM
>>> +        def_bool y
>>> +
>>>  config NEW_VGIC
>>>  	bool
>>>  	prompt "Use new VGIC implementation"
>>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>>> index 51ceebe..8fc3423 100644
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -879,8 +879,14 @@ void watchdog_domain_destroy(struct domain *d);
>>>  
>>>  #define is_pv_domain(d) ((d)->guest_type == guest_type_pv)
>>>  #define is_pv_vcpu(v)   (is_pv_domain((v)->domain))
>>> +
>>> +#if CONFIG_HVM
>>>  #define is_hvm_domain(d) ((d)->guest_type == guest_type_hvm)
>>> +#else
>>> +#define is_hvm_domain(d) (0)
>>> +#endif
>>>  #define is_hvm_vcpu(v)   (is_hvm_domain(v->domain))
>> The need for the following patch is caused by a bug here, in that you
>> don't evaluate d.
> I know. I didn't classified that as a bug though.

I'm afraid that the x86 maintainership will disagree with you there.

>
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index 51ceebe..fdd18a7 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -879,8 +879,17 @@ void watchdog_domain_destroy(struct domain *d);
>>
>>  #define is_pv_domain(d) ((d)->guest_type == guest_type_pv)
>>  #define is_pv_vcpu(v)   (is_pv_domain((v)->domain))
>> -#define is_hvm_domain(d) ((d)->guest_type == guest_type_hvm)
>> -#define is_hvm_vcpu(v)   (is_hvm_domain(v->domain))
>> +
>> +static inline bool is_hvm_domain(const struct domain *d)
>> +{
>> +    return IS_ENABLED(CONFIG_HVM) ? d->guest_type == guest_type_hvm : false;
>> +}
>> +
>> +static inline bool is_hvm_vcpu(const struct vcpu *v)
>> +{
>> +    return is_hvm_domain(v->domain);
>> +}
>> +
> This should work too. I'm not too fuss whether is_hvm_* are macros or
> functions.

static inlines are superior to macros in a lot of ways.  If in doubt,
use a static inline (if you can.  we've got some header file tangles
which occasionally make it very hard to use static inlines).

>
>>  #define is_pinned_vcpu(v) ((v)->domain->is_pinned || \
>>                             cpumask_weight((v)->cpu_hard_affinity) == 1)
>>  #ifdef CONFIG_HAS_PASSTHROUGH
>>
>> seems to compile, and should DTRT including all appropriate parameter
>> evaluation.
> I will test if DCE works properly with static inline functions.

It does (/should).

~Andrew

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