Re: [PATCH v2 2/5] x86: Handle the Xen MSRs via the new guest_{rd, wr}msr() infrastructure

From: "Jan Beulich" <JBeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Sergey Dyasli <sergey.dyasli@citrix.com>,
	Paul Durrant <paul.durrant@citrix.com>,
	Wei Liu <wei.liu2@citrix.com>,
	Xen-devel <xen-devel@lists.xen.org>
Subject: Re: [PATCH v2 2/5] x86: Handle the Xen MSRs via the new guest_{rd, wr}msr() infrastructure
Date: Fri, 07 Sep 2018 09:47:25 -0600	[thread overview]
Message-ID: <5B929D8D02000078001E6793@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <d637e783-d49a-9257-1a89-9c4cade4b6a7@citrix.com>

>>> On 07.09.18 at 16:30, <andrew.cooper3@citrix.com> wrote:
> On 13/03/18 15:04, Jan Beulich wrote:
>>>>> On 07.03.18 at 19:58, <andrew.cooper3@citrix.com> wrote:

Wow, resuming a discussion after a full half year.

>>> --- a/xen/arch/x86/msr.c
>>> +++ b/xen/arch/x86/msr.c
>>> @@ -185,6 +185,10 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>>>          }
>>>  
>>>          /* Fallthrough. */
>>> +    case MSR_XEN_ALT_START ... MSR_XEN_ALT_START + NR_XEN_MSRS - 1:
>> To account for what I've said on patch 1, perhaps this better would
>> be
>>
>>     case MSR_HYPERVISOR_START + NR_VIRIDIAN_MSRS - 1 ... MSR_XEN_ALT_START + NR_XEN_MSRS - 1:
>>
>> to produce consistent results regardless of the value of
>> NR_VIRIDIAN_MSRS (which I suppose is somewhere specified)?
> 
> This demonstrates perfectly why using names here complicates things.  No
> expression like this is going to be obvious to read.
> 
> The raw numbers are the normal way developers think about these ranges,
> and its trivial to spot that the ranges are adjacent.

That's a personal thing - to me it's sometimes this way, sometimes
the other.

> Please compare this suggestion to the guest_cpuid().  The CPUID code is
> far far clearer to read, and the more I think about this, the more I'm
> considering switching back to raw numbers.

That depends very much on how easily the reader can associate back
the raw numbers. This is more likely to be the case for the rather
common CPUID leaves or groups of leaves, and perhaps less likely for
some of the less well known MSRs.

>>> --- a/xen/include/asm-x86/msr-index.h
>>> +++ b/xen/include/asm-x86/msr-index.h
>>> @@ -543,5 +543,7 @@
>>>  /* Hypervisor leaves in the 0x4xxxxxxx range. */
>>>  #define MSR_HYPERVISOR_START            0x40000000
>>>  #define NR_VIRIDIAN_MSRS                0x00000200
>>> +#define MSR_XEN_ALT_START               0x40000200
>>> +#define NR_XEN_MSRS                     0x00000100
>> Where is this count coming from? I don't think it's part of the public
>> interface, but if there was such an upper bound I think it should be.
> 
> Its not part of the public ABI, and it should not be, because we don't
> want to impose an arbitrary limit on how many blocks of 0x100 MSRs the
> Xen range uses.  Its an artefact of attempting to use numbers.

"attempting to use numbers"??? How would we get away without
using some form of number somewhere?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel