Re: [Xen-devel] [PATCH v4 07/34] arm/x86: Use struct virtual_region to do bug, symbol, and (x86) exception tables

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/03/16 19:51, Andrew Cooper wrote:
> On 15/03/16 19:34, Konrad Rzeszutek Wilk wrote:
>> On Tue, Mar 15, 2016 at 07:24:30PM +0000, Andrew Cooper wrote:
>>> On 15/03/16 17:56, Konrad Rzeszutek Wilk wrote:
>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>> index 31d2115..b62c91f 100644
>>>> --- a/xen/arch/arm/traps.c
>>>> +++ b/xen/arch/arm/traps.c
>>>> @@ -16,6 +16,7 @@
>>>>   * GNU General Public License for more details.
>>>>   */
>>>>  
>>>> +#include <xen/bug_ex_symbols.h>
>>> how about just <xen/virtual_region.h> ? It contains more than just
>>> bugframes.
>> /me nods.
>>>> diff --git a/xen/common/bug_ex_symbols.c b/xen/common/bug_ex_symbols.c
>>>> new file mode 100644
>>>> index 0000000..77bb72b
>>>> --- /dev/null
>>>> +++ b/xen/common/bug_ex_symbols.c
>>>> @@ -0,0 +1,119 @@
>>>> +/*
>>>> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
>>>> + *
>>>> + */
>>>> +
>>>> +#include <xen/bug_ex_symbols.h>
>>>> +#include <xen/config.h>
>>>> +#include <xen/kernel.h>
>>>> +#include <xen/init.h>
>>>> +#include <xen/spinlock.h>
>>>> +
>>>> +extern char __stext[];
>>> There is no such symbol.  _stext comes in via kernel.h
>> Argh.
>>
>>>> +
>>>> +struct virtual_region kernel_text = {
>>> How about just "compiled" ? This is more than just .text.
>>>
>>>> +    .list = LIST_HEAD_INIT(kernel_text.list),
>>>> +    .start = (unsigned long)_stext,
>>>> +    .end = (unsigned long)_etext,
>>>> +#ifdef CONFIG_X86
>>>> +    .ex = (struct exception_table_entry *)__start___ex_table,
>>>> +    .ex_end = (struct exception_table_entry *)__stop___ex_table,
>>>> +#endif
>>>> +};
>>>> +
>>>> +/*
>>>> + * The kernel_inittext should only be used when system_state
>>>> + * is booting. Otherwise all accesses should be ignored.
>>>> + */
>>>> +static bool_t ignore_if_active(unsigned int flag, unsigned long priv)
>>>> +{
>>>> +    return (system_state >= SYS_STATE_active);
>>>> +}
>>>> +
>>>> +/*
>>>> + * Becomes irrelevant when __init sections are cleared.
>>>> + */
>>>> +struct virtual_region kernel_inittext  = {
>>>> +    .list = LIST_HEAD_INIT(kernel_inittext.list),
>>>> +    .skip = ignore_if_active,
>>>> +    .start = (unsigned long)_sinittext,
>>>> +    .end = (unsigned long)_einittext,
>>>> +#ifdef CONFIG_X86
>>>> +    /* Even if they are __init their exception entry still gets stuck 
>>>> here. */
>>>> +    .ex = (struct exception_table_entry *)__start___ex_table,
>>>> +    .ex_end = (struct exception_table_entry *)__stop___ex_table,
>>>> +#endif
>>>> +};
>>> This can live in .init.data and be taken off the linked list in
>>> init_done(), which performs other bits of cleanup relating to .init
>> Unfortunatly at that point of time it is SMP - so if we clean it up
>> we need to use a spin_lock.
>>
>>>> +
>>>> +/*
>>>> + * No locking. Additions are done either at startup (when there is only
>>>> + * one CPU) or when all CPUs are running without IRQs.
>>>> + *
>>>> + * Deletions are big tricky. We MUST make sure all but one CPU
>>>> + * are running cpu_relax().
>>> It should still be possible to lock this properly.  We expect no
>>> contention, at which point acquiring and releasing the locks will always
>>> hit fastpaths, but it will avoid accidental corruption if something goes
>>> wrong.
>>>
>>> In each of register or deregister, take the lock, then confirm whether
>>> the current region is in a list or not, by looking at r->list.  With the
>>> single virtual_region_lock held, that can safely avoid repeatedly adding
>>> the region to the region list.
>> Yeah. I don't know why I was thinking we can't. Ah, I was thinking about
>> traversing the list - and we don't want the spin_lock as this is in
>> the do_traps or other code that really really should not take any spinlocks.
>>
>> But if the adding/removing is done under a spinlock then that is OK.
>>
>> Let me do that.
> Actually, that isn't sufficient.  Sorry for misleaing you. 
>
> You have to exclude modifications to the list against other cpus waking
> it in an exception handler, which might include NMI and MCE context.
>
> Now I think about it, going lockless here is probably a bonus, as we
> don't want to be messing around with locks in fatal contexts.  In which
> case, it would be better to use a single linked list and cmpxchg to
> insert/remove elements.  It generally wants to be walked forwards, and
> will only have a handful of elements, so searching forwards to delete
> will be ok.

Actually, knowing that the list is only ever walked forwards by the
exception handlers, and with some regular spinlocks around mutation,
dudicious use of list_add_tail_rcu() and list_del_rcu() should suffice
(I think), and will definitely be better than handrolling a single
linked list.

~Andrew

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