Re: [PATCH v2 5/5] x86/tdx: Implement movs for MMIO

[Alexey Gladkov <legion@kernel.org>]

On Thu, Aug 08, 2024 at 08:48:26AM -0500, Tom Lendacky wrote:
> On 8/5/24 08:29, Alexey Gladkov (Intel) wrote:
> > Add emulation of the MOVS instruction on MMIO regions. MOVS emulation
> > consists of dividing it into a series of read and write operations,
> > which in turn will be validated separately.
> > 
> > Signed-off-by: Alexey Gladkov (Intel) <legion@kernel.org>
> > ---
> >  arch/x86/coco/tdx/tdx.c | 57 ++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 53 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> > index 4e2fb9bf83a1..8573cb23837e 100644
> > --- a/arch/x86/coco/tdx/tdx.c
> > +++ b/arch/x86/coco/tdx/tdx.c
> > @@ -509,6 +509,54 @@ static int decode_insn_struct(struct insn *insn, struct pt_regs *regs)
> >  	return 0;
> >  }
> >  
> > +static int handle_mmio_movs(struct insn *insn, struct pt_regs *regs, int size, struct ve_info *ve)
> > +{
> > +	unsigned long ds_base, es_base;
> > +	unsigned char *src, *dst;
> > +	unsigned char buffer[8];
> > +	int off, ret;
> > +	bool rep;
> > +
> > +	/*
> > +	 * The in-kernel code must use a special API that does not use MOVS.
> > +	 * If the MOVS instruction is received from in-kernel, then something
> > +	 * is broken.
> > +	 */
> > +	if (WARN_ON_ONCE(!user_mode(regs)))
> > +		return -EFAULT;
> > +
> > +	ds_base = insn_get_seg_base(regs, INAT_SEG_REG_DS);
> > +	es_base = insn_get_seg_base(regs, INAT_SEG_REG_ES);
> > +
> > +	if (ds_base == -1L || es_base == -1L)
> > +		return -EINVAL;
> > +
> > +	rep = insn_has_rep_prefix(insn);
> > +
> > +	do {
> > +		src = ds_base + (unsigned char *) regs->si;
> > +		dst = es_base + (unsigned char *) regs->di;
> > +
> > +		ret = __get_iomem(src, buffer, size);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = __put_iomem(dst, buffer, size);
> > +		if (ret)
> > +			return ret;
> > +
> > +		off = (regs->flags & X86_EFLAGS_DF) ? -size : size;
> > +
> > +		regs->si += off;
> > +		regs->di += off;
> > +
> > +		if (rep)
> > +			regs->cx -= 1;
> > +	} while (rep || regs->cx > 0);
> > +
> > +	return insn->length;
> > +}
> > +
> >  static int handle_mmio_write(struct insn *insn, enum insn_mmio_type mmio, int size,
> >  			     struct pt_regs *regs, struct ve_info *ve)
> >  {
> > @@ -530,9 +578,8 @@ static int handle_mmio_write(struct insn *insn, enum insn_mmio_type mmio, int si
> >  		return insn->length;
> >  	case INSN_MMIO_MOVS:
> >  		/*
> > -		 * MMIO was accessed with an instruction that could not be
> > -		 * decoded or handled properly. It was likely not using io.h
> > -		 * helpers or accessed MMIO accidentally.
> > +		 * MOVS is processed through higher level emulation which breaks
> > +		 * this instruction into a sequence of reads and writes.
> >  		 */
> >  		return -EINVAL;
> >  	default:
> > @@ -602,6 +649,9 @@ static int handle_mmio(struct pt_regs *regs, struct ve_info *ve)
> >  	if (WARN_ON_ONCE(mmio == INSN_MMIO_DECODE_FAILED))
> >  		return -EINVAL;
> >  
> > +	if (mmio == INSN_MMIO_MOVS)
> > +		return handle_mmio_movs(&insn, regs, size, ve);
> 
> You check the address in the non-MOVS case using valid_vaddr(), but you
> don't seem to be doing that in the MOVS case, was that intentional?

The MOVS instruction is allowed only in userspace. The MOVS instruction
is emulated through separate read and write operations, which are in turn
checked by valid_vaddr(). 

> Thanks,
> Tom
> 
> > +
> >  	vaddr = (unsigned long)insn_get_addr_ref(&insn, regs);
> >  
> >  	if (user_mode(regs)) {
> > @@ -630,7 +680,6 @@ static int handle_mmio(struct pt_regs *regs, struct ve_info *ve)
> >  	switch (mmio) {
> >  	case INSN_MMIO_WRITE:
> >  	case INSN_MMIO_WRITE_IMM:
> > -	case INSN_MMIO_MOVS:
> >  		ret = handle_mmio_write(&insn, mmio, size, regs, ve);
> >  		break;
> >  	case INSN_MMIO_READ:
> 

-- 
Rgrds, legion