[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Minios-devel] [UNIKRAFT PATCH 4/4] arch: boilerplate Arm support for thread-local storage



Hi Florian,

On 4/24/19 10:27 AM, Florian Schmidt wrote:
> Hi Costin,
> 
> On 4/16/19 3:22 PM, Costin Lupu wrote:
>> Hi Florian,
>>
>> I'm not sure what is the expected outcome of this patch, however I
>> couldn't build on ARM64 because the following files are not included in
>> the Makefile:
> 
> The whole point of this patch was simply to make sure unikraft still
> compiled on Arm, even if there was no usable functionality in some
> parts. Apparently, I failed with that task. ;-)
> 
> However:
> 
>> +LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) +=
>> $(UK_PLAT_COMMON_BASE)/thread.c|common
>> +LIBKVMPLAT_SRCS-$(CONFIG_ARCH_ARM_64) +=
>> $(UK_PLAT_COMMON_BASE)/sw_ctx.c|common
> 
> That doesn't seem right to me either, for two reasons: There is no
> proper scheduling implemented yet for Arm, if I remember correctly; and
> sw_ctx.c can't properly work on Arm because it includes x86-specific
> code at this point in time. I think any build that enables uksched at
> this point in time fails to build. Maybe that can be fixed by including
> those two files, but I would expect everything to fail spectacularly as
> soon as you run it, then.
> 

The problem here is that sw_ctx.c should not contain any hardware
specific code. So the x86-specific code should be moved elsewhere and an
abstraction should be used instead.

The software context (sw_ctx) is an abstraction used in scheduling when
the hardware support isn't used, e.g. in cooperative scheduling. It's
the opposite of hardware context (hw_ctx) which involves using hardware
support for preemptive scheduling.

