Re: [PATCH] x86emul: allow ->write_msr() to distinguish origins of writes

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jan 27, 2026 at 11:21:06AM +0100, Jan Beulich wrote:
> Only explicit writes are subject to e.g. the checking of the MSR intercept
> bitmap, which extends to VM-event's hvm_monitor_msr(). Indicate, by way of
> a new boolean parameter, to the hook functions which variant it is.
> 
> Fixes: 6eb43fcf8a0b ("x86emul: support SWAPGS")
> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
> Later, in particular for nested, ->read_msr() may need to gain a similar
> parameter.
> 
> Prior to nested making use of it, the new parameter is effectively dead
> code with VM_EVENT=n. If we accepted Misra rule 2.2, something would
> likely need doing about this.

Hm, yes, it propagates into hvm_msr_write_intercept() which then turns
into `if ( may_defer && false )` in the VM_EVENT=n.  But then you
could say the same about the code inside the if block above for the
VM_EVENT=n, it's also effectively unreachable code.

> I've suitably re-based "x86emul: misc additions" on top of this, but I
> don't think I'll re-post it just for that.
> 
> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -569,7 +569,8 @@ static int fuzz_read_msr(
>  static int fuzz_write_msr(
>      unsigned int reg,
>      uint64_t val,
> -    struct x86_emulate_ctxt *ctxt)
> +    struct x86_emulate_ctxt *ctxt,
> +    bool explicit)
>  {
>      struct fuzz_state *s = ctxt->data;
>      struct fuzz_corpus *c = s->corpus;
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1705,7 +1705,8 @@ static int cf_check hvmemul_write_io_dis
>  static int cf_check hvmemul_write_msr_discard(
>      unsigned int reg,
>      uint64_t val,
> -    struct x86_emulate_ctxt *ctxt)
> +    struct x86_emulate_ctxt *ctxt,
> +    bool explicit)
>  {
>      return X86EMUL_OKAY;
>  }
> @@ -2427,9 +2428,10 @@ static int cf_check hvmemul_read_msr(
>  static int cf_check hvmemul_write_msr(
>      unsigned int reg,
>      uint64_t val,
> -    struct x86_emulate_ctxt *ctxt)
> +    struct x86_emulate_ctxt *ctxt,
> +    bool explicit)
>  {
> -    int rc = hvm_msr_write_intercept(reg, val, true);
> +    int rc = hvm_msr_write_intercept(reg, val, explicit);

I've wondered whether we also want to rename the parameter of
hvm_msr_write_intercept() into something different than may_defer.  It
feels weird to translate a parameter that denotes an explicit MSR
access into one that signals whether it's fine to defer the operation
or not.

Thanks, Roger.