| Message ID | 20230801202006.20322-5-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Hyperlaunch domain roles and capabilities | expand |
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 > 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@apertussolutions.com> 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. > --- > 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 >
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@apertussolutions.com> > > 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 >>
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@apertussolutions.com> > --- > 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,
Hi Daniel, On 03/08/2023 16:41, Daniel P. Smith wrote: > On 8/1/23 21:01, Stefano Stabellini wrote: >> On Tue, 1 Aug 2023, Daniel P. Smith wrote: >>> 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@apertussolutions.com> >> >> 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. Thanks for the explanation. Just to confirm my understanding, what you are suggesting is that for a given role, a domain will at least have the matching capabilities (more could be granted). Is that correct? If so, this wouldn't this mean we can remove d->role and simply use d->capabilities? > > 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. At least from the explanation above, I think it would make sense to break down role to multiple capabilities. Cheers,
On 01.08.2023 22:20, Daniel P. Smith wrote: > --- 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; > +} What you implement here is effectively domain_has_any_cap(), which I don't think is of much use. At the very least you want to assert that cap is a power of two. But perhaps you rather want the caller to pass in a bit number, not a mask, such that it's obvious that only one thing can be checked at a time. > +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); > +} The "set" operation doesn't need to be an inline function, does it? Jan
On 8/3/23 17:24, Julien Grall wrote: > Hi Daniel, > > On 03/08/2023 16:41, Daniel P. Smith wrote: >> On 8/1/23 21:01, Stefano Stabellini wrote: >>> On Tue, 1 Aug 2023, Daniel P. Smith wrote: >>>> 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@apertussolutions.com> >>> >>> 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. > > Thanks for the explanation. Just to confirm my understanding, what you > are suggesting is that for a given role, a domain will at least have the > matching capabilities (more could be granted). Is that correct? > > If so, this wouldn't this mean we can remove d->role and simply use > d->capabilities? We could start out with CAP_CTRL and CAP_HW, but it is a little illogical. For instance, control domain has many capabilities, they just have never been fully broken out. XSM did some, but the focus there was just on system resources. Similar with the hardware domain. You are right that it would better deal with the limited number of bits currently available. >> >> 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. > > At least from the explanation above, I think it would make sense to > break down role to multiple capabilities. Would it be acceptable to do this incrementally over time as we are able to determine what needs to be broken out as a distinct capability?
On 8/3/23 17:03, Julien Grall wrote: > Hi, Greetings > 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@apertussolutions.com> >> --- >> 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 '!'. Ack. >> + 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? This is the construction of a domU, so the system is very much capable of coming up and reviewing the hypervisor messages from dom0 to discover the issue. I am hard pressed to believe the hypervisor should be panicked because the domU is not allowed to use the hypervisor's console. g>> + >> 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 <<. Ack. >> + 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. I was informed it was[1], also, please see next_domain_in_cpupool() amongst many others further below. [1] https://lists.xenproject.org/archives/html/xen-devel/2021-07/msg01133.html >> +{ >> + return d->capabilities & cap; >> +} >> + >> +static always_inline bool domain_set_cap( >> + struct domain *d, uint8_t cap) > > Same about the coding style here. Ditto. > 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? Currently the only means being devise to set this is via hyperlaunch domain creation. If a domctl op was added to be able to manipulate the caps, then yes a lock on the domain would be advised to block. With that said, if we switch over to CAP_CTRL/HW, then it might be good to grab a lock on the domain for the late hardware domain case. >> +{ >> + switch ( cap ) >> + { >> + case CAP_CONSOLE_IO: >> + d->capabilities |= cap; >> + break; >> + default: > > Is this meant to be reached? If not, maybe add ASSERT_UNREACHABLE()? Yah, that would be a good idea. >> + 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 ) v/r, dps
On 8/8/23 11:24, Jan Beulich wrote: > On 01.08.2023 22:20, Daniel P. Smith wrote: >> --- 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; >> +} > > What you implement here is effectively domain_has_any_cap(), which I > don't think is of much use. At the very least you want to assert that > cap is a power of two. But perhaps you rather want the caller to pass > in a bit number, not a mask, such that it's obvious that only one > thing can be checked at a time. I did this thinking I was going to save keystrokes and encapsulate the check, but I just double checked and it would have only saved one. I will drop this and put the explicit bit checks at all the check sites. >> +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); >> +} > > The "set" operation doesn't need to be an inline function, does it? I guess not, I can move into common/domain.c. v/r, dps
Hi Daniel, On 08/08/2023 23:31, Daniel P. Smith wrote: > > > On 8/3/23 17:24, Julien Grall wrote: >> Hi Daniel, >> >> On 03/08/2023 16:41, Daniel P. Smith wrote: >>> On 8/1/23 21:01, Stefano Stabellini wrote: >>>> On Tue, 1 Aug 2023, Daniel P. Smith wrote: >>>>> 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@apertussolutions.com> >>>> >>>> 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. >> >> Thanks for the explanation. Just to confirm my understanding, what you >> are suggesting is that for a given role, a domain will at least have >> the matching capabilities (more could be granted). Is that correct? >> >> If so, this wouldn't this mean we can remove d->role and simply use >> d->capabilities? > > We could start out with CAP_CTRL and CAP_HW, but it is a little > illogical. For instance, control domain has many capabilities, they just > have never been fully broken out. XSM did some, but the focus there was > just on system resources. Similar with the hardware domain. You are > right that it would better deal with the limited number of bits > currently available. > >>> >>> 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. >> >> At least from the explanation above, I think it would make sense to >> break down role to multiple capabilities. > > Would it be acceptable to do this incrementally over time as we are able > to determine what needs to be broken out as a distinct capability? I would be fine with that. Note that some care will be needed for the Device-Tree to either version the capabilities or at least not break boot when using an old DT on a new Xen. Cheers,
Hi Daniel, On 08/08/2023 23:49, Daniel P. Smith wrote: > On 8/3/23 17:03, Julien Grall wrote: >> 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@apertussolutions.com> >>> --- >>> 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 '!'. > > Ack. > >>> + 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? > > This is the construction of a domU, so the system is very much capable > of coming up and reviewing the hypervisor messages from dom0 to discover > the issue. I am hard pressed to believe the hypervisor should be > panicked because the domU is not allowed to use the hypervisor's console. I understand the system may be able to boot. However, the problem is that it may take a while to discover that the console is not working properly (the more if you only use it for error logging). So on Arm, we have so always decided to fail early rather than late in order to help debugging. So I would rather not change the behavior even if this is "just" for the console. If you expect the console to be disabled, then we should provide a property in the Device-Tree to select/deselect. It should not be hidden. > > g>> + >>> 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 <<. > > Ack. > >>> + 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. > > I was informed it was[1], also, please see next_domain_in_cpupool() > amongst many others further below. The unwritten coding style strike again... I am not sure where the agreement comes from. At least on Arm, we have been using the first version in that thread and if it can't be wrapped to 80 characters, then move the "static inline void " on its own line. The advantage with the Arm approach is that parameters are always indented the same way. Anyway, the way you wrote is not my personal preference but I am also not up to bikeshed too much on it. Hopefully this sort of style discussion will be resolved with clang-format. [...] > >> 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? > > Currently the only means being devise to set this is via hyperlaunch > domain creation. If a domctl op was added to be able to manipulate the > caps, then yes a lock on the domain would be advised to block. Loking at patch #6, you are using domctl there. > With that > said, if we switch over to CAP_CTRL/HW, then it might be good to grab a > lock on the domain for the late hardware domain case. Are you planning to clear the caps? If not, then using set_bit() and test_bit() should be enough. Cheers,
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) ) + 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 )
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@apertussolutions.com> --- 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(-)