Re: [XEN PATCH v3 04/16] x86: introduce CONFIG_ALTP2M Kconfig option

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.06.2024 12:48, Sergiy Kibrik wrote:
> 07.06.24 10:47, Jan Beulich:
>> On 03.06.2024 13:13, Sergiy Kibrik wrote:
>>> --- a/xen/arch/x86/include/asm/p2m.h
>>> +++ b/xen/arch/x86/include/asm/p2m.h
>>> @@ -577,10 +577,10 @@ static inline gfn_t mfn_to_gfn(const struct domain 
>>> *d, mfn_t mfn)
>>>           return _gfn(mfn_x(mfn));
>>>   }
>>>   
>>> -#ifdef CONFIG_HVM
>>>   #define AP2MGET_prepopulate true
>>>   #define AP2MGET_query false
>>>   
>>> +#ifdef CONFIG_ALTP2M
>>>   /*
>>>    * Looks up altp2m entry. If the entry is not found it looks up the entry 
>>> in
>>>    * hostp2m.
>>
>> In principle this #ifdef shouldn't need moving. It's just that the
>> three use sites need taking care of a little differently. E.g. ...
>>
>>> @@ -589,6 +589,15 @@ static inline gfn_t mfn_to_gfn(const struct domain *d, 
>>> mfn_t mfn)
>>>   int altp2m_get_effective_entry(struct p2m_domain *ap2m, gfn_t gfn, mfn_t 
>>> *mfn,
>>>                                  p2m_type_t *t, p2m_access_t *a,
>>>                                  bool prepopulate);
>>> +#else
>>> +static inline int altp2m_get_effective_entry(struct p2m_domain *ap2m,
>>> +                                             gfn_t gfn, mfn_t *mfn,
>>> +                                             p2m_type_t *t, p2m_access_t 
>>> *a,
>>> +                                             bool prepopulate)
>>> +{
>>> +    ASSERT_UNREACHABLE();
>>> +    return -EOPNOTSUPP;
>>> +}
>>
>> static inline int altp2m_get_effective_entry(struct p2m_domain *ap2m,
>>                                               gfn_t gfn, mfn_t *mfn,
>>                                               p2m_type_t *t, p2m_access_t *a)
>> {
>>      ASSERT_UNREACHABLE();
>>      return -EOPNOTSUPP;
>> }
>> #define altp2m_get_effective_entry(ap2m, gfn, mfn, t, a, prepopulate) \
>>          altp2m_get_effective_entry(ap2m, gfn, mfn, t, a)
>>
>> Misra doesn't like such shadowing, so the inline function may want
>> naming slightly differently, e.g. _ap2m_get_effective_entry().
> 
> 
> I can do that, sure.
> Though here I'm curious what benefits we're getting from little 
> complication of an indirect call to an empty stub -- is avoiding of 
> AP2MGET_* defines worth it?

First - how is an indirect call coming into the picture here? We aim at
avoiding indirect calls where possible, after all. Yet iirc all calls
to altp2m_get_effective_entry() are direct ones.

As to avoiding the (or in fact any such) #define-s - imo it's better to
not have items in the name space which can't validly be used, unless this
(elsewhere) helps to keep the code tidy overall. This way problems (e.g.
from misunderstandings or flaws elsewhere) can be detected early.

Jan