Re: [RFC 4/6] capabilities: introduce console io as a domain capability

[Julien Grall <julien@xxxxxxx>]

Hi,

On 01/08/2023 21:20, Daniel P. Smith wrote:
> The field `is_console` suggests that the field represents a state of being or
> posession, not that it reflects the privilege to access the console. In this
> patch the field is renamed to capabilities to encapsulate the capabilities a
> domain has been granted. The first capability being the ability to read/write
> the Xen console.
>
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> ---
>   xen/arch/arm/domain_build.c |  4 +++-
>   xen/include/xen/sched.h     | 25 +++++++++++++++++++++++--
>   xen/include/xsm/dummy.h     |  2 +-
>   3 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 51b4daefe1..ad7432b029 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -4076,7 +4076,9 @@ void __init create_domUs(void)
>               panic("Error creating domain %s (rc = %ld)\n",
>                     dt_node_name(node), PTR_ERR(d));
>   
> -        d->is_console = true;
> +        if ( ! domain_set_cap(d, CAP_CONSOLE_IO) )
Coding style: We don't usually add a space after '!'.

> +            printk("failed setting console_io on %pd\n", d);
I find a bit odd that we would continue even if the cap cannot be set. 
Can you clarify?
> +
>           dt_device_set_used_by(node, d->domain_id);
>   
>           rc = construct_domU(d, node);
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index ec0f9baff6..b04fbe0565 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -472,8 +472,8 @@ struct domain
>   #define ROLE_HARDWARE_DOMAIN   (1U<<2)
>   #define ROLE_XENSTORE_DOMAIN   (1U<<3)
>       uint8_t          role;
> -    /* Can this guest access the Xen console? */
> -    bool             is_console;
> +#define CAP_CONSOLE_IO  (1U<<0)
Coding style: Space before and after <<.

> +    uint8_t          capabilities;
>       /* Is this guest being debugged by dom0? */
>       bool             debugger_attached;
>       /*
> @@ -1146,6 +1146,27 @@ static always_inline bool is_hvm_vcpu(const struct vcpu 
> *v)
>       return is_hvm_domain(v->domain);
>   }
>   
> +static always_inline bool domain_has_cap(
> +    const struct domain *d, uint8_t cap)
Coding style: We don't usually wrap the arguments this way. See 
domain_create() for an example.
> +{
> +    return d->capabilities & cap;
> +}
> +
> +static always_inline bool domain_set_cap(
> +    struct domain *d, uint8_t cap)
Same about the coding style here.

Also, do you expect the cap to be set only when the domain is created? 
If not, would you prevent potentially concurrent update to d->capabilities?

> +{
> +    switch ( cap )
> +    {
> +    case CAP_CONSOLE_IO:
> +        d->capabilities |= cap;
> +        break;
> +    default:
Is this meant to be reached? If not, maybe add ASSERT_UNREACHABLE()?

> +        return false;
> +    }
> +
> +    return domain_has_cap(d, cap);
> +} > +
>   static always_inline bool hap_enabled(const struct domain *d)
>   {
>       /* sanitise_domain_config() rejects HAP && !HVM */
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index 18f1ddd127..067ff1d111 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -268,7 +268,7 @@ static XSM_INLINE int cf_check xsm_console_io(
>       XSM_DEFAULT_ARG struct domain *d, int cmd)
>   {
>       XSM_ASSERT_ACTION(XSM_OTHER);
> -    if ( d->is_console )
> +    if ( domain_has_cap(d, CAP_CONSOLE_IO) )
>           return xsm_default_action(XSM_HOOK, d, NULL);
>   #ifdef CONFIG_VERBOSE_DEBUG
>       if ( cmd == CONSOLEIO_write )
Cheers,

--
Julien Grall