On Wed, 3 Feb 2021 00:41:00 +0200 Jarkko Sakkinen wrote:

On Wed, Feb 03, 2021 at 08:23:40AM +1300, Kai Huang wrote:

quoted

On Tue, 2 Feb 2021 19:24:42 +0200 Jarkko Sakkinen wrote:

quoted

On Mon, Feb 01, 2021 at 01:32:59PM +1300, Kai Huang wrote:

quoted

On Sat, 30 Jan 2021 17:00:46 +0200 Jarkko Sakkinen wrote:

quoted

On Tue, Jan 26, 2021 at 10:31:37PM +1300, Kai Huang wrote:

quoted

From: Sean Christopherson <redacted>

Convert vcpu_vmx.exit_reason from a u32 to a union (of size u32).  The
full VM_EXIT_REASON field is comprised of a 16-bit basic exit reason in
bits 15:0, and single-bit modifiers in bits 31:16.

Historically, KVM has only had to worry about handling the "failed
VM-Entry" modifier, which could only be set in very specific flows and
required dedicated handling.  I.e. manually stripping the FAILED_VMENTRY
bit was a somewhat viable approach.  But even with only a single bit to
worry about, KVM has had several bugs related to comparing a basic exit
reason against the full exit reason store in vcpu_vmx.

Upcoming Intel features, e.g. SGX, will add new modifier bits that can

BTW, SGX is not an upcoming CPU feature.

Probably Sean was implying: "Upcoming CPU features that will be supported by
Linux". I don't see big deal here.

quoted

Also, broadly speaking of upcoming features is not right thing to do.
Better just to scope this down SGX. Theoretically upcoming CPU features
can do pretty much anything. This is change is first and foremost done
for SGX.

quoted

quoted

quoted

be set on more or less any VM-Exit, as opposed to the significantly more
restricted FAILED_VMENTRY, i.e. correctly handling everything in one-off
flows isn't scalable.  Tracking exit reason in a union forces code to
explicitly choose between consuming the full exit reason and the basic
exit, and is a convenient way to document and access the modifiers.

I *believe* that the change is correct but I dropped in the last paragraph
- most likely only because of lack of expertise in this area.

I ask the most basic question: why SGX will add new modifier bits?

Not 100% sure about your question. Assuming you are asking SGX hardware
behavior, SGX architecture adds a new modifier bit (27) to Exit Reason, similar
to new #PF.SGX bit. 

Please refer to SDM Volume 3, Chapter 27.2.1 Basic VM-Exit Information.

Sean's commit msg already provides significant motivation of the change in this
patch.

Just describe why SGX requires this. That's all.

This patch is to change vmexit info from u32 to union, because at least one
additional modifier is going to be added, due to SGX. So the motivation of this
patch is the fact that "one or more additional modifier bits will be added",
and SGX is just example. 

So I don't think adding too much SGX backgroud in *THIS* patch is needed.
And another patch: 

[RFC PATCH v3 21/27] KVM: VMX: Add basic handling of VM-Exit from SGX enclave

already has enough information of "why new modifier bit is aadded for SGX".
Sean also replied to you. 

Well it comes after this patch. So you either need to provide the context
here or reorder patches. If latter is impossible, I would just add those
couple of paragraphs that Sean wrote.

As I explained, to me the motivation of this patch is due to "adding additional
modifier bit", but not due to "adding additional modifier bit *due to SGX*". 

For instance, let's remove SGX in the commit msg, this patch still stands.
Correct?

Sean's paragraph is about why *SGX* adds one additional modifier bit, which
needs to be in another patch, and logically, that patch comes later.

quoted

Please look at that patch and see whether it satisfies you.

Well there needs to be causality in patches. I should be able to review
the patches if 17-> did not exist.

quoted

quoted

/Jarkko

/Jarkko