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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

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

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

Jan


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