| Message ID | 20200106155423.9508-7-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/boot: Remove mappings at 0 | expand |
On 06.01.2020 16:54, Andrew Cooper wrote: > Now that NULL will fault at boot, there is no need for a special constant to > signify "current not set up yet". Mind making this "... no strong need ..."? The benefit of an easily recognizable value goes away, but I guess we'll be fine without. IOW I'm not meaning to object. > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -705,7 +705,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) > /* Critical region without IDT or TSS. Any fault is deadly! */ > > set_processor_id(0); > - set_current(INVALID_VCPU); /* debug sanity. */ > + set_current(NULL); /* debug sanity. */ > idle_vcpu[0] = current; Is any of this actually changing any value in memory? I.e. wouldn't it be better to delete all of this, or leave it in a comment for documentation purposes? (I'm willing to ack the patch as is, but I'd like this alternative to at least be considered.) Jan
On 07/01/2020 16:52, Jan Beulich wrote: > On 06.01.2020 16:54, Andrew Cooper wrote: >> Now that NULL will fault at boot, there is no need for a special constant to >> signify "current not set up yet". > Mind making this "... no strong need ..."? The benefit of an easily > recognizable value goes away, but I guess we'll be fine without. > IOW I'm not meaning to object. Fine. > >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -705,7 +705,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) >> /* Critical region without IDT or TSS. Any fault is deadly! */ >> >> set_processor_id(0); >> - set_current(INVALID_VCPU); /* debug sanity. */ >> + set_current(NULL); /* debug sanity. */ >> idle_vcpu[0] = current; > Is any of this actually changing any value in memory? Yes. Observe: /* Set up stack. */ lea STACK_SIZE + sym_esi(cpu0_stack), %esp twice in head.S, meaning that the top-of-stack block is junk at this point. Explicitly setting it to NULL here seems like a safer option than trusting that noone has actually used the stack yet. ~Andrew
On 07.01.2020 19:07, Andrew Cooper wrote: > On 07/01/2020 16:52, Jan Beulich wrote: >> On 06.01.2020 16:54, Andrew Cooper wrote: >>> --- a/xen/arch/x86/setup.c >>> +++ b/xen/arch/x86/setup.c >>> @@ -705,7 +705,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) >>> /* Critical region without IDT or TSS. Any fault is deadly! */ >>> >>> set_processor_id(0); >>> - set_current(INVALID_VCPU); /* debug sanity. */ >>> + set_current(NULL); /* debug sanity. */ >>> idle_vcpu[0] = current; >> Is any of this actually changing any value in memory? > > Yes. Observe: > > /* Set up stack. */ > lea STACK_SIZE + sym_esi(cpu0_stack), %esp > > twice in head.S, meaning that the top-of-stack block is junk at this point. Why junk, when we have char __section(".bss.stack_aligned") __aligned(STACK_SIZE) cpu0_stack[STACK_SIZE]; > Explicitly setting it to NULL here seems like a safer option than > trusting that noone has actually used the stack yet. The actual "stack" part of cpu0_stack[] may have been used already, but the top-of-stack block ought to be untouched, or else we have other problems. Anyway, I don't heavily mind writing several zeros over what is already zero here, it just seems pretty pointless (and increasingly so by you now writing yet another zero). Jan
diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c index c8cecc4976..0c572b04f2 100644 --- a/xen/arch/x86/cpu/mcheck/mce.c +++ b/xen/arch/x86/cpu/mcheck/mce.c @@ -260,7 +260,7 @@ static int mca_init_global(uint32_t flags, struct mcinfo_global *mig) &mig->mc_coreid, &mig->mc_core_threadid, &mig->mc_apicid, NULL, NULL, NULL); - if ( curr != INVALID_VCPU ) + if ( curr ) { mig->mc_domid = curr->domain->domain_id; mig->mc_vcpuid = curr->vcpu_id; diff --git a/xen/arch/x86/domain_page.c b/xen/arch/x86/domain_page.c index 4a07cfb18e..dd32712d2f 100644 --- a/xen/arch/x86/domain_page.c +++ b/xen/arch/x86/domain_page.c @@ -29,7 +29,7 @@ static inline struct vcpu *mapcache_current_vcpu(void) * When current isn't properly set up yet, this is equivalent to * running in an idle vCPU (callers must check for NULL). */ - if ( v == INVALID_VCPU ) + if ( !v ) return NULL; /* diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 452f5bdd37..a7ca2236f4 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -705,7 +705,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) /* Critical region without IDT or TSS. Any fault is deadly! */ set_processor_id(0); - set_current(INVALID_VCPU); /* debug sanity. */ + set_current(NULL); /* debug sanity. */ idle_vcpu[0] = current; init_shadow_spec_ctrl_state(); diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c index 3e828fe204..5020c4ad49 100644 --- a/xen/arch/x86/tboot.c +++ b/xen/arch/x86/tboot.c @@ -392,7 +392,7 @@ void tboot_shutdown(uint32_t shutdown_type) * During early boot, we can be called by panic before idle_vcpu[0] is * setup, but in that case we don't need to change page tables. */ - if ( idle_vcpu[0] != INVALID_VCPU ) + if ( idle_vcpu[0] ) write_ptbase(idle_vcpu[0]); ((void(*)(void))(unsigned long)g_tboot_shared->shutdown_entry)(); diff --git a/xen/include/asm-x86/setup.h b/xen/include/asm-x86/setup.h index 861d46d6ac..28257bc5c8 100644 --- a/xen/include/asm-x86/setup.h +++ b/xen/include/asm-x86/setup.h @@ -4,9 +4,6 @@ #include <xen/multiboot.h> #include <asm/numa.h> -/* vCPU pointer used prior to there being a valid one around */ -#define INVALID_VCPU ((struct vcpu *)0xccccccccccccc000UL) - extern const char __2M_text_start[], __2M_text_end[]; extern const char __2M_rodata_start[], __2M_rodata_end[]; extern char __2M_init_start[], __2M_init_end[];
Now that NULL will fault at boot, there is no need for a special constant to signify "current not set up yet". Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/cpu/mcheck/mce.c | 2 +- xen/arch/x86/domain_page.c | 2 +- xen/arch/x86/setup.c | 2 +- xen/arch/x86/tboot.c | 2 +- xen/include/asm-x86/setup.h | 3 --- 5 files changed, 4 insertions(+), 7 deletions(-)