[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/6] xen: use domid check in is_hardware_domain
On 03/05/2014 04:23 AM, Jan Beulich wrote: On 04.03.14 at 23:51, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:The hardware domain manages devices for PCI pass-through to driver domains or can act as a driver domain itself, depending on the desired degree of disaggregation. It is also the domain managing devices that do not support pass-through: PCI configuration space access, parsing the hardware ACPI tables and system power or machine check events. This is the only domain where is_hardware_domain() is true. The return of is_control_domain() is false for this domain."s/is/may be/" ? I had intended this sentence to describe the model, where the return is always false. However, I agree with the change to avoid confusion that the two is_*_domain() functions are exclusive. --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -1505,7 +1505,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next) } set_cpuid_faulting(is_pv_vcpu(next) && - (next->domain->domain_id != 0)); + !is_control_domain(next->domain));I can't see why the hardware domain (which can't be migrated) should be prevented from seeing the real CPUID values. It's less obvious whether a control domain should always see real values. Hence if you think the latter should be the case (as the change above shows), I think you should check for both cases here. Agreed, the hardware domain also needs to be checked for here. The reason the control domain is present is that it needs to see real CPUID values in order to set CPUID policy for guests based on the real hardware values. --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -738,7 +738,7 @@ void pv_cpuid(struct cpu_user_regs *regs) c = regs->ecx; d = regs->edx; - if ( current->domain->domain_id != 0 ) + if ( !is_control_domain(current->domain) )The same consideration applies here then, obviously.--- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -242,7 +242,7 @@ struct domain *domain_create( else if ( domcr_flags & DOMCRF_pvh ) d->guest_type = guest_type_pvh; - if ( domid == 0 ) + if ( is_hardware_domain(d) ) { d->is_pinned = opt_dom0_vcpus_pin; d->disable_migrate = 1; @@ -267,10 +267,10 @@ struct domain *domain_create( d->is_paused_by_controller = 1; atomic_inc(&d->pause_count); - if ( domid ) - d->nr_pirqs = nr_static_irqs + extra_domU_irqs; - else + if ( is_hardware_domain(d) ) d->nr_pirqs = nr_static_irqs + extra_dom0_irqs; + else + d->nr_pirqs = nr_static_irqs + extra_domU_irqs;I'd prefer the if/else cases to remain as they are - makes the patch smaller, and fits better with the (weak) model of using the if branch for the common case and the else one for the special one (outside of error handling of course). OK. I prefer to avoid the if (!foo) bar; else baz; construct where possible, but common case first is a good reason to use it. --- a/xen/common/xenoprof.c +++ b/xen/common/xenoprof.c @@ -603,7 +603,7 @@ static int xenoprof_op_init(XEN_GUEST_HANDLE_PARAM(void) arg) xenoprof_init.is_primary = ((xenoprof_primary_profiler == d) || - ((xenoprof_primary_profiler == NULL) && (d->domain_id == 0))); + ((xenoprof_primary_profiler == NULL) && is_control_domain(d)));Do you really consider profiling a control domain property? This is even more so questionable without knowing whether you checked that there are no issues with all of the sudden there perhaps being more than one domain eligible for becoming the primary profiler - the oprofile code isn't in that good a shape to be certain without explicit checking. Jan I don't directly consider profiling to be a control domain property, but I also don't consider it a hardware domain property, and it does need to be restricted in some way. I could make a separate patch changing the condition to use an XSM hook, only setting xenoprof_primary_profiler if the domain is allowed the XEN__PRIVPROFILE permission, but this still could cause multiple domains to be eligible. From my cursory examination when I made this change, the first domain to try becoming the primary profiler will succeed and be assigned to xenoprof_primary_profiler. Later domains will not be considered since the primary will already be set. One thing I had not considered that may be a problem: if the profiling domain is shut down without calling XENOPROF_shutdown, it will not be possible to use profiling this boot unless the struct domain* is reused. This may then become a security issue because an arbitrary domain is now the primary profiler, although the XSM policy should prevent any actions by a domain not permitted to use the profiling hypercalls. Using is_hardware_domain here avoids that problem (as the hardware domain may never shut down or be destroyed), so that may be the simplest solution until a better model of who is responsible for profiling is presented. -- Daniel De Graaf National Security Agency _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |