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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

> In any case, I think it would be good to check that the system_state <
> SYS_state_active (could potentially be an ASSERT()) to prevent any misuse.

My preference would be to make those non-inline then I think, we could
place them in common/domain.c maybe? There's no performance reason to
have those helpers as inline.

Another option would be what Jason suggested about creating the idle
domain as privileged and then dropping the privileges before starting
any domains (ie: before setting the system as ACTIVE).

This expands the duration the idle domain context is marked as
privileged, but OTOH we don't need to add a hook to set
is_privileged.

Thanks, Roger.