Re: [Xen-devel] [PATCH 2 of 2 V4] x86/kexec: Change NMI and MCE handling on kexec path

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.12.12 at 18:24, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> On 11/12/12 15:58, Jan Beulich wrote:
>>>>> On 11.12.12 at 16:34, Andrew Cooper<andrew.cooper3@xxxxxxxxxx>  wrote:
>>> -    /* Would it be better to replace the trap vector here? */
>>> -    set_nmi_callback(crash_nmi_callback);
>>> +
>>> +    /* Change NMI trap handlers.  Non-crashing pcpus get nmi_crash which
>>> +     * invokes do_nmi_crash (above), which cause them to write state and
>>> +     * fall into a loop.  The crashing pcpu gets the nop handler to
>>> +     * cause it to return to this function ASAP.
>>> +     */
>>> +    for ( i = 0; i<  nr_cpu_ids; ++i )
>>> +        if ( idt_tables[i] )
>>> +        {
>>> +
>>> +            if ( i == cpu )
>>> +            {
>>> +                /* Disable the interrupt stack tables for this MCE and
>>> +                 * NMI handler (shortly to become a nop) as there is a 1
>>> +                 * instruction race window where NMIs could be
>>> +                 * re-enabled and corrupt the exception frame, leaving
>>> +                 * us unable to continue on this crash path (which half
>>> +                 * defeats the point of using the nop handler in the
>>> +                 * first place).
>>> +                 *
>>> +                 * This update is safe from a security point of view, as
>>> +                 * this pcpu is never going to try to sysret back to a
>>> +                 * PV vcpu.
>>> +                 */
>> This comment appears to have become stale with the latest
>> changes.
> 
> Ok
> 
>>
>>> +                _set_gate_lower(&idt_tables[i][TRAP_nmi], 14, 0,&trap_nop);
>> No need for the extra&  on functions and arrays.
> 
> I was continuing the prevailing style from traps.c  Personally, I prefer 
> the & notation for function pointers, to be consistent with regular 
> pointers, even though I am aware that it is not strictly needed.  I am 
> not fussed if you wish to insist on one style, but we do have mixed 
> styles across the codebase.

I'll leave that to your preference then.

>>> +{
>>> +    ASSERT(gate->b == new->b);
>>> +    *(volatile unsigned long *)&gate->a = new->a;
>> volatile? And if so, why not volatilie-qualify the function parameter?
> 
> I was looking to avoid having the compiler inline this function and 
> decide that it can merge *gate_addr and idte together, resulting in 
> multiple writes to gate_addr.  Without the volatile, the compiler is 
> free to make this optimization, which puts us back with the racy case we 
> are trying to avoid.  The reason for avoiding the volatile function 
> parameter is so the assertion equality can be optimized where possible.

Optimizing ASSERT() expressions is pointless. If you really want
volatile here, put it on the parameter. Casts, as having some risk
associated with them, ought to be avoided wherever possible.

>>> +} while (0)
>>> +
>>> +/* Update the lower half handler of an IDT Entry, without changing any
>>> + * other configuration. */
>>> +static inline void _update_gate_addr_lower(idt_entry_t * gate, void * addr)
>> Any reason for this being an inline function and the other being
>> a macro?
>>
>> Jan
> 
> Where possible, I prefer static inline in preference to macros because 
> of the added type checking etc.
> 
> _set_gate_lower is based on _set_gate, so I used the _set_gate style.
> 
> Again, I am not overly fussed if you have a strong preference for 
> style.  (Both of these styles are mixed across the codebase and have no 
> indication of preference in CODING_STYLE)

Generally, when a parameter is referenced more than once, or
when local variables are needed, we should prefer inline
functions over macros. With the exception of this causing
dependency problems, of course.

Jan


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