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 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.

Jan