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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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

~Andrew

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