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

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

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

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.

 -George

Attachment: signature.asc
Description: Message signed with OpenPGP