Re: [PATCH v2 2/2] Implement PT_GETXSTATE and PT_SETXSTATE

[Michał Górny]

On Wed, 2019-06-05 at 11:53 -0400, Christos Zoulas wrote:
> On Jun 5,  5:08pm, mgo...@gentoo.org (=?UTF-8?q?Micha=C5=82=20G=C3=B3rny?=) 
> wrote:
> -- Subject: [PATCH v2 2/2] Implement PT_GETXSTATE and PT_SETXSTATE
> 
> > Introduce two new ptrace() requests: PT_GETXSTATE and PT_SETXSTATE,
> > that provide access to the extended (and extensible) set of FPU
> > registers on amd64 and i386.  At the moment, this covers AVX (YMM)
> > and AVX-512 (ZMM, opmask) registers.  It can be easily extended
> > to cover further register types without breaking backwards
> > compatibility.
> > 
> > PT_GETXSTATE issues the XSAVE instruction with all kernel-supported
> > extended components enabled.  The data is copied into 'struct xstate'
> > (which -- unlike the XSAVE area itself -- has stable format
> > and offsets).
> > 
> > PT_SETXSTATE issues the XRSTOR instruction to restore the register
> > values from user-provided 'struct xstate'.  The function replaces only
> > the specific XSAVE components that are listed in 'xs_xstate_bv' field,
> > making it possible to issue partial updates.
> > 
> > Both syscalls take a 'struct iovec' pointer rather than a direct
> > argument.  This requires the caller to explicitly specify the buffer
> > size.  As a result, existing code will continue to work correctly
> > when the structure is extended (performing partial reads/updates).
> 
> LGTM:
> 
> - There is no need for '} else {' after return

I presume you mean the one in ptrace_machdep_dorequest()?  I suppose it
was there (the code is based on xmmregs in i386) to clearly scope
variables.

I think having something like:

  if (foo)
    return x;
  {
    ...
  }

would be confusing (it would look like a misplaced '{').  Do you prefer
if I put the scope outside 'if', i.e. directly for 'case'?

> - Style says 'return a;' not 'return (a);'

Fixed.

> - While no early returns in 
>       +       if (kl < 0)
>       +               error = EINVAL;
>   and below?
> 

I suppose this avoids duplicating 'uio->uio_offset = 0'.  However, it's
also code copied from elsewhere so I'm not sure what the original
motivation was.

-- 
Best regards,
Michał Górny

signature.asc
Description: This is a digitally signed message part