Re: [PATCH] Add support for the x86 XSAVE extended state on FreeBSD/x86.

[Mark Kettenis <mark dot kettenis at xs4all dot nl>]

> Date: Sat, 21 Mar 2015 15:14:48 -0400
> From: John Baldwin <jhb@FreeBSD.org>
> 
> >> +  if (x86_xsave_len != 0)
> >> +    {
> >> +      switch (xcr0 & X86_XSTATE_ALL_MASK)
> >> +	{
> >> +	case X86_XSTATE_MPX_AVX512_MASK:
> >> +	case X86_XSTATE_AVX512_MASK:
> >> +	  if (is64)
> >> +	    return tdesc_amd64_avx512;
> >> +	  else
> >> +	    return tdesc_i386_avx512;
> >> +	case X86_XSTATE_MPX_MASK:
> >> +	  if (is64)
> >> +	    return tdesc_amd64_mpx;
> >> +	  else
> >> +	    return tdesc_i386_mpx;
> >> +	case X86_XSTATE_AVX_MASK:
> >> +	  if (is64)
> >> +	    return tdesc_amd64_avx;
> >> +	  else
> >> +	    return tdesc_i386_avx;
> >> +	default:
> >> +	  if (is64)
> >> +	    return tdesc_amd64;
> >> +	  else
> >> +	    return tdesc_i386;
> >> +	}
> > 
> > These xcr0 -> tdesc mappings need to appear in multiple places.
> > I wonder whether it'd make sense to put them in a single helper
> > function (in the fbsd tdep file) that takes "xcr0" and "is64" as
> > parameters, and returns the corresponding tdesc.
> 
> There are a couple of options I've thought about for this.  One
> has been to have a shared to_read_description implementation in
> an x86fbsd-nat.c (Linux uses a shared one in x86-linux-nat.c).
> However, these case statements are also not really FreeBSD (or BSD)
> specific.  What if I added functions in amd64-tdep.c and i386-tdep.c
> that returned the correct target description for a given xcr0
> value?  Something like:
> 
> struct target_desc *
> i386_target_description(uint64_t xcr0)
> {
> 
>   /* i386 switch statement here */
> }
> 
> That could be reused for the core read_description callback as
> well as the native ones.  This could also be reused by other
> systems that grow XSAVE support in the future.

Probably a good idea.  I'm working on XSAVE support in the OpenBSD
kernel, so I'll eventually need this as well.

I have no real objection to adding the ptrace-specific bits to the
generic BSD native code like your diff is doing.  I'll probably try to
use the same interface for my OpenBSD implementation.  I have one
concern about that code though.  The _supply_xsave() and
_collect_xsave() functions don't accept a length, so they can't do any
bounds checking.  Therefore, 'xstat_bv' as returned by the kernel must
be set correctly (i.e. not have bits sets that imply state beyond
x86_save_len is present.  Does the FreeBSD kernel guarantee that?

Oh, and please rename x86_xsave_len into amd64bsd_xsave_len and
i386bsd_xsave_len to keep the "namesapce" clean.