Re: [Patch 2/2] x86/traps: improvements to {rd, wr}msr_hypervisor_regs()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/10/13 11:17, Jan Beulich wrote:
>>>> On 07.10.13 at 11:48, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -595,55 +595,45 @@ DO_ERROR_NOCODE(TRAP_copro_error,     coprocessor_error)
>>  DO_ERROR(       TRAP_alignment_check, alignment_check)
>>  DO_ERROR_NOCODE(TRAP_simd_error,      simd_coprocessor_error)
>>  
>> +/* Returns 1 if handled, 0 if not and -Exx for error. */
> This comment is not in line with all current uses of the function.
> Either you fix the comment, or you fix the callers.

Hmm yes - the error case isn't dealt with properly.  I shall fix the
comment in preference to editing the callsites at the moment.

I have a separate proposal for a change in the way this msr handling
code works, and will fix this up then.

>
>>  int rdmsr_hypervisor_regs(uint32_t idx, uint64_t *val)
>>  {
>>      struct domain *d = current->domain;
>>      /* Optionally shift out of the way of Viridian architectural MSRs. */
>>      uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
>>  
>> -    idx -= base;
>> -    if ( idx > 0 )
>> -        return 0;
>> -
>> -    switch ( idx )
>> +    switch ( idx - base )
>>      {
>> -    case 0:
>> +    case 0: /* Write hypercall page */
> This comment looks more confusing that clarifying considering that
> we're in "rdmsr_...".

Very true.  I shall adjust.

>
>>      {
>>          *val = 0;
>> -        break;
>> +        return 1;
>>      }
>>      default:
>> -        BUG();
>> +        return 0;
> In a situation like this I think it is better to not have a "default:" at
> all, and instead have the "return" at the end of the function deal
> with all cases not getting handled inside the switch statement.
> But yes, this is a matter of taste.
>
>>      }
>> -
>> -    return 1;
>>  }
>>  
>> +/* Returns 1 if handled, 0 if not and -Exx for error. */
>>  int wrmsr_hypervisor_regs(uint32_t idx, uint64_t val)
>>  {
>>      struct domain *d = current->domain;
>>      /* Optionally shift out of the way of Viridian architectural MSRs. */
>>      uint32_t base = is_viridian_domain(d) ? 0x40000200 : 0x40000000;
>>  
>> -    idx -= base;
>> -    if ( idx > 0 )
>> -        return 0;
>> -
>> -    switch ( idx )
>> +    switch ( idx - base )
>>      {
>> -    case 0:
>> +    case 0: /* Write hypercall page */
>>      {
>>          void *hypercall_page;
>> -        unsigned long gmfn = val >> 12;
>> -        unsigned int idx  = val & 0xfff;
>> +        unsigned long gmfn = val >> PAGE_SHIFT;
>>          struct page_info *page;
>>          p2m_type_t t;
>>  
>> -        if ( idx > 0 )
>> +        if ( val & PAGE_MASK )
> Did you mean ~PAGE_MASK? And in the light of this - did you test
> the change?

I did indeed mean ~PAGE_MASK, but also had that in the version of the
code tested.  I think I lost that in a botched rebase, and shall try to
be more careful in the future.

>
>>          {
>>              gdprintk(XENLOG_WARNING,
>> -                     "Out of range index %u to MSR %08x\n",
>> -                     idx, 0x40000000);
>> +                     "Expected aligned frame for writing hypercall page\n");
> I don't think that's the intention here. Instead the low bits are
> specifying the n-th hypercall page, and hence talking about
> alignment here seems wrong.
>
> Jan
>

Is this documented anywhere?  The cpuid documentation describes how to
locate the base address.

Looking at things more closely, that does make sense. I did mistake it
for an alignment check, given unconditionally 1 hypercall page.

I will return it back to what it was intended, but without shadowing the
idx function parameter.

~Andrew