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: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.

~Andrew

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