Re: Remaining MSR wrinkles

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

On Thu, Mar 11, 2021 at 08:55:29AM +0100, Jan Beulich wrote:
> On 10.03.2021 20:09, Ian Jackson wrote:
> > (I bounced Roger's original mail to xen-devel.  I hope it made it...)
> > 
> > Roger Pau Monné writes ("Remaining MSR wrinkles"):
> >> 1. MSR behavior for PV guests without a #GP handler set: PV Linux versions 
> >> <
> >>    4.14 will use rdmsr_safe (and likely wrmsr_safe?) without having a #GP
> >>    handler setup, which results in a crash. This bug was hidden in previous
> >>    Xen releases by allowing unlimited read access to the MSR space.
> 
> I've not observed problematic wrmsr_safe() so far.
> 
> >>    Jan has posted several proposals to address this:
> >>
> >>    
> >> https://lore.kernel.org/xen-devel/7e69db81-cee7-3c7b-be64-4f5ff50fbe9c@xxxxxxxx/
> >>    
> >> https://lore.kernel.org/xen-devel/d794bbee-a5e5-6632-3d1f-acd8164b7171@xxxxxxxx/
> >>
> >>    Which all rely on the fact that for PV guests Xen knows whether there's 
> >> a
> >>    #GP handler setup and can hence prevent injection of a #GP fault if no
> >>    handler is present.
> >>
> >>    Andrew opinion is that we should instead try to figure out which MSRs 
> >> the
> >>    buggy Linux versions try to access and special case them. Andrew also 
> >> raised
> >>    the point that continue running with a 'fake' (ie: 0) MSR value might be
> >>    worse than crashing.
> >>
> >>    Part of the discussion has also happened here:
> >>
> >>    
> >> https://lore.kernel.org/xen-devel/4da62f0b-8a08-dd84-2040-fd55d74fd85a@xxxxxxxxxx/
> >>
> >>    Look for the last quote.
> >>
> >>    Another option is to document that PV Linux < 4.14 will require 
> >> msr_relaxed=1
> >>    in order to run. That as Jan pointed out will also imply PV Linux to 
> >> run with
> >>    a faked (0) MSR value instead of crashing.
> > 
> >> For 1. I do agree with Jan than this workaround is likely the best option 
> >> we
> >> have, sort of resorting to request enabling msr_relaxed for all Linux PV 
> >> guests
> >> < 4.14. Whether we want to limit this workaround to the read side only I'm 
> >> not
> >> fully convinced. There's something nice about having symmetry in the read 
> >> and
> >> write paths, but if all the calls we have identified are rdmsr only I 
> >> prefer to
> >> leave the write path unaltered and request users to use msr_relaxed if 
> >> write
> >> issues are found later.
> 
> Especially if Andrew's ambiguous objection was against the write side
> only, I think I'd prefer to go with this latter variant. Considering
> that dealing with the read side alone is sufficient to address the
> observed issue, I'm even inclined to prefer this irrespective of that
> constraint.
> 
> > Thanks.  I find your explanation and arguments convincing.  I have
> > read what Andy says in that link and I find that less convincing.  In
> > particular "I don't think we should legitimise buggy PV behaviour" is
> > not entirely consistent with our previous approach to
> > bug-compatibility and support for old guests.
> > 
> > Accordingly, (with committer tie-breaker hat on) I would prefer to
> > apply the patches from Jan.  I don't have an opinion about the read vs
> > write question, and will probably be happy with whatever you and Jan
> > can agree on.
> > 
> > I don't think I Release-Acked those patches yet so, for those two,
> > 
> > Release-Acked-by: Ian Jackson <iwj@xxxxxxxxxxxxxx>
> 
> You didn't, indeed, but "those two" is slightly confusing, the two
> links Roger did provide are just different versions of the same
> patch. Hence I'd like to double check that it is exactly this one
> patch of mine (of which I need to send another version, at least
> to include Roger's requested documentation of the behavior, and
> possibly also the write side equivalent - still waiting for Andrew
> to come back and clarify the scope of his objection).

I would leave the write side out. Now we have the fallback msr_relaxed
option which should be enough to cover for any write side issue that
might arise later. Also you have not identified any problematic
wrmsr_safe so far.

Thanks, Roger.