Re: [Minios-devel] [UNIKRAFT RFC PATCH 3/5] arch/arm64: Don't pass -mgeneral-regs-only when CONFIG_FLOAT_POINT is y

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

On 19.12.19 15:46, Julien Grall wrote:
> On 19/12/2019 14:27, Jia He wrote:
> > Support CONFIG_FLOAT_POINT in Unikraft app will take some overhead during
> > context switch. Hence still use -mgeneral-regs-only when
> > CONFIG_FLOAT_POINT is 'n'
> >
> > Signed-off-by: Jia He <justin.he@xxxxxxx>
> > ---
> >   arch/arm/arm64/Makefile.uk | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/arch/arm/arm64/Makefile.uk b/arch/arm/arm64/Makefile.uk
> > index eb73cac..b2c093c 100644
> > --- a/arch/arm/arm64/Makefile.uk
> > +++ b/arch/arm/arm64/Makefile.uk
> > @@ -3,9 +3,15 @@
> >   # we should not enable the FP & SIMD access for kernel. As GCC will
> >   # the FP & SIMD registers to pass parameters, we use 
> > -mgeneral-regs-only
> >   # flag to force GCC to use generic registers
> > +ifeq ($(CONFIG_FLOAT_POINT),y)
> > +ASFLAGS  += -D__ARM_64__
> > +CFLAGS   += -D__ARM_64__ -fms-extensions
> > +CXXFLAGS += -D__ARM_64__ -fms-extensions
> > +else
> The comments above clearly states that FPSIMD access for the kernel 
> should not be allowed. This was introduced by commit:
> commit 670fe83874523f659769aa9fdd3dda891086fb7a
> Author: Wei Chen <Wei.Chen@xxxxxxx>
> Date:   Fri Sep 14 07:56:41 2018 +0000
>
>      arch/arm64: Avoid using the floating-point and Advanced SIMD registers
>
>      On Arm64, sometimes, the GCC will use floating-point and Advanced
>      SIMD registers to pass parameters. For example, the va_list will
>      use the SIMD&FP registers (like q0, q1) to store parameters, no
>      matter you are using floating-point or not.
>
>      But before we include the FP & SIMD registers in context switch,
>      we should not enable the FP & SIMD access for kernel. So, we use
>      the GCC -mgeneral-regs-only flag to force GCC to use generic
>      registers only util the floating-point and Advanced SIMD registers
>      are required actually.
>
>      Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
>      Reviewed-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
>
> The main concern here is if you enable FPSIMD for the kernel, then this 
> could be used by exception (such as interrupts) and therefore corrupt 
> the state.
The problem with all these extra CPU extensions (floating point unit, 
vector registers, etc.) is that on one side, you should not use them in 
kernel context because things like switching to short-living kernel 
contexts (e.g., interrupts) are becoming tricky and/or costly, but on 
the other hand you want them being available in a Unikernel. The actual 
problem is that here we do not have a clear split between kernel and 
user space. We do not have an idea where kernel starts and where user 
program context relies; everything is mixed together with function 
calls; it is a single AS and a single context. To make things even 
worse: We have even just one libc which is shared between what you would 
call traditionally a kernel component and the application.
I think we all agree that ported programs should be able to use 
extensions as they want. Completely switching CPU features off would not 
make much sense. From my past experience from the NFV world, I would 
even say that some traditional kernel components, like network stacks, 
can achieve even better performance when they are instrumented to 
utilize them.
I agree with Julien that we do not have an architectural concept yet, so 
we should agree on one now instead of doing random things.
My suggestion would be to address this problem from the ISR and trap 
handler side: We need to make sure that they do not use any extended CPU 
features so that we do not break any unsaved state. In the end we have 
to restrict the handlers to call only functions that keep this property 
valid. For this purpose I would mark such handlers, and also all 
functions that they use, as "ISR-safe". Such functions should be 
compiled on Arm with `-mgeneral-regs-only` and on x86 we have to do the 
equivalent by disabling SSE, AVX, FP, etc.. Calling a non-ISR-safe 
function from an ISR-safe one breaks the property. The opposite way 
round should be fine as long as there are no differences in the calling 
conventions (I really hope not).
In my opinion, I don't think it would make sense to mark our libc's 
(nolibc, newlib, musl) completely as ISR-safe. Unfortunately, this makes 
it even more likely that extended features are used all over the place 
but I expect that there is another minimal subset of functions that 
handlers would want to use (e.g., memcpy yes; printing and sockets no). 
For those we should provide ISR-safe alternatives (e.g., an isr-safe 
memcpy: memcpy_isr()).
It may get a bit more tricky for things like semaphores or rings that 
can bridge between ISR-safe and normal context world but should still be 
implementable.
In general, I would start by adding a new CFLAGS-y variable to the build 
system: CFLAGS_ISR-y. This one would be used for compilation units that 
need to be ISR-safe. The architecture selection would intialize both 
CFLAGS lists accordingly.
I am not sure if there exists a more elegant way than applying different 
enabled features set by compilation units. I would love to have 
something like function attributes or function declaration macros to set 
and even check the property at compile time. Maybe you have an idea?

Thanks,

Simon

> I really think we should keep the kernel free of FPSIMD to avoid to 
> increase the size of the context to switch on exception. >
> >   ASFLAGS  += -D__ARM_64__ -mgeneral-regs-only
> >   CFLAGS   += -D__ARM_64__ -fms-extensions -mgeneral-regs-only
> >   CXXFLAGS += -D__ARM_64__ -fms-extensions -mgeneral-regs-only
> > +endif
> >   CINCLUDES   += -I$(CONFIG_UK_BASE)/arch/arm/arm64/include
> >   ASINCLUDES  += -I$(CONFIG_UK_BASE)/arch/arm/arm64/include
> Cheers,
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel