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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/11/16 15:25, Jan Beulich wrote:
>>>> 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).

I still don't like the use of ASSERT_UNREACHABLE(); as the "you failed
the typecheck" case.

Would ASSERT(!"%tr typecheck failure") be acceptable?

>
>> +    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"?

Yes.  Sorry.  Fixed.

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