Re: [Xen-devel] [PATCH 3/3] x86/emul: Drop segment_attributes_t

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 03/07/17 13:50, Jan Beulich wrote:
>>>> On 30.06.17 at 17:04, <andrew.cooper3@xxxxxxxxxx> wrote:
>> The amount of namespace resolution is unnecessarily large, as all code deals
>> in terms of struct segment_register.  This removes the attr.fields part of 
>> all
>> references, and alters attr.bytes to just attr.
> Yeah, quite a bit easier to read and type this way.
>
>> @@ -256,7 +255,7 @@ int arch_set_info_hvm_guest(struct vcpu *v, const 
>> vcpu_hvm_context_t *ctx)
>>          v->arch.hvm_vcpu.guest_cr[4] = regs->cr4;
>>          v->arch.hvm_vcpu.guest_efer  = regs->efer;
>>  
>> -#define SEG(l, a) (struct segment_register){ .limit = (l), .attr.bytes = 
>> (a) }
>> +#define SEG(l, a) (struct segment_register){ 0, { (a) }, (l), 0 }
> The parens around a and l are pointless here.

Will do.

>
>> @@ -3832,25 +3832,25 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t 
>> cs, uint16_t ip)
>>      reg.sel = cs;
>>      reg.base = (uint32_t)reg.sel << 4;
>>      reg.limit = 0xffff;
>> -    reg.attr.bytes = 0x09b;
>> +    reg.attr = 0x09b;
>>      hvm_set_segment_register(v, x86_seg_cs, &reg);
>>  
>>      reg.sel = reg.base = 0;
>>      reg.limit = 0xffff;
>> -    reg.attr.bytes = 0x093;
>> +    reg.attr = 0x093;
> Could I talk you into at once dropping the stray inner zeros in both
> cases?

Will do.

>
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>> @@ -83,33 +83,26 @@ struct x86_event {
>>      unsigned long cr2;          /* Only for TRAP_page_fault h/w exception 
>> */
>>  };
>>  
>> -/* 
>> - * Attribute for segment selector. This is a copy of bit 40:47 & 52:55 of 
>> the
>> - * segment descriptor. It happens to match the format of an AMD SVM VMCB.
>> - */
> The reference to segment descriptors here helped understand what ...
>
>> -typedef union segment_attributes {
>> -    uint16_t bytes;
>> -    struct
>> -    {
>> -        uint16_t type:4;    /* 0;  Bit 40-43 */
>> -        uint16_t s:   1;    /* 4;  Bit 44 */
>> -        uint16_t dpl: 2;    /* 5;  Bit 45-46 */
>> -        uint16_t p:   1;    /* 7;  Bit 47 */
>> -        uint16_t avl: 1;    /* 8;  Bit 52 */
>> -        uint16_t l:   1;    /* 9;  Bit 53 */
>> -        uint16_t db:  1;    /* 10; Bit 54 */
>> -        uint16_t g:   1;    /* 11; Bit 55 */
> ... the bit numbers here refer to. Hence I like to ask to either restore
> that reference or ...
>
>>  /*
>>   * Full state of a segment register (visible and hidden portions).
>> - * Again, this happens to match the format of an AMD SVM VMCB.
>> + * Chosen to match the format of an AMD SVM VMCB.
>>   */
>>  struct segment_register {
>>      uint16_t   sel;
>> -    segment_attributes_t attr;
>> +    union {
>> +        uint16_t attr;
>> +        struct {
>> +            uint16_t type:4;    /* 0;  Bit 40-43 */
>> +            uint16_t s:   1;    /* 4;  Bit 44 */
>> +            uint16_t dpl: 2;    /* 5;  Bit 45-46 */
>> +            uint16_t p:   1;    /* 7;  Bit 47 */
>> +            uint16_t avl: 1;    /* 8;  Bit 52 */
>> +            uint16_t l:   1;    /* 9;  Bit 53 */
>> +            uint16_t db:  1;    /* 10; Bit 54 */
>> +            uint16_t g:   1;    /* 11; Bit 55 */
> ... drop the Bit NN comments here, as these aren't bit numbers
> inside the VMCB field layout afaict. With at least this last aspect
> taken care of
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

I think I'd prefer to omit the comments entirely.  The main structure
comment identifies which layout we've chosen to use, and its not like
these bitfield names are obscure in any segment register/descriptor context.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel