On Wed, 2019-06-05 at 11:53 -0400, Christos Zoulas wrote:
> On Jun 5, 5:08pm, mgorny%gentoo.org@localhost (=?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
Attachment:
signature.asc
Description: This is a digitally signed message part