1. Patchew
  2. Xen
  3. [Xen-devel] [PATCH 0/6] x86/boot: Remove mappings at 0
  4. View patch

[Xen-devel] [PATCH 6/6] x86/boot: Drop INVALID_VCPU

Andrew Cooper posted 6 patches 6 years, 1 month ago
There is a newer version of this series
[Xen-devel] [PATCH 6/6] x86/boot: Drop INVALID_VCPU
Posted by Andrew Cooper 6 years, 1 month ago 
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(-)

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[];
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 6/6] x86/boot: Drop INVALID_VCPU
Posted by Jan Beulich 6 years, 1 month ago 
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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 6/6] x86/boot: Drop INVALID_VCPU
Posted by Andrew Cooper 6 years, 1 month ago 
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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 6/6] x86/boot: Drop INVALID_VCPU
Posted by Jan Beulich 6 years, 1 month ago 
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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.