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

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 5/9/22 14:38, Julien Grall wrote:
> 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

Ack.

v/r,
dps