Re: [Xen-devel] [PATCH v2 5/5] x86: Reorganise and rename debug register fields in struct vcpu

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/10/18 14:56, Roger Pau Monné wrote:
> On Mon, Oct 15, 2018 at 11:36:20AM +0100, Andrew Cooper wrote:
>> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
>> index 115ddf6..cc85395 100644
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -1576,8 +1576,11 @@ void arch_get_info_guest(struct vcpu *v, 
>> vcpu_guest_context_u c)
>>          }
>>      }
>>  
>> -    for ( i = 0; i < ARRAY_SIZE(v->arch.debugreg); ++i )
>> -        c(debugreg[i] = v->arch.debugreg[i]);
>> +    for ( i = 0; i < ARRAY_SIZE(v->arch.dr); ++i )
>> +        c(debugreg[i] = v->arch.dr[i]);
>> +    c(debugreg[6] = v->arch.dr6);
>> +    c(debugreg[7] = v->arch.dr7 |
>> +      (is_pv_domain(d) ? v->arch.pv.dr7_emul : 0));
> Wouldn't it be clearer to use c(debugreg[6]) = v->arch.dr6;? I know
> existing code does as above, but I find it more difficult to
> understand.

Clearer => absolutely.

However, your suggestion won't compile.

domctl.c: In function ‘arch_get_info_guest’:
domctl.c:1556:49: error: lvalue required as left operand of assignment
     c(flags) = v->arch.vgc_flags & ~(VGCF_i387_valid|VGCF_in_kernel);
                                                 ^
which is why all the code is written like this.

>
>> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
>> index cdb43e4..6c0887d 100644
>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -549,6 +549,12 @@ struct pv_vcpu
>>      spinlock_t shadow_ldt_lock;
>>  #endif
>>  
>> +    /*
>> +     * %dr7 bits the guest has set, but aren't loaded into hardware, and are
>> +     * completely emulated.
>> +     */
>> +    uint32_t dr7_emul;
> Just to match the size of the actual register shouldn't this be
> unsigned long?
>
> I assume this is not very relevant because the high bits are not
> actually used, but a comment might be appropriate here.

There is no point wasting storage for 4 bytes which are not used.  After
all, its 1/1000'th of struct vcpu.

>
>> +
>>      /* data breakpoint extension MSRs */
>>      uint32_t dr_mask[4];
>>  
>> @@ -567,7 +573,10 @@ struct arch_vcpu
>>      void              *fpu_ctxt;
>>      unsigned long      vgc_flags;
>>      struct cpu_user_regs user_regs;
>> -    unsigned long      debugreg[8];
>> +
>> +    /* Debug registers. */
>> +    unsigned long dr[4], dr7;
>> +    unsigned int dr6;
> I'm likely missing some information here because I don't get why dr6
> is 32bits while dr7 is 64bits wide. According to the spec the high
> part (bits 63-32) on both registers is reserved, so I think it would
> make more sense to use the same type for both.

dr7 would ideally be 32 bits wide, but __vmread() uses unsigned long *.

~Andrew

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