From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel De Graaf Subject: Re: [PATCH 8/9] xen/arch/x86: use is_hardware_domain instead of domid == 0 Date: Fri, 12 Apr 2013 10:24:59 -0400 Message-ID: <5168193B.20008@tycho.nsa.gov> References: <1365711220-17995-1-git-send-email-dgdegra@tycho.nsa.gov> <1365711220-17995-9-git-send-email-dgdegra@tycho.nsa.gov> <5167DCFE02000078000CCAB3@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5167DCFE02000078000CCAB3@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: keir@xen.org, Ian.Campbell@citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 04/12/2013 04:07 AM, Jan Beulich wrote: >>>> On 11.04.13 at 22:13, Daniel De Graaf wrote: >> --- a/xen/arch/x86/domain.c >> +++ b/xen/arch/x86/domain.c >> @@ -1571,7 +1571,7 @@ void context_switch(struct vcpu *prev, struct vcpu *next) >> } >> >> set_cpuid_faulting(!is_hvm_vcpu(next) && >> - (next->domain->domain_id != 0)); >> + !is_hardware_domain(next->domain)); > > This one is highly questionable. Following the comments on the > previous patch, I'd think is_control_domain() here is more > appropriate, but maybe it would really need to become the or > of both? The question really is which domains we specifically > don't want CPUID faulting for. My impression was that this would be the hardware domain since it is the one that would be parsing ACPI tables and similar things that need CPUID to work. The control domain would just be involved in creating and managing domains, which doesn't need anything out of CPUID that a normal PV domain would need. >> --- a/xen/arch/x86/hvm/i8254.c >> +++ b/xen/arch/x86/hvm/i8254.c >> @@ -541,7 +541,7 @@ int pv_pit_handler(int port, int data, int write) >> .data = data >> }; >> >> - if ( (current->domain->domain_id == 0) && dom0_pit_access(&ioreq) ) >> + if ( is_hardware_domain(current->domain) && dom0_pit_access(&ioreq) ) > > And this one is now becoming inconsistent in itself: You remove the > explicit 0 on the left side, but leave the "dom0" in place on the right > side. If we drop the association with domid == 0 being the special > one, then all uses of "dom0" would logically also need to go away. I didn't want to take on the mass function rename that this would imply. I had considered naming the function "is_dom0_domain" but this seemed less clear given that I was trying to isolate hardware accesses. >> --- a/xen/arch/x86/time.c >> +++ b/xen/arch/x86/time.c >> @@ -1928,7 +1928,7 @@ void tsc_set_info(struct domain *d, >> uint32_t tsc_mode, uint64_t elapsed_nsec, >> uint32_t gtsc_khz, uint32_t incarnation) >> { >> - if ( is_idle_domain(d) || (d->domain_id == 0) ) >> + if ( is_idle_domain(d) || is_hardware_domain(d) ) > > Just like with the CPUID faulting, it is unclear (without a proper > model described end established first) which domains we really > want to special case here. Hence, rather than papering over the > issue, I'd prefer keeping the explicit domid check in place until > such a model can get established. OK; my logic here was the same as for CPUID faulting. >> --- a/xen/arch/x86/traps.c >> +++ b/xen/arch/x86/traps.c >> @@ -737,7 +737,7 @@ static void pv_cpuid(struct cpu_user_regs *regs) >> c = regs->ecx; >> d = regs->edx; >> >> - if ( current->domain->domain_id != 0 ) >> + if ( !is_hardware_domain(current->domain) ) > > Again, if anything this is more likely is_control_domain() or the or > of both. > > Jan > > -- Daniel De Graaf National Security Agency