|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |