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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.02.18 at 19:32, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 28/02/18 16:40, Jan Beulich wrote:
>>>>> On 26.02.18 at 18:35, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/hvm/viridian.c
>>> +++ b/xen/arch/x86/hvm/viridian.c
>>> @@ -554,13 +554,11 @@ static void update_reference_tsc(struct domain *d, 
> bool_t initialize)
>>>      put_page_and_type(page);
>>>  }
>>>  
>>> -int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
>>> +int guest_wrmsr_viridian(struct vcpu *v, uint32_t idx, uint64_t val)
>> unsigned int would do instead of uint32_t (same on the read side).
> 
> MSRs are architecturally uint32_t, and your proposed coding style
> suggestions would have uint32_t here.  At the moment, uint32_t is used
> consistently throughout the new MSR infrastructure.

I don't really agree: Passing around MSR numbers can be done
using any type at least 32 bits wide. Hence - with our general
assumptions in mind - unsigned int would be suitable (see e.g.
cpuid_count()), as would be uint_least32_t or uint_fast32_t.
IOW as long as we prefer built in types over stdint.h-like ones,
there should really be only extremely few uses of uintNN_t
outside of the public headers and other interface definitions
where "exact width" is what we want.

But as indicated - I'm not going to insist here, as it's clear there
can be quite different views.

>>> @@ -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.

Jan


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