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 12:53, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 15/05/14 11:19, Jan Beulich wrote:
>>>>> 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?
> 
> Which states would you suggest then?

Perhaps "IRQs enabled" and "SMP boot"?

>>> --- 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.
> 
> I attempted to change each of them such that SYS_STATE_boot and
> SYS_STATE_smp_boot acted the same, and that further insertions of new
> states wouldn't require changes quite this wide.

For the former, I think if the locking is okay at that point (which I
think it is) you should drop the change and just mention the semantic
change. For the latter, all I was after is that you make the patch
description match it implementation.

Jan


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