diff mbox series

[v14,1/7] x86: lib: Separate instruction decoder MMIO type from MMIO trace

Message ID 20230101162910.710293-2-Jason@zx2c4.com
State New
Headers
Series implement getrandom() in vDSO |

Commit Message

Jason A. Donenfeld Jan. 1, 2023, 4:29 p.m. UTC 
  Both mmiotrace.h and insn-eval.h define various MMIO_ enum constants.
Rename the insn ones to have a INSN_ prefix, so that the headers can be
used from the same source file.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 arch/x86/coco/tdx/tdx.c          | 26 +++++++++++++-------------
 arch/x86/include/asm/insn-eval.h | 18 +++++++++---------
 arch/x86/kernel/sev.c            | 18 +++++++++---------
 arch/x86/lib/insn-eval.c         | 20 ++++++++++----------
 4 files changed, 41 insertions(+), 41 deletions(-)

Comments

Ingo Molnar Jan. 3, 2023, 10:32 a.m. UTC | #1 
* Jason A. Donenfeld <Jason@zx2c4.com> wrote:

> Both mmiotrace.h and insn-eval.h define various MMIO_ enum constants.
> Rename the insn ones to have a INSN_ prefix, so that the headers can be
> used from the same source file.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  arch/x86/coco/tdx/tdx.c          | 26 +++++++++++++-------------
>  arch/x86/include/asm/insn-eval.h | 18 +++++++++---------
>  arch/x86/kernel/sev.c            | 18 +++++++++---------
>  arch/x86/lib/insn-eval.c         | 20 ++++++++++----------
>  4 files changed, 41 insertions(+), 41 deletions(-)

FYI, I've applied this fix to tip:x86/asm, as the fix for the namespace 
clash makes sense independently of the vDSO getrandom feature.

Thanks,

	Ingo
Jason A. Donenfeld Jan. 3, 2023, 2:51 p.m. UTC | #2 
On Tue, Jan 03, 2023 at 11:32:14AM +0100, Ingo Molnar wrote:
> 
> * Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> 
> > Both mmiotrace.h and insn-eval.h define various MMIO_ enum constants.
> > Rename the insn ones to have a INSN_ prefix, so that the headers can be
> > used from the same source file.
> > 
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > ---
> >  arch/x86/coco/tdx/tdx.c          | 26 +++++++++++++-------------
> >  arch/x86/include/asm/insn-eval.h | 18 +++++++++---------
> >  arch/x86/kernel/sev.c            | 18 +++++++++---------
> >  arch/x86/lib/insn-eval.c         | 20 ++++++++++----------
> >  4 files changed, 41 insertions(+), 41 deletions(-)
> 
> FYI, I've applied this fix to tip:x86/asm, as the fix for the namespace 
> clash makes sense independently of the vDSO getrandom feature.

I guess you missed the conversation with Borislav yesterday about that.
He mentioned that I'd just take it through random.git when this whole
series goes in.

(Unless of course you wanted to put this into 6.2? That'd be easiest for
me.)

Jason
Ingo Molnar Jan. 3, 2023, 5 p.m. UTC | #3 
* Jason A. Donenfeld <Jason@zx2c4.com> wrote:

> On Tue, Jan 03, 2023 at 11:32:14AM +0100, Ingo Molnar wrote:
> > 
> > * Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > 
> > > Both mmiotrace.h and insn-eval.h define various MMIO_ enum constants.
> > > Rename the insn ones to have a INSN_ prefix, so that the headers can be
> > > used from the same source file.
> > > 
> > > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > > ---
> > >  arch/x86/coco/tdx/tdx.c          | 26 +++++++++++++-------------
> > >  arch/x86/include/asm/insn-eval.h | 18 +++++++++---------
> > >  arch/x86/kernel/sev.c            | 18 +++++++++---------
> > >  arch/x86/lib/insn-eval.c         | 20 ++++++++++----------
> > >  4 files changed, 41 insertions(+), 41 deletions(-)
> > 
> > FYI, I've applied this fix to tip:x86/asm, as the fix for the namespace 
> > clash makes sense independently of the vDSO getrandom feature.
> 
> I guess you missed the conversation with Borislav yesterday about that.
> He mentioned that I'd just take it through random.git when this whole
> series goes in.

Please base your tree off on tip:x86/asm then (or pull it in) - it only 
includes this patch and another trivial patch at:

   a0e3aa8fe6cb ("x86/insn: Avoid namespace clash by separating instruction decoder MMIO type from MMIO trace type")

We often modify these files - roughly ~4 commits to arch/x86/kernel/sev.c 
alone per cycle on average - and it would be better to avoid conflicts 
here.

