Re: [PATCH 16/16] x86/P2M: the majority for struct p2m_domain's fields are HVM-only

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.02.2022 17:07, Jan Beulich wrote:
> On 14.02.2022 16:51, George Dunlap wrote:
>>> On Jul 5, 2021, at 5:15 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>
>>> ..., as are the majority of the locks involved. Conditionalize things
>>> accordingly.
>>>
>>> Also adjust the ioreq field's indentation at this occasion.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> 
> Thanks.
> 
>> With one question…
>>
>>> @@ -905,10 +917,10 @@ int p2m_altp2m_propagate_change(struct d
>>> /* Set a specific p2m view visibility */
>>> int p2m_set_altp2m_view_visibility(struct domain *d, unsigned int idx,
>>>                                    uint8_t visible);
>>> -#else
>>> +#else /* CONFIG_HVM */
>>> struct p2m_domain *p2m_get_altp2m(struct vcpu *v);
>>> static inline void p2m_altp2m_check(struct vcpu *v, uint16_t idx) {}
>>> -#endif
>>> +#endif /* CONFIG_HVM */
>>
>> This is relatively minor, but what’s the normal for how to label #else 
>> macros here?  Wouldn’t you normally see “#endif /* CONFIG_HVM */“ and think 
>> that the immediately preceding lines are compiled only if CONFIG_HVM is 
>> defined?  I.e., would this be more accurate to write “!CONFIG_HVM” here?
>>
>> I realize in this case it’s not a big deal since the #else is just three 
>> lines above it, but since you took the time to add the comment in there, it 
>> seems like it’s worth the time to have a quick think about whether that’s 
>> the right thing to do.
> 
> Hmm, yes, let me make this !CONFIG_HVM. I think we're not really
> consistent with this, but I agree it's more natural like you say.

Coming to write a similar construct elsewhere, I've realized this is
odd. Looking through include/asm/, the model generally used is

#ifdef CONFIG_xyz
#else /* !CONFIG_xyz */
#endif /* CONFIG_xyz */

That's what I'll switch to here then as well.

Jan