Re: [Xen-devel] [PATCH 08/15] x86/hvm: Reposition the modification of raw segment data from the VMCB/VMCS

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.11.16 at 16:38, <andrew.cooper3@xxxxxxxxxx> wrote:
> +    case x86_seg_tr:
> +        ASSERT(reg->attr.fields.p);                  /* Usable. */
> +        ASSERT(!reg->attr.fields.s);                 /* System segment. */
> +        ASSERT(!(reg->sel & 0x4));                   /* !TI. */
> +        ASSERT(reg->attr.fields.type == SYS_DESC_tss16_busy ||
> +               reg->attr.fields.type == SYS_DESC_tss_busy);
> +        ASSERT(is_canonical_address(reg->base));
> +        break;

I can't help thinking that the slightly more strict

+        if ( reg->attr.fields.type == SYS_DESC_tss_busy )
+            ASSERT(is_canonical_address(reg->base));
+        else if ( reg->attr.fields.type == SYS_DESC_tss16_busy )
+            ASSERT(!(reg->base >> 32));
+        else
+            ASSERT_UNREACHABLE();

would be better, even if that's going to make TR checking look a
little different than the others (but we should leverage the
information we have).

> +    case x86_seg_ldtr:
> +        if ( reg->attr.fields.p )
> +        {
> +            ASSERT(!reg->attr.fields.s);             /* System segment. */
> +            ASSERT(!(reg->sel & 0x4));               /* !TI. */
> +            ASSERT(reg->attr.fields.type == SYS_DESC_ldt);
> +            ASSERT(is_canonical_address(reg->base));
> +        }
> +        break;
> +
> +    case x86_seg_gdtr:
> +    case x86_seg_idtr:
> +        ASSERT(is_canonical_address(reg->base));
> +        ASSERT((reg->limit >> 16) == 0);             /* Upper bits clear. */
> +        break;
> +
> +    default:
> +        ASSERT_UNREACHABLE();
> +        break;
> +    }

Didn't you agree to change this last "break" to "return"?

> --- a/xen/include/asm-x86/desc.h
> +++ b/xen/include/asm-x86/desc.h
> @@ -89,7 +89,13 @@
>  #ifndef __ASSEMBLY__
>  
>  /* System Descriptor types for GDT and IDT entries. */
> +#define SYS_DESC_tss16_avail  1
>  #define SYS_DESC_ldt          2
> +#define SYS_DESC_tss16_busy   3
> +#define SYS_DESC_call_gate16  4
> +#define SYS_DESC_task_gate    5
> +#define SYS_DESC_irq_gate16   6
> +#define SYS_DESC_trap_gate16  7
>  #define SYS_DESC_tss_avail    9
>  #define SYS_DESC_tss_busy     11
>  #define SYS_DESC_call_gate    12

Thanks for completing the set.

Regardless of how you decide on the two earlier remarks,
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan


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