Thanks,

	Ingo
Borislav Petkov Jan. 3, 2023, 5:29 p.m. UTC | #4 
On Tue, Jan 03, 2023 at 06:00:46PM +0100, Ingo Molnar wrote:
> > I guess you missed the conversation with Borislav yesterday about that.
> > He mentioned that I'd just take it through random.git when this whole
> > series goes in.
> 
> Please base your tree off on tip:x86/asm then (or pull it in) - it only

My idea was a lot simpler: avoid the tree inter-dependency by us acking this
patch so that it can go through the random.git tree.
Jason A. Donenfeld Jan. 3, 2023, 5:30 p.m. UTC | #5 
On Tue, Jan 3, 2023 at 6:29 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Tue, Jan 03, 2023 at 06:00:46PM +0100, Ingo Molnar wrote:
> > > I guess you missed the conversation with Borislav yesterday about that.
> > > He mentioned that I'd just take it through random.git when this whole
> > > series goes in.
> >
> > Please base your tree off on tip:x86/asm then (or pull it in) - it only
>
> My idea was a lot simpler: avoid the tree inter-dependency by us acking this
> patch so that it can go through the random.git tree.

Indeed I would prefer this.

Or... just put this in 6.2 because it's trivial anyway? Heck, even
mark it as stable@ so make future backporting easier. Then it'll meet
tip's urgent criteria.

Jason
Ingo Molnar Jan. 3, 2023, 5:47 p.m. UTC | #6 
* Jason A. Donenfeld <Jason@zx2c4.com> wrote:

> On Tue, Jan 3, 2023 at 6:29 PM Borislav Petkov <bp@alien8.de> wrote:
> >
> > On Tue, Jan 03, 2023 at 06:00:46PM +0100, Ingo Molnar wrote:
> > > > I guess you missed the conversation with Borislav yesterday about that.
> > > > He mentioned that I'd just take it through random.git when this whole
> > > > series goes in.
> > >
> > > Please base your tree off on tip:x86/asm then (or pull it in) - it only
> >
> > My idea was a lot simpler: avoid the tree inter-dependency by us acking this
> > patch so that it can go through the random.git tree.
> 
> Indeed I would prefer this.
> 
> Or... just put this in 6.2 because it's trivial anyway? Heck, even mark 
> it as stable@ so make future backporting easier. Then it'll meet tip's 
> urgent criteria.

Yeah - that's sensible too, it does fix a header namespace bug - I've put 
it into tip:x86/urgent.

Thanks,

	Ingo
Jason A. Donenfeld Jan. 3, 2023, 5:48 p.m. UTC | #7 
On Tue, Jan 3, 2023 at 6:47 PM Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> > On Tue, Jan 3, 2023 at 6:29 PM Borislav Petkov <bp@alien8.de> wrote:
> > >
> > > On Tue, Jan 03, 2023 at 06:00:46PM +0100, Ingo Molnar wrote:
> > > > > I guess you missed the conversation with Borislav yesterday about that.
> > > > > He mentioned that I'd just take it through random.git when this whole
> > > > > series goes in.
> > > >
> > > > Please base your tree off on tip:x86/asm then (or pull it in) - it only
> > >
> > > My idea was a lot simpler: avoid the tree inter-dependency by us acking this
> > > patch so that it can go through the random.git tree.
> >
> > Indeed I would prefer this.
> >
> > Or... just put this in 6.2 because it's trivial anyway? Heck, even mark
> > it as stable@ so make future backporting easier. Then it'll meet tip's
> > urgent criteria.
>
> Yeah - that's sensible too, it does fix a header namespace bug - I've put
> it into tip:x86/urgent.

Excellent, thanks. I'll rebase on that when it hits Linus' tree.
Ingo Molnar Jan. 4, 2023, 8:25 p.m. UTC | #8 
* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> 
> > On Tue, Jan 3, 2023 at 6:29 PM Borislav Petkov <bp@alien8.de> wrote:
> > >
> > > On Tue, Jan 03, 2023 at 06:00:46PM +0100, Ingo Molnar wrote:
> > > > > I guess you missed the conversation with Borislav yesterday about that.
> > > > > He mentioned that I'd just take it through random.git when this whole
> > > > > series goes in.
> > > >
> > > > Please base your tree off on tip:x86/asm then (or pull it in) - it only
> > >
> > > My idea was a lot simpler: avoid the tree inter-dependency by us acking this
> > > patch so that it can go through the random.git tree.
> > 
> > Indeed I would prefer this.
> > 
> > Or... just put this in 6.2 because it's trivial anyway? Heck, even mark 
> > it as stable@ so make future backporting easier. Then it'll meet tip's 
> > urgent criteria.
> 
> Yeah - that's sensible too, it does fix a header namespace bug - I've put 
> it into tip:x86/urgent.

