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

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

On 07/10/13 13:26, Paul Durrant wrote:
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
>> bounces@lists.xen.org] On Behalf Of Andrew Cooper
>> Sent: 07 October 2013 13:01
>> To: Xen-devel
>> Cc: Andrew Cooper; Keir (Xen.org); Jan Beulich
>> Subject: [Xen-devel] [Patch v3] x86/traps: improvements to {rd,
>> wr}msr_hypervisor_regs()
>>
>> Coverity ID: 1055249 1055250
>>
>> Coverity was complaining that the switch statments contained dead code in
>> their default statements.  While this is quite minor, the code flow in
>> wrmsr_hypervisor_regs() was sufficiently opaque that I felt it approprate to
>> fix.
>>
>> Other improvements include:
>>  * not shadowing the function parameter 'idx'.
>>  * use of PAGE_SHIFT instead of opencoded numbers.
>>  * a more descriptive error message for attempting to write invalid indicies
>>    for hypercall pages.
>>
>> There is no behavioural change as a result.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> ---
>>  xen/arch/x86/traps.c |   44 ++++++++++++++++++--------------------------
>>  1 file changed, 18 insertions(+), 26 deletions(-)
>>
>> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
>> index 6c7bd99..3bb62f0 100644
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -595,55 +595,50 @@ 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 0 if not handled, and non-0 for error. (The calling semantics are
>> + * in need of some work)
>> + */
>>  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.  Reads are invalid. Hand a #GP back. */
>>      {
>>          *val = 0;
>> -        break;
>> +        return 1;
>>      }
>> -    default:
>> -        BUG();
>>      }
>>
>> -    return 1;
>> +    return 0;
>>  }
>>
>> +/* 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 & 0xfff )
> If you're not going to use 12, then shouldn't you use PAGE_SIZE-1 rather than 0xfff?
>
>>          {
>> +            /* Bottom 12 bits are hypercall page index.  Only 0 is valid. */
> And modify this comment accordingly?
>
>   Paul

I suppose.  The use of ~PAGE_MASK before was my mistaken impression that
this was an alignment check rather than a hypercall index check.

As this stuff is not documented anywhere I can find (other than by
reading the code), I am not sure it is caring too much about.  Ideally,
there would be somewhere XEN_MSR_HYPERCALL_PAGE_INDEX_MASK as 0xfff and
XEN_MSR_HYPERCALL_PAGE_MAX_INDEX, but as it is unlikely that Xen will
ever start using multiple hypercall pages, this sounds like overkill.

I am happy to implement it however people prefer, but would err on the
lazy side of leaving it alone if there is no overwhelming objection.

~Andrew