Re: [PATCH 3/3] xen/vcpu: Rework sanity checks in vcpu_create()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 06/09/18 21:07, Jason Andryuk wrote:
> On Thu, Sep 6, 2018 at 3:28 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> Poisoning idle_vcpu[0] with the sanity debug value isn't actually a clever
>> idea, because it passes a NULL pointer check but isn't a usable vcpu.  It is
>> also the reason for the (!is_idle_domain(d) || vcpu_id) part of the existing
>> sanity BUG_ON().
>>
>> Now that d->max_vcpus is appropriately set up before vcpu_create() is called,
>> we can properly range check the requested vcpu_id.  Drop the BUG_ON() and
>> replace it with code which is runtime safe but non-fatal.
>>
>> While v0 must be the first allocated vcpu for for_each_vcpu() to work, it
>> isn't a requirement for the threading the vcpu into the linked list, so update
>> the threading code to be more generic, and add a comment explaining why we
>> need to search for prev_id.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> ---
>>  xen/arch/arm/setup.c |  1 -
>>  xen/arch/x86/setup.c |  1 -
>>  xen/common/domain.c  | 32 ++++++++++++++++++++++++++++----
>>  3 files changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 01aaaab..d06ac40 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -704,7 +704,6 @@ void __init start_xen(unsigned long boot_phys_offset,
>>      set_processor_id(0); /* needed early, for smp_processor_id() */
>>
>>      set_current((struct vcpu *)0xfffff000); /* debug sanity */
>> -    idle_vcpu[0] = current;
>>
>>      setup_virtual_regions(NULL, NULL);
>>      /* Initialize traps early allow us to get backtrace when an error occurred */
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index a2f22a1..5e1e8ae 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -691,7 +691,6 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>
>>      set_processor_id(0);
>>      set_current(INVALID_VCPU); /* debug sanity. */
>> -    idle_vcpu[0] = current;
> xen/arch/x86/tboot.c:tboot_shutdown() has:
>     if ( idle_vcpu[0] != INVALID_VCPU )
>         write_ptbase(idle_vcpu[0]);
>
> With your change, this should be update to check for non-NULL.

Oh - very good catch.  Thanks.

Looking at the code, ideally it would change to
write_cr3(idle_pg_table), but that isn't going to include the additional
CR4 changes.  Leaving this in this form (with a NULL) check is probably
best.

~Andrew

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