Re: [PATCH v2 gnumach 1/3] add xfloat thread state interface

[Samuel Thibault]

Hello,

Thanks for the improved version!

Luca Dariz, le mer. 21 août 2024 18:36:14 +0200, a ecrit:
> @@ -495,10 +534,11 @@ ASSERT_IPL(SPL0);
>   * concurrent fpu_set_state or fpu_get_state.
>   */
>  kern_return_t
> -fpu_get_state(const thread_t thread,
> -           struct i386_float_state *state)
> +fpu_get_state(const thread_t thread, void *state, int flavor)
>  {
>       pcb_t pcb = thread->pcb;
> +     struct i386_float_state *fstate = (struct i386_float_state*)state;
> +     struct i386_xfloat_state *xfstate = (struct i386_xfloat_state*)state;
>       struct i386_fpsave_state *ifps;
>  
>  ASSERT_IPL(SPL0);
> @@ -512,7 +552,10 @@ ASSERT_IPL(SPL0);
>            * No valid floating-point state.
>            */
>           simple_unlock(&pcb->lock);
> -         memset(state, 0, sizeof(struct i386_float_state));
> +            if (flavor == i386_FLOAT_STATE)
> +                memset(state, 0, sizeof(struct i386_float_state));
> +            else if (flavor == i386_XFLOAT_STATE)
> +                memset(state, 0, sizeof(struct i386_xfloat_state));

I guess we should also memset to 0 the fp_xsave_size - sizeof(struct
i386_xfp_save) part, to avoid leaking data? Thus just pass fp_xsave_size
rather than sizeof(struct i386_xfloat_state).

> diff --git a/i386/include/mach/i386/thread_status.h 
> b/i386/include/mach/i386/thread_status.h
> index 94596a74..e5632ed6 100644
> --- a/i386/include/mach/i386/thread_status.h
> +++ b/i386/include/mach/i386/thread_status.h
> @@ -148,6 +149,20 @@ struct i386_float_state {
>  };
>  #define i386_FLOAT_STATE_COUNT (sizeof(struct 
> i386_float_state)/sizeof(unsigned int))
>  
> +#define XFP_STATE_BYTES (sizeof (struct i386_xfp_save))
> +
> +struct i386_xfloat_state {
> +     int             fpkind;                 /* FP_NO..FP_387X (readonly) */
> +     int             initialized;
> +     int             exc_status;             /* exception status (readonly) 
> */
> +     int             fp_save_kind;           /* format of hardware state */
> +     unsigned char   hw_state[XFP_STATE_BYTES]; /* actual "hardware" state */

I'm wondering if it's really useful to use XFP_STATE_BYTES here, since
callers are supposed to be able to support the extended part. Better
make hw_state zero-sized so that any miss in supporting the extended
part will hopefully be much more probably caught. That'll also make
i386_get_xstate_size simpler.

Samuel