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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 15.03.16 at 21:02, <andrew.cooper3@citrix.com> wrote:
> 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.

Good that I went to the end of this sub-thread, before replying to
suggest just this.

Jan

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