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

[Julien Grall <julien@xxxxxxx>]

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

--
Julien Grall