> So yeah, the files in this patch are just there to allow compiling and
> linking on Arm with the current feature set (scheduling disabled). Plus
> some very preliminary untested implementation of tls_copy() for Arm64,
> since I figured I'd do it now that I had read all the documentation
> fresh in my mind.
> 
> 
>>
>> Now, I lost track of the ARM changes and I'm not sure if this is WIP or
>> it should be fixed by these TLS series.
>>
>> Please see my other comments inline.
>>
>> On 4/15/19 1:03 PM, Florian Schmidt wrote:
>>> Arm32 is ure boilerplate. Arm64 has some rough guesses about how the
>>
>> A typo here: ure?
> 
> Ah, yes. It should have been "pure boilerplate".
> 
>>
>>> layout should look like, but is untested.
>>>
>>> Signed-off-by: Florian Schmidt <florian.schmidt@xxxxxxxxx>
>>> ---
>>>   arch/arm/arm/include/uk/asm/thread.h   | 60 ++++++++++++++++++++++
>>>   arch/arm/arm64/include/uk/asm/thread.h | 70 ++++++++++++++++++++++++++
>>>   plat/kvm/arm/link64.lds.S              | 18 +++++++
>>>   plat/xen/arm/link32.lds                | 18 +++++++
>>>   4 files changed, 166 insertions(+)
>>>   create mode 100644 arch/arm/arm/include/uk/asm/thread.h
>>>   create mode 100644 arch/arm/arm64/include/uk/asm/thread.h
>>>
>>> diff --git a/arch/arm/arm/include/uk/asm/thread.h
>>> b/arch/arm/arm/include/uk/asm/thread.h
>>> new file mode 100644
>>> index 00000000..7225209e
>>> --- /dev/null
>>> +++ b/arch/arm/arm/include/uk/asm/thread.h
>>> @@ -0,0 +1,60 @@
>>> +/* SPDX-License-Identifier: BSD-3-Clause */
>>> +/*
>>> + * Authors: Florian Schmidt <florian.schmidt@xxxxxxxxx>
>>> + *
>>> + * Copyright (c) 2019, NEC Europe Ltd., NEC Corporation. All rights
>>> reserved.
>>> + *
>>> + * Redistribution and use in source and binary forms, with or without
>>> + * modification, are permitted provided that the following conditions
>>> + * are met:
>>> + *
>>> + * 1. Redistributions of source code must retain the above copyright
>>> + *    notice, this list of conditions and the following disclaimer.
>>> + * 2. Redistributions in binary form must reproduce the above copyright
>>> + *    notice, this list of conditions and the following disclaimer
>>> in the
>>> + *    documentation and/or other materials provided with the
>>> distribution.
>>> + * 3. Neither the name of the copyright holder nor the names of its
>>> + *    contributors may be used to endorse or promote products
>>> derived from
>>> + *    this software without specific prior written permission.
>>> + *
>>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
>>> CONTRIBUTORS "AS IS"
>>> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
>>> TO, THE
>>> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
>>> PARTICULAR PURPOSE
>>> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
>>> CONTRIBUTORS BE
>>> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
>>> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
>>> BUSINESS
>>> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
>>> WHETHER IN
>>> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
>>> OTHERWISE)
>>> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
>>> ADVISED OF THE
>>> + * POSSIBILITY OF SUCH DAMAGE.
>>> + *
>>> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
>>> + */
>>> +
>>> +#ifndef __UKARCH_THREAD_H__
>>> +#error Do not include this header directly
>>> +#endif
>>> +
>>> +#error Thread-local storage not implemented for arm32!
>>> +
>>> +extern char _tls_start[], _etdata[], _tls_end[];
>>> +
>>> +static inline unsigned long tls_area_size(void)
>>> +{
>>> +    return 0;
>>> +}
>>> +
>>> +static inline unsigned int tls_area_align(void)
>>> +{
>>> +    return 1;
>>> +}
>>> +
>>> +static inline void tls_copy(void *tls_area)
>>> +{
>>> +}
>>> +
>>> +static inline void *tls_pointer(void *tls_area)
>>> +{
>>> +    return NULL;
>>> +}
>>> diff --git a/arch/arm/arm64/include/uk/asm/thread.h
>>> b/arch/arm/arm64/include/uk/asm/thread.h
>>> new file mode 100644
>>> index 00000000..2f5d9b98
>>> --- /dev/null
>>> +++ b/arch/arm/arm64/include/uk/asm/thread.h
>>
>> Lets follow the same suggestions for this files as the ones mentioned
>> for the previous patches.
>>
>>> @@ -0,0 +1,70 @@
>>> +/* SPDX-License-Identifier: BSD-3-Clause */
>>> +/*
>>> + * Authors: Florian Schmidt <florian.schmidt@xxxxxxxxx>
>>> + *
>>> + * Copyright (c) 2019, NEC Europe Ltd., NEC Corporation. All rights
>>> reserved.
>>> + *
>>> + * Redistribution and use in source and binary forms, with or without
>>> + * modification, are permitted provided that the following conditions
>>> + * are met:
>>> + *
>>> + * 1. Redistributions of source code must retain the above copyright
>>> + *    notice, this list of conditions and the following disclaimer.
>>> + * 2. Redistributions in binary form must reproduce the above copyright
>>> + *    notice, this list of conditions and the following disclaimer
>>> in the
>>> + *    documentation and/or other materials provided with the
>>> distribution.
>>> + * 3. Neither the name of the copyright holder nor the names of its
>>> + *    contributors may be used to endorse or promote products
>>> derived from
>>> + *    this software without specific prior written permission.
>>> + *
>>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
>>> CONTRIBUTORS "AS IS"
>>> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
>>> TO, THE
>>> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
>>> PARTICULAR PURPOSE
>>> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
>>> CONTRIBUTORS BE
>>> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
>>> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
>>> BUSINESS
>>> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
>>> WHETHER IN
>>> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
>>> OTHERWISE)
>>> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
>>> ADVISED OF THE
>>> + * POSSIBILITY OF SUCH DAMAGE.
>>> + *
>>> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
>>> + */
>>> +
>>> +#ifndef __UKARCH_THREAD_H__
>>> +#error Do not include this header directly
>>> +#endif
>>> +
>>> +#warning Thread-local storage has not been tested on aarch64!
>>> +
>>> +extern char _tls_start[], _etdata[], _tls_end[];
>>> +
>>> +static inline unsigned long tls_area_size(void)
>>> +{
>>> +    /* aarch64 ABI adds 16 bytes of TCB at the beginning of the TLS
>>> area,
>>> +     * followed by the actual TLS data.
>>> +     */
>>> +    return _tls_end - _tls_start + 16;
>>> +}
>>> +
>>> +static inline unsigned int tls_area_align(void)
>>> +{
>>> +    return 8;
>>> +}
>>> +
>>> +static inline void tls_copy(void *tls_area)
>>> +{
>>> +    unsigned int tls_len = _tls_end - _tls_start;
>>> +    unsigned int tls_data_len = _etdata - _tls_start;
>>> +    unsigned int tls_bss_len = _tls_end - _etdata;
>>> +
>>> +    memset(tls_area, 0, 16);
>>> +    memcpy(tls_area+16, _tls_start, tls_data_len);
>>> +    memset(tls_area+tls_data_len+16, 0, tls_bss_len);
>>
>> Again, lets stick to Unikraft coding style for the spacing of these 2
>> lines of code in order to avoid propagating such examples.
> 
> Agreed.
> 
>>
>>> +}
>>> +
>>> +static inline void *tls_pointer(void *tls_area)
>>> +{
>>> +    return tls_area;
>>> +}
>>> diff --git a/plat/kvm/arm/link64.lds.S b/plat/kvm/arm/link64.lds.S
>>> index 82328633..1ce22781 100644
>>> --- a/plat/kvm/arm/link64.lds.S
>>> +++ b/plat/kvm/arm/link64.lds.S
>>> @@ -118,6 +118,24 @@ SECTIONS {
>>>       _ectors = .;
>>>       . = ALIGN(__PAGE_SIZE);
>>>   +    . = ALIGN(0x8);
>>> +    _tls_start = .;
>>> +    .tdata :
>>> +    {
>>> +        *(.tdata)
>>> +        *(.tdata.*)
>>> +        *(.gnu.linkonce.td.*)
>>> +    }
>>> +    _etdata = .;
>>> +    .tbss :
>>> +    {
>>> +        *(.tbss)
>>> +        *(.tbss.*)
>>> +        *(.gnu.linkonce.tb.*)
>>> +        . = ALIGN(0x8);
>>> +    }
>>> +    _tls_end = . + SIZEOF(.tbss);
>>> +
>>
>> Again these linker script changes may be refactored into a single linker
>> file which would be used by all architectures and platforms.
>>
>>>       /* Read-write data that is initialized explicitly in code */
>>>       _data = .;
>>>       .data :
>>> diff --git a/plat/xen/arm/link32.lds b/plat/xen/arm/link32.lds
>>> index 2b8e42dd..e679ba6f 100644
>>> --- a/plat/xen/arm/link32.lds
>>> +++ b/plat/xen/arm/link32.lds
>>> @@ -96,6 +96,24 @@ SECTIONS
>>>       . = ALIGN(4096);
>>>       _ectors = .;
>>>   +    . = ALIGN(0x8);
>>> +    _tls_start = .;
>>> +    .tdata :
>>> +    {
>>> +        *(.tdata)
>>> +        *(.tdata.*)
>>> +        *(.gnu.linkonce.td.*)
>>> +    }
>>> +    _etdata = .;
>>> +    .tbss :
>>> +    {
>>> +        *(.tbss)
>>> +        *(.tbss.*)
>>> +        *(.gnu.linkonce.tb.*)
>>> +        . = ALIGN(0x8);
>>> +    }
>>> +    _tls_end = . + SIZEOF(.tbss);
>>> +
>>>       /* Data */
>>>       _data = .;
>>>       .data : {
>>>
> 

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.