This namespace clash fix is now upstream as of 512dee0c00ad & later kernels.

Thanks,

	Ingo
Jason A. Donenfeld Jan. 4, 2023, 8:29 p.m. UTC | #9 
On Wed, Jan 4, 2023 at 9:26 PM Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Ingo Molnar <mingo@kernel.org> wrote:
>
> >
> > * Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > > On Tue, Jan 3, 2023 at 6:29 PM Borislav Petkov <bp@alien8.de> wrote:
> > > >
> > > > On Tue, Jan 03, 2023 at 06:00:46PM +0100, Ingo Molnar wrote:
> > > > > > I guess you missed the conversation with Borislav yesterday about that.
> > > > > > He mentioned that I'd just take it through random.git when this whole
> > > > > > series goes in.
> > > > >
> > > > > Please base your tree off on tip:x86/asm then (or pull it in) - it only
> > > >
> > > > My idea was a lot simpler: avoid the tree inter-dependency by us acking this
> > > > patch so that it can go through the random.git tree.
> > >
> > > Indeed I would prefer this.
> > >
> > > Or... just put this in 6.2 because it's trivial anyway? Heck, even mark
> > > it as stable@ so make future backporting easier. Then it'll meet tip's
> > > urgent criteria.
> >
> > Yeah - that's sensible too, it does fix a header namespace bug - I've put
> > it into tip:x86/urgent.
>
> This namespace clash fix is now upstream as of 512dee0c00ad & later kernels.

Thanks. I've rebased my vdso branch on that now. I guess at this point
it probably doesn't matter that much since everyone hates my use of
the instruction decoder anyway, so I'll see what else I can do for
v15, but still, it'll at least make it easier to experiment.

Jason
diff mbox series

