linux-kernel - Re: [PATCH] x86: lib: Separate instruction decoder MMIO type from MMIO trace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20221214195629.GA15255@ranerica-svr.sc.intel.com>
Date:   Wed, 14 Dec 2022 11:56:29 -0800
From:   Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     tglx@...utronix.de, mingo@...hat.com, dave.hansen@...ux.intel.com,
        bp@...en8.de, x86@...nel.org, hpa@...or.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86: lib: Separate instruction decoder MMIO type from
 MMIO trace

On Mon, Dec 12, 2022 at 02:02:55PM -0700, Jason A. Donenfeld 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@...c4.com>
> ---
> This doesn't fix any existing compile error that I'm aware of, but if
> this makes it into 6.2, it would avoid me having to carry it in a series
> I'm working on, where the clash does result in a compile error.
> 
>  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(-)
> 
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index b8998cf0508a..c05428668fbc 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -347,7 +347,7 @@ static int handle_mmio(struct pt_regs *regs, struct ve_info *ve)
>  	unsigned long *reg, val, vaddr;
>  	char buffer[MAX_INSN_SIZE];
>  	struct insn insn = {};
> -	enum mmio_type mmio;
> +	enum insn_mmio_type mmio;

It would be good to preserve the reverse fir order in the variable
declarations.

>  	int size, extend_size;
>  	u8 extend_val = 0;
>  
> @@ -362,10 +362,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;
> @@ -386,23 +386,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
> @@ -419,15 +419,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..ecd991ec1a98 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -1537,31 +1537,31 @@ static enum es_result vc_handle_mmio(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
>  {
>  	struct insn *insn = &ctxt->insn;
>  	unsigned int bytes = 0;
> -	enum mmio_type mmio;
> +	enum insn_mmio_type mmio;

Same here.

Thanks and BR,
Ricardo