On Mon, Mar 08, 2021 at 02:49:19PM +0100, Jan Beulich wrote:
> On 08.03.2021 13:11, Roger Pau Monné wrote:
> > On Mon, Mar 08, 2021 at 10:33:12AM +0100, Jan Beulich wrote:
> >> On 08.03.2021 09:56, Roger Pau Monné wrote:
> >>> On Fri, Mar 05, 2021 at 10:50:34AM +0100, Jan Beulich wrote:
> >>>> --- a/xen/arch/x86/pv/emul-priv-op.c
> >>>> +++ b/xen/arch/x86/pv/emul-priv-op.c
> >>>> @@ -874,7 +874,7 @@ static int read_msr(unsigned int reg, ui
> >>>> struct vcpu *curr = current;
> >>>> const struct domain *currd = curr->domain;
> >>>> const struct cpuid_policy *cp = currd->arch.cpuid;
> >>>> - bool vpmu_msr = false;
> >>>> + bool vpmu_msr = false, warn = false;
> >>>> int ret;
> >>>>
> >>>> if ( (ret = guest_rdmsr(curr, reg, val)) != X86EMUL_UNHANDLEABLE )
> >>>> @@ -882,7 +882,7 @@ static int read_msr(unsigned int reg, ui
> >>>> if ( ret == X86EMUL_EXCEPTION )
> >>>> x86_emul_hw_exception(TRAP_gp_fault, 0, ctxt);
> >>>>
> >>>> - return ret;
> >>>> + goto done;
> >>>> }
> >>>>
> >>>> switch ( reg )
> >>>> @@ -986,7 +986,7 @@ static int read_msr(unsigned int reg, ui
> >>>> }
> >>>> /* fall through */
> >>>> default:
> >>>> - gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", reg);
> >>>> + warn = true;
> >>>> break;
> >>>>
> >>>> normal:
> >>>> @@ -995,7 +995,19 @@ static int read_msr(unsigned int reg, ui
> >>>> return X86EMUL_OKAY;
> >>>> }
> >>>>
> >>>> - return X86EMUL_UNHANDLEABLE;
> >>>> + done:
> >>>
> >>> Won't this handling be better placed in the 'default' switch case
> >>> above?
> >>
> >> No - see the "goto done" added near the top of the function.
> >
> > Yes, I'm not sure of that. If guest_rdmsr returns anything different
> > than X86EMUL_UNHANDLEABLE it means it has handled the MSR in some way,
> > and hence we shouldn't check whether the #GP handler is set or not.
> >
> > This is not the behavior of older Xen versions, so I'm unsure whether
> > we should introduce a policy that's even less strict than the previous
> > one in regard to whether a #GP is injected or not.
> >
> > I know injecting a #GP when the handler is not set is not helpful for
> > the guest, but we should limit the workaround to kind of restoring the
> > previous behavior for making buggy guests happy, not expanding it
> > anymore.
>
> Yet then we risk breaking guests with any subsequent addition to
> guest_rdmsr() which, under whatever extra conditions, wants to
> raise #GP.
But it's always been like that AFAICT? Additions to guest_{rd/wr}msr
preventing taking the default path in the {read/write}_msr PV
handlers.
If #GP signaled by guest_{rd/wr}msr are no longer injected when the guest
#GP handler is not set we might as well drop the rdmsr_safe check and
just don't try to inject any #GP at all from MSR accesses unless the
handler is setup?
I feel this is likely going too far. I agree we should attempt to
restore something compatible with the previous behavior for unhandled
MSRs, but also not injecting the #GPs signaled by guest_{rd/wr}msr
seems to go beyond that.
Thanks, Roger.
