| Message ID | 20170908065634.5420-7-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 08.09.17 at 08:56, <jgross@suse.com> wrote: > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -3667,6 +3667,32 @@ void grant_table_init_vcpu(struct vcpu *v) > v->maptrack_tail = MAPTRACK_TAIL; > } > > +int grant_table_set_limits(struct domain *d, unsigned int grant_frames, > + unsigned int maptrack_frames) > +{ > + struct grant_table *gt = d->grant_table; > + int ret = -EBUSY; > + > + if ( !gt ) > + return -EEXIST; How does EEXIST represent the error condition? > + grant_write_lock(gt); > + > + if ( gt->nr_grant_frames ) > + goto unlock; I think you can do without goto here with no risk of lowered readability. > + ret = 0; > + if ( grant_frames ) > + gt->max_grant_frames = grant_frames; > + if ( maptrack_frames ) > + gt->max_maptrack_frames = maptrack_frames; Together with what I have said regarding making the invocation of this domctl mandatory, I think these two shouldn't be conditional. In particular for maptrack I also don't see why a domain couldn't do with a limit of zero, as long as it's not serving as a backend for another guest. > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -1163,6 +1163,13 @@ struct xen_domctl_psr_cat_op { > typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t; > DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t); > > +struct xen_domctl_set_gnttab_limits { > + uint32_t grant_frames; /* IN: if 0, dont change */ > + uint32_t maptrack_frames; /* IN: if 0, dont change */ > +}; > +typedef struct xen_domctl_set_gnttab_limits xen_domctl_set_gnttab_limits_t; > +DEFINE_XEN_GUEST_HANDLE(xen_domctl_set_gnttab_limits_t); In another context I had already recently requested to stop the bad habit of adding typedef and handle for all domctl-s. I don't see what they're needed for, they just clutter the name space. Jan
On 08/09/17 17:55, Jan Beulich wrote: >>>> On 08.09.17 at 08:56, <jgross@suse.com> wrote: >> --- a/xen/common/grant_table.c >> +++ b/xen/common/grant_table.c >> @@ -3667,6 +3667,32 @@ void grant_table_init_vcpu(struct vcpu *v) >> v->maptrack_tail = MAPTRACK_TAIL; >> } >> >> +int grant_table_set_limits(struct domain *d, unsigned int grant_frames, >> + unsigned int maptrack_frames) >> +{ >> + struct grant_table *gt = d->grant_table; >> + int ret = -EBUSY; >> + >> + if ( !gt ) >> + return -EEXIST; > > How does EEXIST represent the error condition? Yes, this was a bad choice. What about ENOENT? > >> + grant_write_lock(gt); >> + >> + if ( gt->nr_grant_frames ) >> + goto unlock; > > I think you can do without goto here with no risk of lowered > readability. Okay. > >> + ret = 0; >> + if ( grant_frames ) >> + gt->max_grant_frames = grant_frames; >> + if ( maptrack_frames ) >> + gt->max_maptrack_frames = maptrack_frames; > > Together with what I have said regarding making the invocation > of this domctl mandatory, I think these two shouldn't be conditional. > In particular for maptrack I also don't see why a domain couldn't > do with a limit of zero, as long as it's not serving as a backend for > another guest. Okay, then I'd need to specify a "take hypervisor default" value (e.g. ~0) in order to handle the case where no value was specified in the domain's config file. The question would be then: do we want to set maptrack to 0 as default and require it to be specified for backend domains (driver domains, stubdoms)? >> --- a/xen/include/public/domctl.h >> +++ b/xen/include/public/domctl.h >> @@ -1163,6 +1163,13 @@ struct xen_domctl_psr_cat_op { >> typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t; >> DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t); >> >> +struct xen_domctl_set_gnttab_limits { >> + uint32_t grant_frames; /* IN: if 0, dont change */ >> + uint32_t maptrack_frames; /* IN: if 0, dont change */ >> +}; >> +typedef struct xen_domctl_set_gnttab_limits xen_domctl_set_gnttab_limits_t; >> +DEFINE_XEN_GUEST_HANDLE(xen_domctl_set_gnttab_limits_t); > > In another context I had already recently requested to stop the > bad habit of adding typedef and handle for all domctl-s. I don't > see what they're needed for, they just clutter the name space. I'm happy to remove it from the patch. Juergen
>>> On 11.09.17 at 12:48, <jgross@suse.com> wrote: > On 08/09/17 17:55, Jan Beulich wrote: >>>>> On 08.09.17 at 08:56, <jgross@suse.com> wrote: >>> --- a/xen/common/grant_table.c >>> +++ b/xen/common/grant_table.c >>> @@ -3667,6 +3667,32 @@ void grant_table_init_vcpu(struct vcpu *v) >>> v->maptrack_tail = MAPTRACK_TAIL; >>> } >>> >>> +int grant_table_set_limits(struct domain *d, unsigned int grant_frames, >>> + unsigned int maptrack_frames) >>> +{ >>> + struct grant_table *gt = d->grant_table; >>> + int ret = -EBUSY; >>> + >>> + if ( !gt ) >>> + return -EEXIST; >> >> How does EEXIST represent the error condition? > > Yes, this was a bad choice. What about ENOENT? Fine with me. Or ENODEV. >>> + ret = 0; >>> + if ( grant_frames ) >>> + gt->max_grant_frames = grant_frames; >>> + if ( maptrack_frames ) >>> + gt->max_maptrack_frames = maptrack_frames; >> >> Together with what I have said regarding making the invocation >> of this domctl mandatory, I think these two shouldn't be conditional. >> In particular for maptrack I also don't see why a domain couldn't >> do with a limit of zero, as long as it's not serving as a backend for >> another guest. > > Okay, then I'd need to specify a "take hypervisor default" value (e.g. > ~0) in order to handle the case where no value was specified in the > domain's config file. Well, part of the point I was trying to make in earlier replies on other patches of this series is that I think the lack of a guest config file setting should not invoke a _hypervisor_ default. Instead, the tool stack should establish a sensible one. > The question would be then: do we want to set maptrack to 0 as default > and require it to be specified for backend domains (driver domains, > stubdoms)? I think so, yes. Question is whether there's a way for the tool stack to easily recognize a driver domain when it's being created. Jan
On 11/09/17 13:14, Jan Beulich wrote: >>>> On 11.09.17 at 12:48, <jgross@suse.com> wrote: >> On 08/09/17 17:55, Jan Beulich wrote: >>>>>> On 08.09.17 at 08:56, <jgross@suse.com> wrote: >>>> --- a/xen/common/grant_table.c >>>> +++ b/xen/common/grant_table.c >>>> @@ -3667,6 +3667,32 @@ void grant_table_init_vcpu(struct vcpu *v) >>>> v->maptrack_tail = MAPTRACK_TAIL; >>>> } >>>> >>>> +int grant_table_set_limits(struct domain *d, unsigned int grant_frames, >>>> + unsigned int maptrack_frames) >>>> +{ >>>> + struct grant_table *gt = d->grant_table; >>>> + int ret = -EBUSY; >>>> + >>>> + if ( !gt ) >>>> + return -EEXIST; >>> >>> How does EEXIST represent the error condition? >> >> Yes, this was a bad choice. What about ENOENT? > > Fine with me. Or ENODEV. > >>>> + ret = 0; >>>> + if ( grant_frames ) >>>> + gt->max_grant_frames = grant_frames; >>>> + if ( maptrack_frames ) >>>> + gt->max_maptrack_frames = maptrack_frames; >>> >>> Together with what I have said regarding making the invocation >>> of this domctl mandatory, I think these two shouldn't be conditional. >>> In particular for maptrack I also don't see why a domain couldn't >>> do with a limit of zero, as long as it's not serving as a backend for >>> another guest. >> >> Okay, then I'd need to specify a "take hypervisor default" value (e.g. >> ~0) in order to handle the case where no value was specified in the >> domain's config file. > > Well, part of the point I was trying to make in earlier replies on > other patches of this series is that I think the lack of a guest > config file setting should not invoke a _hypervisor_ default. > Instead, the tool stack should establish a sensible one. Okay, I can add this to the series. > >> The question would be then: do we want to set maptrack to 0 as default >> and require it to be specified for backend domains (driver domains, >> stubdoms)? > > I think so, yes. Question is whether there's a way for the tool stack > to easily recognize a driver domain when it's being created. I could think of various mechanisms for driver domains: 1. Add an explicit config item (I guess this could be utilized for other cases like XSM, too). 2. Do some guess work, e.g. any domain with PCI-passthrough configured could be regarded as a potential driver domain. 3. Just require the max_maptrack_frames= config item. Starting with 3. is the easiest solution for now, it can be switched to 1. or 2. later. Juergen
diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te index 338caaf41e..1643b400f0 100644 --- a/tools/flask/policy/modules/dom0.te +++ b/tools/flask/policy/modules/dom0.te @@ -39,7 +39,7 @@ allow dom0_t dom0_t:domain { }; allow dom0_t dom0_t:domain2 { set_cpuid gettsc settsc setscheduler set_max_evtchn set_vnumainfo - get_vnumainfo psr_cmt_op psr_cat_op + get_vnumainfo psr_cmt_op psr_cat_op set_gnttab_limits }; allow dom0_t dom0_t:resource { add remove }; diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if index 912640002e..55437496f6 100644 --- a/tools/flask/policy/modules/xen.if +++ b/tools/flask/policy/modules/xen.if @@ -52,7 +52,7 @@ define(`create_domain_common', ` settime setdomainhandle getvcpucontext set_misc_info }; allow $1 $2:domain2 { set_cpuid settsc setscheduler setclaim set_max_evtchn set_vnumainfo get_vnumainfo cacheflush - psr_cmt_op psr_cat_op soft_reset }; + psr_cmt_op psr_cat_op soft_reset set_gnttab_limits }; allow $1 $2:security check_context; allow $1 $2:shadow enable; allow $1 $2:mmu { map_read map_write adjust memorymap physmap pinpage mmuext_op updatemp }; diff --git a/xen/common/domctl.c b/xen/common/domctl.c index 42658e5744..58381f8fe9 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -14,6 +14,7 @@ #include <xen/sched-if.h> #include <xen/domain.h> #include <xen/event.h> +#include <xen/grant_table.h> #include <xen/domain_page.h> #include <xen/trace.h> #include <xen/console.h> @@ -1149,6 +1150,11 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) copyback = 1; break; + case XEN_DOMCTL_set_gnttab_limits: + ret = grant_table_set_limits(d, op->u.set_gnttab_limits.grant_frames, + op->u.set_gnttab_limits.maptrack_frames); + break; + default: ret = arch_do_domctl(op, d, u_domctl); break; diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index c00119f2fe..83f1a9dd34 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -3667,6 +3667,32 @@ void grant_table_init_vcpu(struct vcpu *v) v->maptrack_tail = MAPTRACK_TAIL; } +int grant_table_set_limits(struct domain *d, unsigned int grant_frames, + unsigned int maptrack_frames) +{ + struct grant_table *gt = d->grant_table; + int ret = -EBUSY; + + if ( !gt ) + return -EEXIST; + + grant_write_lock(gt); + + if ( gt->nr_grant_frames ) + goto unlock; + + ret = 0; + if ( grant_frames ) + gt->max_grant_frames = grant_frames; + if ( maptrack_frames ) + gt->max_maptrack_frames = maptrack_frames; + + unlock: + grant_write_unlock(gt); + + return ret; +} + #ifdef CONFIG_HAS_MEM_SHARING int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref, gfn_t *gfn, uint16_t *status) diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 50ff58f5b9..f7e3509c27 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -1163,6 +1163,13 @@ struct xen_domctl_psr_cat_op { typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t; DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t); +struct xen_domctl_set_gnttab_limits { + uint32_t grant_frames; /* IN: if 0, dont change */ + uint32_t maptrack_frames; /* IN: if 0, dont change */ +}; +typedef struct xen_domctl_set_gnttab_limits xen_domctl_set_gnttab_limits_t; +DEFINE_XEN_GUEST_HANDLE(xen_domctl_set_gnttab_limits_t); + struct xen_domctl { uint32_t cmd; #define XEN_DOMCTL_createdomain 1 @@ -1240,6 +1247,7 @@ struct xen_domctl { #define XEN_DOMCTL_monitor_op 77 #define XEN_DOMCTL_psr_cat_op 78 #define XEN_DOMCTL_soft_reset 79 +#define XEN_DOMCTL_set_gnttab_limits 80 #define XEN_DOMCTL_gdbsx_guestmemio 1000 #define XEN_DOMCTL_gdbsx_pausevcpu 1001 #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 @@ -1302,6 +1310,7 @@ struct xen_domctl { struct xen_domctl_psr_cmt_op psr_cmt_op; struct xen_domctl_monitor_op monitor_op; struct xen_domctl_psr_cat_op psr_cat_op; + struct xen_domctl_set_gnttab_limits set_gnttab_limits; uint8_t pad[128]; } u; }; diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h index 84a8d61616..dd9aa3b9ee 100644 --- a/xen/include/xen/grant_table.h +++ b/xen/include/xen/grant_table.h @@ -40,6 +40,8 @@ int grant_table_init( void grant_table_destroy( struct domain *d); void grant_table_init_vcpu(struct vcpu *v); +int grant_table_set_limits(struct domain *d, unsigned int grant_frames, + unsigned int maptrack_frames); /* * Check if domain has active grants and log first 10 of them. diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index 56dc5b0ab9..7b005af834 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -749,6 +749,9 @@ static int flask_domctl(struct domain *d, int cmd) case XEN_DOMCTL_soft_reset: return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SOFT_RESET); + case XEN_DOMCTL_set_gnttab_limits: + return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SET_GNTTAB_LIMITS); + default: return avc_unknown_permission("domctl", cmd); } diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors index da9f3dfb2e..3a2d863b8f 100644 --- a/xen/xsm/flask/policy/access_vectors +++ b/xen/xsm/flask/policy/access_vectors @@ -248,6 +248,8 @@ class domain2 mem_sharing # XEN_DOMCTL_psr_cat_op psr_cat_op +# XEN_DOMCTL_set_gnttab_limits + set_gnttab_limits } # Similar to class domain, but primarily contains domctls related to HVM domains