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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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) and the "x" bit is an invention
of yours afaict, which I'd prefer to be e.g. "code".

> @@ -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?

Jan

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