Re: [Xen-devel] [PATCH 2/6] x86/hvm: Handle viridian MSRs via the new guest_{rd, wr}msr() infrastructure

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.03.18 at 13:29, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 01/03/18 09:58, Jan Beulich wrote:
>>
>>>>> @@ -173,11 +175,26 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, 
>>>>>  
> uint64_t *val)
>>>>>                 _MSR_MISC_FEATURES_CPUID_FAULTING;
>>>>>          break;
>>>>>  
>>>>> +    case 0x40000000 ... 0x400001ff:
>>>> As was already suggested, these want to gain #define-s.
>>>>
>>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>> with at least the latter taken care of.
>>> Just like on the CPUID side, the range of valid MSRs depend on the
>>> fallthrough pattern, and which hypervisor(s) we are emulating for.
>>>
>>> This is clearer by the end of the subsequent patch, but the logic is far
>>> easier to follow without these numbers being hidden.
>> I disagree (it's simply impossible to make the connection between
>> the read side and the right side this way, because the numbers
>> could also just happen to be the same, nor is it possible to
>> reasonably find all uses of those numbers via e.g. grep), but well,
>> I don't want to block the patch over this.
> 
> I'm not looking to try and railroad this through.
> 
> If you disagree, then what naming would you suggest instead?  I'd be
> happy to use any naming which doesn't impede the clarity of the logic,
> but I have spent a long time trying to find something suitable, and
> failed to do so.  Raw numbers really are clearer to follow than any
> naming scheme I tried.  The root of the problem is with the fact that
> the MSR spaces overlap, but this information disappears when you try to
> put named constants in.  Also notice that the number of CPUID leaves and
> the number of MSR leaves have a different stride, which adds more
> complexity.

The Viridian MSR range is fixed aiui, so any simple naming scheme
could be used in this patch (as e.g. suggested by Roger, to be lifted
from viridian.c). For the conflicting hypervisor range, how about

#define XEN_MSR_BASE(stride) (0x40000000 + 0x200 * (stride))

? We'll anyway want to declare some upper bound on this range,
which would then be used to describe the upper end in case
labels.

Jan


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