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

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

On 8/1/23 21:01, Stefano Stabellini wrote:
> On Tue, 1 Aug 2023, 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
>    ^ possession
Thank you for the catch.

> > 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>
> Patch looks fine to me aside the two minor nits. I am not sure I
> understand 100% the difference between capabilities and roles but I am
> OK with the patch. I'd like to hear Julien's feedback on this as well.
This might be worth a section in the hypervisor-guide. As mentioned in 
the cover letter, this was originally proposed as being under XSM. A 
challenge I ran into is that, depending on your view, the 
`is_privileged` field and `hardware_domain` global were either abused as 
a function check and a non-resource privilege check or are just 
multifaceted variables. This is why the concept of the role was struck 
upon, it is more intuitive (for me at least) that have a role is 
something that imparts accesses (privilege checks) and dictates 
hypervisor behaviors when handling the domain (function checks). This 
then brings us to an access or behavior that may be inherent to some 
role(s) but may want to grant on an individually to a guest. A prime 
example of this is console_io, for which it is inherent that the 
hardware domain role will have access but may want to grant to a guest 
without granting it an entire role. This is why I provided for 
identifying these capabilities so that they may be assigned individually 
to a domain.
While the role/capability is a natural progression from how the 
hypervisor currently operates. Another approach that could be consider 
to deliver a similar experience would be to break down every access and 
function into a capability and then define the standard roles as a 
conglomeration of certain capabilities.
I am open to suggestions here.

> > ---
> >   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) )
> code style
>
>
> > +            printk("failed setting console_io on %pd\n", d);
> > +
> >           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)
> > +    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)
> > +{
> > +    return d->capabilities & cap;
> > +}
> > +
> > +static always_inline bool domain_set_cap(
> > +    struct domain *d, uint8_t cap)
> > +{
> > +    switch ( cap )
> > +    {
> > +    case CAP_CONSOLE_IO:
> > +        d->capabilities |= cap;
> > +        break;
> > +    default:
> > +        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 )
> > --
> > 2.20.1