Re: [PATCH 1/2] x86/pv: Rework guest_io_okay() to return X86EMUL_*

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.09.2024 18:18, Andrew Cooper wrote:
> In order to fix a bug with guest_io_okay() (subsequent patch), rework
> guest_io_okay() to take in an emulation context, and return X86EMUL_* rather
> than a boolean.
> 
> For the failing case, take the opporunity to inject #GP explicitly, rather
> than returning X86EMUL_UNHANDLEABLE.  There is a logical difference between
> "we know what this is, and it's #GP", vs "we don't know what this is".
> 
> There is no change in practice as emulation is the final step on general #GP
> resolution, but returning X86EMUL_UNHANDLEABLE would be a latent bug if a
> subsequent action were to appear.
> 
> No practical change.

I think there is a small functional difference, see below.

> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -156,14 +156,16 @@ static bool iopl_ok(const struct vcpu *v, const struct 
> cpu_user_regs *regs)
>  }
>  
>  /* Has the guest requested sufficient permission for this I/O access? */
> -static bool guest_io_okay(unsigned int port, unsigned int bytes,
> -                          struct vcpu *v, struct cpu_user_regs *regs)
> +static int guest_io_okay(unsigned int port, unsigned int bytes,
> +                         struct x86_emulate_ctxt *ctxt)
>  {
> +    struct cpu_user_regs *regs = ctxt->regs;

const?

> @@ -190,10 +192,12 @@ static bool guest_io_okay(unsigned int port, unsigned 
> int bytes,
>              toggle_guest_pt(v);
>  
>          if ( (x.mask & (((1 << bytes) - 1) << (port & 7))) == 0 )
> -            return true;
> +            return X86EMUL_OKAY;
>      }
>  
> -    return false;
> +    x86_emul_hw_exception(X86_EXC_GP, 0, ctxt);

do_general_protection() has

    /* Pass on GPF as is. */
    pv_inject_hw_exception(X86_EXC_GP, regs->error_code);

which may make a difference in case the insn changes under our feet.
The new behavior may even be deemed better, but the description then
would want to admit to this slight functional change. Then
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan