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

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

On Fri, Jan 30, 2026 at 03:01:28PM +0100, Jan Beulich wrote:
> On 30.01.2026 11:00, Roger Pau Monné wrote:
> > 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>
> 
> Thanks.
> 
> >> ---
> >> 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.
> 
> Unreachable and dead code are different things to Misra, though. It is my
> understanding that we deliberately permit constructs reducing to "if (0)"
> in certain configurations, relying on DCE: There's then no generated code
> for the construct, and hence nothing that cannot be reached. The
> situation is different for a parameter that has no use: Its removal (in
> the specific configuration) wouldn't alter the behavior. Hence the
> parameter and all arguments passed for it are "dead".

Yeah, I think dead is a good definition, the variable is possible
evaluated, but it's value doesn't change the flow of execution in the
VM_EVENT=n case.

> >> @@ -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.
> 
> I did think the same, just that - considering all use sites - I couldn't
> even come close to thinking of some sensible new name.

Let's leave as-is then.  Since maybe_defer is tied to the monitor
logic it might be helpful to give it that meaning, but I'm having a
hard time coming with a proper way to name it that's not too verbose.
Let's leave as-is for now then.

Thanks, Roger.