Re: [PATCH v3 for-4.15] x86/msr: introduce an option for compatible MSR behavior selection

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

On Tue, Mar 09, 2021 at 03:26:07PM +0100, Jan Beulich wrote:
> On 09.03.2021 14:53, Roger Pau Monné wrote:
> > On Tue, Mar 09, 2021 at 12:36:39PM +0100, Jan Beulich wrote:
> >> On 09.03.2021 11:56, Roger Pau Monne wrote:
> >>> @@ -1148,6 +1155,9 @@ static int write_msr(unsigned int reg, uint64_t val,
> >>>          }
> >>>          /* fall through */
> >>>      default:
> >>> +        if ( currd->arch.msr_relaxed && !rdmsr_safe(reg, val) )
> >>> +            return X86EMUL_OKAY;
> >>> +
> >>>          gdprintk(XENLOG_WARNING,
> >>>                   "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
> >>>                   reg, val);
> >>
> >> So what are your thoughts wrt my change to this file? Drop it
> >> altogether and require people to use this new option? Or do you
> >> see both coexist?
> > 
> > I wouldn't be opposed to have both changes co-exist, as long as the PV
> > one is made part of the PV ABI, that is have it properly described in
> > the public headers as part of the PV behavior. I've replied with some
> > details along those lines in your patch.
> > 
> >> In the latter case, since you had suggested
> >> that I drop the write side of my change - does your changing of
> >> the write path indicate you've changed your mind?
> > 
> > Yes, I think we need to provide an option to allow users to revert
> > back to an MSR behavior as close as possible to the previous one for
> > compatibility reasons, and that should include the write side even if
> > we don't know any users requiring it right now.
> > 
> > We would be in a bad position if that use-case gets discovered after
> > the release, so it's IMO best to provide an option that covers both
> > read and write side straight away.
> 
> Well, for your change it's indeed "an option". For my change it's
> not optional behavior (and we also don't mean it to be). Hence I'm
> not sure what I should read out of your reply.

Sorry, maybe my reply wasn't clear. The part of the quote above was my
reply to me re-adding the write side of the change. The reply to
whether I think your PV change is required was in the chunk above.

To clarify:

 - I do think we need the write side of this change, just for the sake
   of providing a behavior as close as possible to the previous
   release.
 - I don't have a strong opinion whether we need two options
   ({rd,wr}msr:relaxed) or just a single one (msr_relaxed). I favor
   a single one because it's likely users will enable both in tandem
   anyway (like you mentioned).
 - I'm fine with your change to PV as long as it's documented in the
   public headers as part of the PV ABI, since it will be enabled
   unconditionally (more about that in a reply [0] to your patch).
 - I think your change to PV should cover the write side also.

Hope that's less confusing.

Thanks, Roger.

[0] https://lore.kernel.org/xen-devel/YEd6GTXJqRIjijl6@Air-de-Roger/