From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH RFC 5/9] x86/traps: Functional prep work Date: Thu, 15 May 2014 13:42:34 +0100 Message-ID: <5374B63A.30305@citrix.com> References: <1400147299-31772-1-git-send-email-andrew.cooper3@citrix.com> <1400147299-31772-6-git-send-email-andrew.cooper3@citrix.com> <5374B4B202000078000128F6@mail.emea.novell.com> <53749AD0.2010608@citrix.com> <5374CBFE0200007800012A22@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5374CBFE0200007800012A22@mail.emea.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: TimDeegan , Keir Fraser , Xen-devel List-Id: xen-devel@lists.xenproject.org On 15/05/14 13:15, Jan Beulich wrote: >>>> On 15.05.14 at 12:45, wrote: >> On 15/05/14 11:36, Jan Beulich wrote: >>>>>> On 15.05.14 at 11:48, wrote: >>>> --- a/xen/arch/x86/setup.c >>>> +++ b/xen/arch/x86/setup.c >>>> @@ -558,6 +558,12 @@ void __init noreturn __start_xen(unsigned long mbi_p) >>>> .stop_bits = 1 >>>> }; >>>> >>>> + set_processor_id(0); >>>> + set_current((struct vcpu *)0xfffff000); /* debug sanity */ >>>> + this_cpu(curr_vcpu) = idle_vcpu[0] = current; >>> The this_cpu() part wasn't there in the original code - is that really >>> needed, and ... >> I was attempting to go for similarity between __start_xen and >> start_secondary, which reminds me I need a further fix regarding cr4, >> which still loads CR4.MCE on APs before having a TRAP_machine_check >> handler available. >> >>>> + >>>> + sort_exception_tables(); >>>> + >>>> percpu_init_areas(); >>> ... is that really safe/meaningful before this function got called? >> There is no specific relationship between sort_exception_tables() and >> percpu_init_areas(), both of which are tweaking well defined state >> inside the .data section. >> >> sort_excetpion_tables() is a prerequisite for getting extable fixups to >> work in the trap handlers, but as indicated, it would be nice to turn it >> into something more like "assert exception tables are sorted" and making >> the linker do the work. > The comment wasn't about sort_exception_tables(), but about the > (at least apparent) conflict of this_cpu() getting used before > percpu_init_areas(). > > Jan > Ah - I see what you mean. The BSP per_cpu_offset is 0, so the code as patched does work correctly. It would however become a latent bug if the implementation of per_cpu variables changed such that the BSP didn't use the copy of the per_cpu data in the .data section. I shall just drop the this_cpu() bit. Consistency with start_secondary is not worth this latent bug. ~Andrew