Patch

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index cfd4c95b9f04..669d9e4f2901 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -386,8 +386,8 @@  static int handle_mmio(struct pt_regs *regs, struct ve_info *ve)
 {
 	unsigned long *reg, val, vaddr;
 	char buffer[MAX_INSN_SIZE];
+	enum insn_mmio_type mmio;
 	struct insn insn = {};
-	enum mmio_type mmio;
 	int size, extend_size;
 	u8 extend_val = 0;
 
@@ -402,10 +402,10 @@  static int handle_mmio(struct pt_regs *regs, struct ve_info *ve)
 		return -EINVAL;
 
 	mmio = insn_decode_mmio(&insn, &size);
-	if (WARN_ON_ONCE(mmio == MMIO_DECODE_FAILED))
+	if (WARN_ON_ONCE(mmio == INSN_MMIO_DECODE_FAILED))
 		return -EINVAL;
 
-	if (mmio != MMIO_WRITE_IMM && mmio != MMIO_MOVS) {
+	if (mmio != INSN_MMIO_WRITE_IMM && mmio != INSN_MMIO_MOVS) {
 		reg = insn_get_modrm_reg_ptr(&insn, regs);
 		if (!reg)
 			return -EINVAL;
@@ -426,23 +426,23 @@  static int handle_mmio(struct pt_regs *regs, struct ve_info *ve)
 
 	/* Handle writes first */
 	switch (mmio) {
-	case MMIO_WRITE:
+	case INSN_MMIO_WRITE:
 		memcpy(&val, reg, size);
 		if (!mmio_write(size, ve->gpa, val))
 			return -EIO;
 		return insn.length;
-	case MMIO_WRITE_IMM:
+	case INSN_MMIO_WRITE_IMM:
 		val = insn.immediate.value;
 		if (!mmio_write(size, ve->gpa, val))
 			return -EIO;
 		return insn.length;
-	case MMIO_READ:
-	case MMIO_READ_ZERO_EXTEND:
-	case MMIO_READ_SIGN_EXTEND:
+	case INSN_MMIO_READ:
+	case INSN_MMIO_READ_ZERO_EXTEND:
+	case INSN_MMIO_READ_SIGN_EXTEND:
 		/* Reads are handled below */
 		break;
-	case MMIO_MOVS:
-	case MMIO_DECODE_FAILED:
+	case INSN_MMIO_MOVS:
+	case INSN_MMIO_DECODE_FAILED:
 		/*
 		 * MMIO was accessed with an instruction that could not be
 		 * decoded or handled properly. It was likely not using io.h
@@ -459,15 +459,15 @@  static int handle_mmio(struct pt_regs *regs, struct ve_info *ve)
 		return -EIO;
 
 	switch (mmio) {
-	case MMIO_READ:
+	case INSN_MMIO_READ:
 		/* Zero-extend for 32-bit operation */
 		extend_size = size == 4 ? sizeof(*reg) : 0;
 		break;
-	case MMIO_READ_ZERO_EXTEND:
+	case INSN_MMIO_READ_ZERO_EXTEND:
 		/* Zero extend based on operand size */
 		extend_size = insn.opnd_bytes;
 		break;
-	case MMIO_READ_SIGN_EXTEND:
+	case INSN_MMIO_READ_SIGN_EXTEND:
 		/* Sign extend based on operand size */
 		extend_size = insn.opnd_bytes;
 		if (size == 1 && val & BIT(7))
diff --git a/arch/x86/include/asm/insn-eval.h b/arch/x86/include/asm/insn-eval.h
index f07faa61c7f3..54368a43abf6 100644
--- a/arch/x86/include/asm/insn-eval.h
+++ b/arch/x86/include/asm/insn-eval.h
@@ -32,16 +32,16 @@  int insn_fetch_from_user_inatomic(struct pt_regs *regs,
 bool insn_decode_from_regs(struct insn *insn, struct pt_regs *regs,
 			   unsigned char buf[MAX_INSN_SIZE], int buf_size);
 
-enum mmio_type {
-	MMIO_DECODE_FAILED,
-	MMIO_WRITE,
-	MMIO_WRITE_IMM,
-	MMIO_READ,
-	MMIO_READ_ZERO_EXTEND,
-	MMIO_READ_SIGN_EXTEND,
-	MMIO_MOVS,
+enum insn_mmio_type {
+	INSN_MMIO_DECODE_FAILED,
+	INSN_MMIO_WRITE,
+	INSN_MMIO_WRITE_IMM,
+	INSN_MMIO_READ,
+	INSN_MMIO_READ_ZERO_EXTEND,
+	INSN_MMIO_READ_SIGN_EXTEND,
+	INSN_MMIO_MOVS,
 };
 
-enum mmio_type insn_decode_mmio(struct insn *insn, int *bytes);
+enum insn_mmio_type insn_decode_mmio(struct insn *insn, int *bytes);
 
 #endif /* _ASM_X86_INSN_EVAL_H */
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index a428c62330d3..679026a640ef 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -1536,32 +1536,32 @@  static enum es_result vc_handle_mmio_movs(struct es_em_ctxt *ctxt,
 static enum es_result vc_handle_mmio(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
 {
 	struct insn *insn = &ctxt->insn;
+	enum insn_mmio_type mmio;
 	unsigned int bytes = 0;
-	enum mmio_type mmio;
 	enum es_result ret;
 	u8 sign_byte;
 	long *reg_data;
 
 	mmio = insn_decode_mmio(insn, &bytes);
-	if (mmio == MMIO_DECODE_FAILED)
+	if (mmio == INSN_MMIO_DECODE_FAILED)
 		return ES_DECODE_FAILED;
 
-	if (mmio != MMIO_WRITE_IMM && mmio != MMIO_MOVS) {
+	if (mmio != INSN_MMIO_WRITE_IMM && mmio != INSN_MMIO_MOVS) {
 		reg_data = insn_get_modrm_reg_ptr(insn, ctxt->regs);
 		if (!reg_data)
 			return ES_DECODE_FAILED;
 	}
 
 	switch (mmio) {
-	case MMIO_WRITE:
+	case INSN_MMIO_WRITE:
 		memcpy(ghcb->shared_buffer, reg_data, bytes);
 		ret = vc_do_mmio(ghcb, ctxt, bytes, false);
 		break;
-	case MMIO_WRITE_IMM:
+	case INSN_MMIO_WRITE_IMM:
 		memcpy(ghcb->shared_buffer, insn->immediate1.bytes, bytes);
 		ret = vc_do_mmio(ghcb, ctxt, bytes, false);
 		break;
-	case MMIO_READ:
+	case INSN_MMIO_READ:
 		ret = vc_do_mmio(ghcb, ctxt, bytes, true);
 		if (ret)
 			break;
@@ -1572,7 +1572,7 @@  static enum es_result vc_handle_mmio(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
 
 		memcpy(reg_data, ghcb->shared_buffer, bytes);
 		break;
-	case MMIO_READ_ZERO_EXTEND:
+	case INSN_MMIO_READ_ZERO_EXTEND:
 		ret = vc_do_mmio(ghcb, ctxt, bytes, true);
 		if (ret)
 			break;
@@ -1581,7 +1581,7 @@  static enum es_result vc_handle_mmio(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
 		memset(reg_data, 0, insn->opnd_bytes);
 		memcpy(reg_data, ghcb->shared_buffer, bytes);
 		break;
-	case MMIO_READ_SIGN_EXTEND:
+	case INSN_MMIO_READ_SIGN_EXTEND:
 		ret = vc_do_mmio(ghcb, ctxt, bytes, true);
 		if (ret)
 			break;
@@ -1600,7 +1600,7 @@  static enum es_result vc_handle_mmio(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
 		memset(reg_data, sign_byte, insn->opnd_bytes);
 		memcpy(reg_data, ghcb->shared_buffer, bytes);
 		break;
-	case MMIO_MOVS:
+	case INSN_MMIO_MOVS:
 		ret = vc_handle_mmio_movs(ctxt, bytes);
 		break;
 	default:
diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
index 21104c41cba0..558a605929db 100644
--- a/arch/x86/lib/insn-eval.c
+++ b/arch/x86/lib/insn-eval.c
@@ -1595,16 +1595,16 @@  bool insn_decode_from_regs(struct insn *insn, struct pt_regs *regs,
  * Returns:
  *
  * Type of the instruction. Size of the memory operand is stored in
- * @bytes. If decode failed, MMIO_DECODE_FAILED returned.
+ * @bytes. If decode failed, INSN_MMIO_DECODE_FAILED returned.
  */
-enum mmio_type insn_decode_mmio(struct insn *insn, int *bytes)
+enum insn_mmio_type insn_decode_mmio(struct insn *insn, int *bytes)
 {
-	enum mmio_type type = MMIO_DECODE_FAILED;
+	enum insn_mmio_type type = INSN_MMIO_DECODE_FAILED;
 
 	*bytes = 0;
 
 	if (insn_get_opcode(insn))
-		return MMIO_DECODE_FAILED;
+		return INSN_MMIO_DECODE_FAILED;
 
 	switch (insn->opcode.bytes[0]) {
 	case 0x88: /* MOV m8,r8 */
@@ -1613,7 +1613,7 @@  enum mmio_type insn_decode_mmio(struct insn *insn, int *bytes)
 	case 0x89: /* MOV m16/m32/m64, r16/m32/m64 */
 		if (!*bytes)
 			*bytes = insn->opnd_bytes;
-		type = MMIO_WRITE;
+		type = INSN_MMIO_WRITE;
 		break;
 
 	case 0xc6: /* MOV m8, imm8 */
@@ -1622,7 +1622,7 @@  enum mmio_type insn_decode_mmio(struct insn *insn, int *bytes)
 	case 0xc7: /* MOV m16/m32/m64, imm16/imm32/imm64 */
 		if (!*bytes)
 			*bytes = insn->opnd_bytes;
-		type = MMIO_WRITE_IMM;
+		type = INSN_MMIO_WRITE_IMM;
 		break;
 
 	case 0x8a: /* MOV r8, m8 */
@@ -1631,7 +1631,7 @@  enum mmio_type insn_decode_mmio(struct insn *insn, int *bytes)
 	case 0x8b: /* MOV r16/r32/r64, m16/m32/m64 */
 		if (!*bytes)
 			*bytes = insn->opnd_bytes;
-		type = MMIO_READ;
+		type = INSN_MMIO_READ;
 		break;
 
 	case 0xa4: /* MOVS m8, m8 */
@@ -1640,7 +1640,7 @@  enum mmio_type insn_decode_mmio(struct insn *insn, int *bytes)
 	case 0xa5: /* MOVS m16/m32/m64, m16/m32/m64 */
 		if (!*bytes)
 			*bytes = insn->opnd_bytes;
-		type = MMIO_MOVS;
+		type = INSN_MMIO_MOVS;
 		break;
 
 	case 0x0f: /* Two-byte instruction */
@@ -1651,7 +1651,7 @@  enum mmio_type insn_decode_mmio(struct insn *insn, int *bytes)
 		case 0xb7: /* MOVZX r32/r64, m16 */
 			if (!*bytes)
 				*bytes = 2;
-			type = MMIO_READ_ZERO_EXTEND;
+			type = INSN_MMIO_READ_ZERO_EXTEND;
 			break;
 
 		case 0xbe: /* MOVSX r16/r32/r64, m8 */
@@ -1660,7 +1660,7 @@  enum mmio_type insn_decode_mmio(struct insn *insn, int *bytes)
 		case 0xbf: /* MOVSX r32/r64, m16 */
 			if (!*bytes)
 				*bytes = 2;
-			type = MMIO_READ_SIGN_EXTEND;
+			type = INSN_MMIO_READ_SIGN_EXTEND;
 			break;
 		}
 		break;