Linux-man Archive on lore.kernel.org
 help / color / Atom feed
From: Keno Fischer <keno@juliacomputing.com>
To: Andi Kleen <andi@firstfloor.org>
Cc: Michael Kerrisk-manpages <mtk.manpages@gmail.com>,
	linux-man <linux-man@vger.kernel.org>,
	Andy Lutomirski <luto@kernel.org>,
	Dave Hansen <dave.hansen@intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>,
	"maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)"
	<x86@kernel.org>, "H. Peter Anvin" <hpa@zytor.com>,
	Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>
Subject: Re: [PATCH] ptrace.2: Describe PTRACE_SET/GETREGSET on NT_X86_XSTATE
Date: Tue, 19 May 2020 23:30:02 -0400
Message-ID: <CABV8kRy7XTSv=hJbVSyyKPbT6US7y9AgmG5r9g8AjBJCiVSntw@mail.gmail.com> (raw)
In-Reply-To: <20200520011900.y4fgsiprg6evaxm4@two.firstfloor.org>

> > +.SS The layout and operation of the NT_X86_XSTATE regset
>
> Should rather have a complete table of NT_* entries first. The others
> can be dummies for now.

Oh boy, I'm not sure my man-page-formatting-fu is up to the task of
creating a nice looking table :).
Michael, can you help?

> > +Obtain the kernel xsave component bitmask from the software-reserved area of the
> > +xstate buffer. The software-reserved area beings at offset 464 into the xsave
>
> It would be better to put some struct defining this into the kernel uapi
> and then reference that instead of magic numbers.

We have user_xstateregs in the kernel, but that's not in the uapi.
I suppose we should move it, given that it is exposed here.

> > +buffer and the first 64 bits of this area contain the kernel xsave component bitmask
> > +.IP 2.
> > +Compute the offset of each state component by adding the sizes of all prior state
> > +components that are enabled in the kernel xsave component bitmask, aligning to 64 byte boundaries along the way. This
> > +format matches that of a compacted xsave area with XCOMP_BV set to the
>
> The sizes of these areas should probably also be in the uapi include

Yes, that seems like a good idea.
What's the policy on helper functions in uapi includes?
Can we have helper functions that given a buffer and the kernel xstate mask,
does this computation for you?

> > +kernel component bitmask. Further details on the layout of the compacted xsave
> > +area may be found in the Intel architecture manual, but note that the xsave
> > +buffer returned from ptrace will have its XCOMP_BV set to 0.
>
> The note seems disconnected. You'll have to explain it here.

Ok, I'll elaborate. The point I wanted to make is that even though the buffer
looks compressed, XCOMP_BV is 0, so it's not a valid compressed buffer
that can be xrstor'ed.

> > +Thus, to obtain an xsave area that may be set back to the tracee, all unused
> > +state components must first be re-set to the correct initial state for the
> > +corresponding state component, and the XSTATE_BV bitfield must subsequently
> > +be adjusted to match the kernel xstate component bitmask (obtained as
> > +described above).
>
> I wonder if we shouldn't just fix the kernel to do this properly on its
> own.  Presumably it won't break any existing user space.
>
> It seems more a bug than something that should be a documented ABI.

I'd be happy to see this interface improved, since I do think it wasn't quite
intended to work this way when originally conceived (i.e. originally, before
the init optimization and before we had flags that turn off various xstate
components resulting in a compressed buffer).

As I said in the other email thread, I think it would be reasonable to change
the offsets to always be non-compressed, which would at least make this
a normal xsave buffer. No ptracer that I looked at knows that this buffer
can be compressed, so changing the kernel behavior here would actually
make it closer to what existing userspace expects ;).

I'm not sure what to do about the getregset/setregset mismatch. On the one
hand it's pretty bad, but on the other hand, I'm not really sure what to do
about it, short of introducing a different NT_X86_* constant that behaves
differently.

> > +
> > +The value of the kernel's state component bitmask is determined on boot and
> > +need not be equivalent to the maximal set of state components supported by the
> > +CPU (as enumerated through CPUID).
>
> Okay so how should someone get it? Looks like that's a hole in the
> kernel API that we need to fix somehow.

The cpuid enumerated value does still represent a maximum so that can be used
to size the buffer and the actual value can then be read from the software saved
area as described here. Is that what you were asking? Not sure I understood
correctly.



Keno

  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-18  3:00 Keno Fischer
2020-05-19 20:44 ` Michael Kerrisk (man-pages)
2020-05-19 21:29   ` Denys Vlasenko
2020-05-19 22:46     ` Keno Fischer
2020-05-20 10:03       ` Denys Vlasenko
2020-05-20  1:19 ` Andi Kleen
2020-05-20  3:30   ` Keno Fischer [this message]
2020-05-20  5:08     ` Michael Kerrisk (man-pages)
2020-05-20 13:56     ` Andi Kleen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABV8kRy7XTSv=hJbVSyyKPbT6US7y9AgmG5r9g8AjBJCiVSntw@mail.gmail.com' \
    --to=keno@juliacomputing.com \
    --cc=andi@firstfloor.org \
    --cc=bp@alien8.de \
    --cc=dave.hansen@intel.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=hpa@zytor.com \
    --cc=linux-man@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mingo@redhat.com \
    --cc=mtk.manpages@gmail.com \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-man Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-man/0 linux-man/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-man linux-man/ https://lore.kernel.org/linux-man \
		linux-man@vger.kernel.org
	public-inbox-index linux-man

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-man


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git