LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: David Edmondson <david.edmondson@oracle.com>
Cc: linux-kernel@vger.kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	Joerg Roedel <joro@8bytes.org>, Ingo Molnar <mingo@redhat.com>,
	Jim Mattson <jmattson@google.com>,
	kvm@vger.kernel.org, Borislav Petkov <bp@alien8.de>,
	David Matlack <dmatlack@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	"H. Peter Anvin" <hpa@zytor.com>,
	x86@kernel.org, Wanpeng Li <wanpengli@tencent.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Joao Martins <joao.m.martins@oracle.com>
Subject: Re: [PATCH v3 2/3] KVM: x86: On emulation failure, convey the exit reason, etc. to userspace
Date: Mon, 2 Aug 2021 16:58:03 +0000	[thread overview]
Message-ID: <YQgkGwGkrleO7I2A@google.com> (raw)
In-Reply-To: <cunk0l4mhjc.fsf@oracle.com>

On Mon, Aug 02, 2021, David Edmondson wrote:
> On Friday, 2021-07-30 at 22:14:48 GMT, Sean Christopherson wrote:
> 
> > On Thu, Jul 29, 2021, David Edmondson wrote:
> >> +		__u64 exit_info1;
> >> +		__u64 exit_info2;
> >> +		__u32 intr_info;
> >> +		__u32 error_code;
> >> +	} exit_reason;
> >
> > Oooh, you're dumping all the fields in kvm_run.  That took me forever to realize
> > because the struct is named "exit_reason".  Unless there's a naming conflict,
> > 'data' would be the simplest, and if that's already taken, maybe 'info'?
> >
> > I'm also not sure an anonymous struct is going to be the easiest to maintain.
> > I do like that the fields all have names, but on the other hand the data should
> > be padded so that each field is in its own data[] entry when dumped to userspace.
> > IMO, the padding complexity isn't worth the naming niceness since this code
> > doesn't actually care about what each field contains.
> 
> Given that this is avowedly not an ABI and that we are expecting any
> (human) consumer to be intimate with the implementation to make sense of
> it, is there really any requirement or need for padding?

My thought with the padding was to force each field into its own data[] entry.
E.g. if userspace does something like

	for (i = 0; i < ndata; i++)
		printf("\tdata[%d] = 0x%llx\n", i, data[i]);

then padding will yield

	data[0] = flags
	data[1] = exit_reason
	data[2] = exit_info1
	data[3] = exit_info2
	data[4] = intr_info
	data[5] = error_code

versus

	data[0] = <flags>
	data[1] = (exit_info1 << 32) | exit_reason
	data[2] = (exit_info2 << 32) | (exit_info1 >> 32)
	data[3] = (intr_info << 32) | (exit_info2 >> 32)
	data[4] = error_code

Changing exit_reason to a u64 would clean up the worst of the mangling, but until
there's actually a 64-bit exit reason to dump, that's just a more subtle way to
pad the data.

> In your example below (most of which I'm fine with), the padding has the
> effect of wasting space that could be used for another u64 of debug
> data.

Yes, but because it's not ABI, we can change it in the future if we get to the
point where we want to dump more info and don't have space.  Until that time, I
think it makes sense to prioritize readability with an ignorant (of the format)
userspace over memory footprint.

> > 	/*
> > 	 * There's currently space for 13 entries, but 5 are used for the exit
> > 	 * reason and info.  Restrict to 4 to reduce the maintenance burden
> > 	 * when expanding kvm_run.emulation_failure in the future.
> > 	 */
> > 	if (WARN_ON_ONCE(ndata > 4))
> > 		ndata = 4;
> >
> > 	if (insn_size) {
> > 		ndata_start = 3;
> > 		run->emulation_failure.flags =
> > 			KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES;
> > 		run->emulation_failure.insn_size = insn_size;
> > 		memset(run->emulation_failure.insn_bytes, 0x90,
> > 		       sizeof(run->emulation_failure.insn_bytes));
> > 		memcpy(run->emulation_failure.insn_bytes, insn_bytes, insn_size);
> > 	} else {
> > 		/* Always include the flags as a 'data' entry. */
> > 		ndata_start = 1;
> > 		run->emulation_failure.flags = 0;
> > 	}
> 
> When we add another flag (presuming that we do, because if not there was
> not much point in the flags) this will have to be restructured again. Is
> there an objection to the original style? (prime ndata=1, flags=0, OR in
> flags and adjust ndata as we go.)

No objection, though if you OR in flags then you should truly _adjust_ ndata, not
set it, e.g.

        /* Always include the flags as a 'data' entry. */
        ndata_start = 1;
        run->emulation_failure.flags = 0;

        if (insn_size) {
                ndata_start += 2;  <----------------------- Adjust, not override
                run->emulation_failure.flags |=
                        KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES;
                run->emulation_failure.insn_size = insn_size;
                memset(run->emulation_failure.insn_bytes, 0x90,
                       sizeof(run->emulation_failure.insn_bytes));
                memcpy(run->emulation_failure.insn_bytes, insn_bytes, insn_size);
        }

> > 	memcpy(&run->internal.data[ndata_start], info, ARRAY_SIZE(info));
> > 	memcpy(&run->internal.data[ndata_start + ARRAY_SIZE(info)], data, ndata);
> > }

  reply	other threads:[~2021-08-02 16:58 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-29 13:39 [PATCH v3 0/3] kvm: x86: Convey the exit reason, etc. to user-space on emulation failure David Edmondson
2021-07-29 13:39 ` [PATCH v3 1/3] KVM: x86: kvm_x86_ops.get_exit_info should include the exit reason David Edmondson
2021-07-29 22:27   ` Sean Christopherson
2021-07-30  7:29     ` David Edmondson
2021-07-29 13:39 ` [PATCH v3 2/3] KVM: x86: On emulation failure, convey the exit reason, etc. to userspace David Edmondson
2021-07-30 22:14   ` Sean Christopherson
2021-08-02  7:28     ` David Edmondson
2021-08-02 16:58       ` Sean Christopherson [this message]
2021-08-02 17:23         ` David Edmondson
2021-08-07  0:59           ` Sean Christopherson
2021-07-29 13:39 ` [PATCH v3 3/3] KVM: x86: SGX must obey the KVM_INTERNAL_ERROR_EMULATION protocol David Edmondson
2021-07-30 22:17   ` Sean Christopherson
2021-08-02  7:18     ` David Edmondson

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=YQgkGwGkrleO7I2A@google.com \
    --to=seanjc@google.com \
    --cc=bp@alien8.de \
    --cc=david.edmondson@oracle.com \
    --cc=dmatlack@google.com \
    --cc=hpa@zytor.com \
    --cc=jmattson@google.com \
    --cc=joao.m.martins@oracle.com \
    --cc=joro@8bytes.org \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=tglx@linutronix.de \
    --cc=vkuznets@redhat.com \
    --cc=wanpengli@tencent.com \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH v3 2/3] KVM: x86: On emulation failure, convey the exit reason, etc. to userspace' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).