Re: [Xen-devel] [PATCH RFC 2/9] x86/traps: Make panic and reboot paths safe during early boot

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.05.14 at 11:48, <andrew.cooper3@xxxxxxxxxx> wrote:
> Make use of SYS_STATE_smp_boot to help machine_{halt,restart}() know if/when
> it is safe to enable interrupts and access the local apic to send IPIs.
> Before system_state == SYS_STATE_smp_boot, we can be certain that only the BSP
> is running.

Hmm, tying SMP boot and IRQ enabling together seems a little
problematic, even if on x86 the former happens soon after the latter
right now. Perhaps these ought to be distinct states?

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5246,7 +5246,7 @@ static l3_pgentry_t *virt_to_xen_l3e(unsigned long v)
>      pl4e = &idle_pg_table[l4_table_offset(v)];
>      if ( !(l4e_get_flags(*pl4e) & _PAGE_PRESENT) )
>      {
> -        bool_t locking = system_state > SYS_STATE_boot;
> +        bool_t locking = system_state >= SYS_STATE_active;

Did you just mechanically adjust occurrences like this one, to (as the
description says) have their semantics remain identical? I ask because
it would seem to me that here you'd likely better change the semantics
by keeping the code unchanged.

> --- a/xen/common/symbols.c
> +++ b/xen/common/symbols.c
> @@ -96,7 +96,7 @@ static unsigned int get_symbol_offset(unsigned long pos)
>  bool_t is_active_kernel_text(unsigned long addr)
>  {
>      return (is_kernel_text(addr) ||
> -            (system_state == SYS_STATE_boot && is_kernel_inittext(addr)));
> +            (system_state < SYS_STATE_active && is_kernel_inittext(addr)));

And here, contrary to the description, you actually do a semantic
(but correct!) change.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel