Re: [Minios-devel] [UNIKRAFT RFC PATCH 2/5] plat/kvm: arm64: Enable the fp/simd at the starting point

[Santiago Pagani <Santiago.Pagani@xxxxxxxxx>]

Hi Julien,

On 20.12.19, 08:50, "Julien Grall" <julien@xxxxxxx> wrote:

    
    
    On 20/12/2019 01:14, Justin He wrote:
    > Hi Julien
    
    Hello,
    
    > 
    >> -----Original Message-----
    >> From: Julien Grall <julien@xxxxxxx>
    >> Sent: Thursday, December 19, 2019 10:35 PM
    >> To: Justin He <Justin.He@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx;
    >> Simon Kuenzer <simon.kuenzer@xxxxxxxxx>; Santiago.Pagani@xxxxxxxxx
    >> Cc: Felipe Huici <felipe.huici@xxxxxxxxx>; Kaly Xin <Kaly.Xin@xxxxxxx>;
    >> Julien Grall <Julien.Grall@xxxxxxx>; Sharan.Santhanam@xxxxxxxxx
    >> Subject: Re: [Minios-devel] [UNIKRAFT RFC PATCH 2/5] plat/kvm: arm64:
    >> Enable the fp/simd at the starting point
    >>
    >> Hi,
    >>
    >> On 19/12/2019 14:27, Jia He wrote:
    >>> Write the sys reg to enable the fp/simd feature, otherwise it will
    >>> cause floating point/simd exception when touching q0-q31.
    >>
    >> In your first patch you say FPSIMD will be available for application
    >> only. But here, you will allow Unikraft itself to use it.
    >>
    >> So don't you want to turn this only when jumping to the application code?
    >>
    > Yes, this might be more graceful.
    > But there is a corner case in libukdebug of Unikraft.
    > If we removed -mgeneral-regs-only, some printf* family functions 
(especially
    > those have variable length parameters, .e.g va_start/end) will be 
compiled to
    > use q0-q31.
    
    I have already had a lenghty discussion in the past with Wei Chen (see 
    [1]) about it.
    
    If you tell GCC to enable floating point for the kernel, then your 
    exception path (e.g interrupt) may also use floating. Therefore you 
    would also have to save/restore them at *every* exception to prevent any 
    corruption.
    
    
    > Hence, if we want to disable fp/simd in unikraft kernel and enable it in
    > userspace, we need to specify different compilation options with/without
    > -mgeneral-regs-only for each unkraft kernel library. I don't know whether
    > it is acceptable? @Simon Kuenzer What do you think of it?
    
    See above, if you decide to enable FPSIMD for your kernel then this 
    series is not enough.
    
    However, this is likely a waste of time as the kernel itself should have 
    limited use of FPSIMD. Now imagine for SVE....
    
    So I would highly recommend to ensure the kernel (particularly the 
    exception path) is free of FPSIMD.
    
I've been thinking a bit more about this, and I do not think we can do what you 
suggest. Namely, in Unikraft there is no difference between a kernel and an 
application. This is not Linux, where we have user space and applications and 
kernel space. Everything gets compiled as a single image, running in flat 
memory space, everything with the same privileges and properties. We could 
compile the "application" files without the -mgeneral-regs-only flag, and the 
"kernel" files with it, but that does not guarantee that there will not be 
corruption in an exception, as the exception could occur when running 
"application code", and it is directly handled without going through the 
"kernel".

Therefore, when we are compiling an image that has floating point support, then 
yes, every will need to save/restore all registers that could potentially get 
corrupted, and this should be added by Justin to his patch. However, we need 
consistency for a given imagen, and we cannot have different parts of the image 
compiled with/without the -mgeneral-regs-only flag.

Thanks and best,
Santiago
    
    > --
    > Cheers,
    > Justin (Jia He)
    > 
    > 
    >>>
    >>> Signed-off-by: Jia He <justin.he@xxxxxxx>
    >>> ---
    >>>    plat/kvm/arm/entry64.S | 8 ++++++++
    >>>    1 file changed, 8 insertions(+)
    >>>
    >>> diff --git a/plat/kvm/arm/entry64.S b/plat/kvm/arm/entry64.S
    >>> index 359a310..1e32268 100644
    >>> --- a/plat/kvm/arm/entry64.S
    >>> +++ b/plat/kvm/arm/entry64.S
    >>> @@ -36,6 +36,7 @@
    >>>    #include <kvm-arm/mm.h>
    >>>    #include <arm/cpu_defs.h>
    >>>    #include <uk/plat/common/sections.h>
    >>> +#include <uk/config.h>
    >>>
    >>>    .global page_table_size
    >>>    .data
    >>> @@ -49,6 +50,13 @@ page_table_size:
    >>>
    >>>    .text
    >>>    ENTRY(_libkvmplat_entry)
    >>> +#ifdef CONFIG_FLOAT_POINT
    >>> +   /* Enable fp/simd support */
    >>> +   ldr        x0, =(3 << 20)
    >>> +   msr        cpacr_el1, x0
    >>> +   isb
    >>> +#endif
    >>> +
    >>>      /* Calculate the image size */
    >>>      ldr x25, =_dtb
    >>>      ldr x26, =_end
    >>>
    >>
    >> --
    >> Julien Grall
    > IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
    > 
    
    Please configure your e-mail client to remove the disclaimer.
    
    Cheers,
    
    [1] 
    
https://lists.xenproject.org/archives/html/minios-devel/2018-07/msg00038.html
    
    -- 
    Julien Grall
    

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