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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/06/17 14:40, Jan Beulich wrote:
>>>> On 07.06.17 at 15:04, <andrew.cooper3@xxxxxxxxxx> wrote:
>> RFC, because I'd also like to float the idea of making this adjustment as 
>> well:
>>
>> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h 
>> b/xen/arch/x86/x86_emulate/x86_emulate.h
>> index 3355c01..53a5480 100644
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>> @@ -90,7 +90,7 @@ struct x86_event {
>>  struct segment_register {
>>       uint16_t   sel;
>>            union {
>> -        uint16_t raw;
>> +        uint16_t attr;
>>          struct {
>>              uint16_t type:4;    /* 0;  Bit 40-43 */
>>              uint16_t s:   1;    /* 4;  Bit 44 */
>> @@ -102,7 +102,7 @@ struct segment_register {
>>              uint16_t g:   1;    /* 11; Bit 55 */
>>              uint16_t pad: 4;
>>          };
>> -    } attr;
>> +    };
>>      uint32_t   limit;
>>      uint64_t   base;
>>  };
>>
>> which would turn .attr.raw into just .attr, and remove .attr for accesses 
>> into
>> the bitfield.
> I'd certainly like this, provided this works with all compiler versions
> we support (see also below).
>
>> Furthermore, in a following patch, I intend to make a similar adjustment as
>> http://xenbits.xen.org/gitweb/?p=xtf.git;a=commitdiff;h=f099211f2ebdadf61ae6 
>> 416559220d69b788cd2b
>> to expose the internal code/data field names.  This will simplify a lot of
>> code which currently uses opencoded numbers against the type field.
> This is nice too, with two caveats: The "a" bit is not code
> segment specific (but placed so)

Least bad option I'm afraid.  It can't live in the common struct because
it is part of the type nibble, and it can't live in both the code and
data anonymous unions because it has the same name.

>  and the "x" bit is an invention
> of yours afaict, which I'd prefer to be e.g. "code".

It is the eXecutable bit.  I got the terminology from one of the two
manuals, but don't recall exactly where.

>
>> @@ -245,7 +245,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){ .limit = (l), .attr.raw = (a) }
> Does this and ...
>
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -1074,18 +1074,18 @@ static unsigned int _vmx_get_cpl(struct vcpu *v)
>>   * things.  We store the ring-3 version in the VMCS to avoid lots of
>>   * shuffling on vmenter and vmexit, and translate in these accessors. */
>>  
>> -#define rm_cs_attr (((union segment_attributes) {                       \
>> -        .fields = { .type = 0xb, .s = 1, .dpl = 0, .p = 1, .avl = 0,    \
>> -                    .l = 0, .db = 0, .g = 0, .pad = 0 } }).bytes)
>> -#define rm_ds_attr (((union segment_attributes) {                       \
>> -        .fields = { .type = 0x3, .s = 1, .dpl = 0, .p = 1, .avl = 0,    \
>> -                    .l = 0, .db = 0, .g = 0, .pad = 0 } }).bytes)
>> -#define vm86_ds_attr (((union segment_attributes) {                     \
>> -        .fields = { .type = 0x3, .s = 1, .dpl = 3, .p = 1, .avl = 0,    \
>> -                    .l = 0, .db = 0, .g = 0, .pad = 0 } }).bytes)
>> -#define vm86_tr_attr (((union segment_attributes) {                     \
>> -        .fields = { .type = 0xb, .s = 0, .dpl = 0, .p = 1, .avl = 0,    \
>> -                    .l = 0, .db = 0, .g = 0, .pad = 0 } }).bytes)
>> +#define rm_cs_attr (((struct segment_register) {                        \
>> +        .attr = { .type = 0xb, .s = 1, .dpl = 0, .p = 1, .avl = 0,      \
>> +                    .l = 0, .db = 0, .g = 0, .pad = 0 } }).attr.raw)
>> +#define rm_ds_attr (((struct segment_register) {                        \
>> +        .attr = { .type = 0x3, .s = 1, .dpl = 0, .p = 1, .avl = 0,      \
>> +                  .l = 0, .db = 0, .g = 0, .pad = 0 } }).attr.raw)
>> +#define vm86_ds_attr (((struct segment_register) {                      \
>> +        .attr = { .type = 0x3, .s = 1, .dpl = 3, .p = 1, .avl = 0,      \
>> +                  .l = 0, .db = 0, .g = 0, .pad = 0 } }).attr.raw)
>> +#define vm86_tr_attr (((struct segment_register) {                      \
>> +        .attr = { .type = 0xb, .s = 0, .dpl = 0, .p = 1, .avl = 0,      \
>> +                  .l = 0, .db = 0, .g = 0, .pad = 0 } }).attr.raw)
> ... all of this work with gcc around the 4.3 era?

Not sure.  I will try.

I had actually wondered why we weren't using plain numbers for these. 
If this doesn't work for GCC 4.3 and older, I probably will swap these
to being raw numbers, rather than lose the other benefits.

~Andrew

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