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

[Jan Beulich <jbeulich@xxxxxxxx>]

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:
>>> Changes since v2:
>>>  - Apply the option to both HVM and PV guest.
>>>  - Handle both reads and writes.
>>
>> I take it that it's intentional that you didn't allow separate read
>> and write control?
> 
> Yes, I don't have a strong opinion, but I think just having a single
> option is better: guests requiring the read side bodge are also likely
> to require the same adjustment on the write side.

I'm not convinced of this - there are many MSRs which merely
need reading to discover a certain piece of (configuration)
information. Note in e.g. how the problem I did run into was
affecting RDMSR only.

> It's also better
> from a user perspective, as it's likely people would enable them in
> tandem anyway.

This part I agree with; in fact I did mention this earlier on.

>>> --- a/xen/arch/x86/pv/emul-priv-op.c
>>> +++ b/xen/arch/x86/pv/emul-priv-op.c
>>> @@ -875,6 +875,7 @@ static int read_msr(unsigned int reg, uint64_t *val,
>>>      const struct domain *currd = curr->domain;
>>>      const struct cpuid_policy *cp = currd->arch.cpuid;
>>>      bool vpmu_msr = false;
>>> +    uint64_t tmp;
>>>      int ret;
>>>  
>>>      if ( (ret = guest_rdmsr(curr, reg, val)) != X86EMUL_UNHANDLEABLE )
>>> @@ -986,6 +987,12 @@ static int read_msr(unsigned int reg, uint64_t *val,
>>>          }
>>>          /* fall through */
>>>      default:
>>> +        if ( currd->arch.msr_relaxed && !rdmsr_safe(reg, tmp) )
>>> +        {
>>> +            *val = 0;
>>> +            return X86EMUL_OKAY;
>>> +        }
>>> +
>>>          gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", reg);
>>>          break;
>>>  
>>> @@ -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.

Jan