Re: [PATCH 1/2] xsm: add ability to elevate a domain to privileged

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.04.2022 10:08, Roger Pau Monné wrote:
> On Fri, Apr 01, 2022 at 06:52:46PM +0100, Julien Grall wrote:
>> Hi,
>>
>> On 31/03/2022 13:36, Roger Pau Monné wrote:
>>> On Wed, Mar 30, 2022 at 07:05:48PM -0400, Daniel P. Smith wrote:
>>>> There are now instances where internal hypervisor logic needs to make 
>>>> resource
>>>> allocation calls that are protected by XSM checks. The internal hypervisor 
>>>> logic
>>>> is represented a number of system domains which by designed are 
>>>> represented by
>>>> non-privileged struct domain instances. To enable these logic blocks to
>>>> function correctly but in a controlled manner, this commit introduces a 
>>>> pair
>>>> of privilege escalation and demotion functions that will make a system 
>>>> domain
>>>> privileged and then remove that privilege.
>>>>
>>>> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>>>> ---
>>>>   xen/include/xsm/xsm.h | 22 ++++++++++++++++++++++
>>>
>>> I'm not sure this needs to be in xsm code, AFAICT it could live in a
>>> more generic file.
>>>
>>>>   1 file changed, 22 insertions(+)
>>>>
>>>> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
>>>> index e22d6160b5..157e57151e 100644
>>>> --- a/xen/include/xsm/xsm.h
>>>> +++ b/xen/include/xsm/xsm.h
>>>> @@ -189,6 +189,28 @@ struct xsm_operations {
>>>>   #endif
>>>>   };
>>>> +static always_inline int xsm_elevate_priv(struct domain *d)
>>>
>>> I don't think it needs to be always_inline, using just inline would be
>>> fine IMO.
>>>
>>> Also this needs to be __init.
>>
>> Hmmm.... I thought adding __init on function defined in header was
>> pointless. In particular, if the compiler decides to inline it.
> 
> Indeed, I didn't realize, thanks for pointing this out.

The question isn't header or not, but declaration or definition.
Attributes like this one are meaningless on declarations (at least
on all the arches we care about; there may be subtleties), but
meaningful for definitions. Iirc even with always_inline the
compiler may find reasons why a function cannot be inlined, and
hence the intended section should be specified. Plus such an
annotation serves a documentation purpose.

Jan