Re: [PATCH v6 1/2] xsm: create idle domain privileged and demote after setup

[Julien Grall <julien@xxxxxxx>]

Hi Daniel,

On 03/05/2022 12:17, Daniel P. Smith wrote:
> There are new capabilities, dom0less and hyperlaunch, that introduce internal
> hypervisor logic which needs to make resource allocation calls that are
> protected by XSM access checks. This creates an issue as a subset of the
> hypervisor code is executed under a system domain, the idle domain, that is
> represented by a per-CPU non-privileged struct domain. To enable these new
> capabilities to function correctly but in a controlled manner, this commit
> changes the idle system domain to be created as a privileged domain under the
> default policy and demoted before transitioning to running. A new XSM hook,
> xsm_set_system_active(), is introduced to allow each XSM policy type to demote
> the idle domain appropriately for that policy type. In the case of SILO, it
> inherits the default policy's hook for xsm_set_system_active().
>
> For flask a stub is added to ensure that flask policy system will function
> correctly with this patch until flask is extended with support for starting the
> idle domain privileged and properly demoting it on the call to
> xsm_set_system_active().
>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> ---
>   xen/arch/arm/setup.c    |  4 ++++
>   xen/arch/x86/setup.c    |  5 +++++
>   xen/common/sched/core.c |  7 ++++++-
>   xen/include/xsm/dummy.h | 17 +++++++++++++++++
>   xen/include/xsm/xsm.h   |  6 ++++++
>   xen/xsm/dummy.c         |  1 +
>   xen/xsm/flask/hooks.c   | 23 +++++++++++++++++++++++
>   7 files changed, 62 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index d5d0792ed4..39a654926d 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -1048,6 +1048,10 @@ void __init start_xen(unsigned long boot_phys_offset,
>       /* Hide UART from DOM0 if we're using it */
>       serial_endboot();
>   
> +    if ( (rc = xsm_set_system_active()) != 0 )
> +        panic("xsm(err=%d): "
> +              "unable to set hypervisor to SYSTEM_ACTIVE privilege\n", rc);
We usually don't split error message over multiple lines (even if they 
are over 80 characters).
> +
>       system_state = SYS_STATE_active;
>   
>       for_each_domain( d )
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 6f20e17892..36a60ce884 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -620,6 +620,11 @@ static void noreturn init_done(void)
>   {
>       void *va;
>       unsigned long start, end;
> +    int err;
> +
> +    if ( (err = xsm_set_system_active()) != 0 )
> +        panic("xsm(err=%d): "
> +              "unable to set hypervisor to SYSTEM_ACTIVE privilege\n", err);
Same here.

Other than the two remarks above and Luca's one:

Acked-by: Julien Grall <jgrall@xxxxxxxxxx> # arm

Cheers,

--
Julien Grall