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 <[email protected]>
> >> Signed-off-by: Jan Beulich <[email protected]>
> >
> > Acked-by: Roger Pau Monné <[email protected]>
>
> 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.
