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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 11/23/2016 10:38 AM, Andrew Cooper wrote:
> Intel VT-x and AMD SVM provide access to the full segment descriptor cache via
> fields in the VMCB/VMCS.  However, the bits which are actually checked by
> hardware and preserved across vmentry/exit are inconsistent, and the vendor
> accessor functions perform inconsistent modification to the raw values.
>
> Convert {svm,vmx}_{get,set}_segment_register() into raw accessors, and alter
> hvm_{get,set}_segment_register() to cook the values consistently.  This allows
> the common emulation code to better rely on finding architecturally-expected
> values.
>
> This does cause some functional changes because of the modifications being
> applied uniformly.  A side effect of this fixes latent bugs where
> vmx_set_segment_register() didn't correctly fix up .G for segments, and
> inconsistent fixing up of the GDTR/IDTR limits.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
> CC: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> ---
>  xen/arch/x86/hvm/hvm.c        | 151 
> ++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/hvm/svm/svm.c    |  20 +-----
>  xen/arch/x86/hvm/vmx/vmx.c    |   6 +-
>  xen/include/asm-x86/desc.h    |   6 ++
>  xen/include/asm-x86/hvm/hvm.h |  17 ++---
>  5 files changed, 164 insertions(+), 36 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index ef83100..804cd88 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -6051,6 +6051,157 @@ void hvm_domain_soft_reset(struct domain *d)
>  }
>  
>  /*
> + * Segment caches in VMCB/VMCS are inconsistent about which bits are checked,
> + * important, and preserved across vmentry/exit.  Cook the values to make 
> them
> + * closer to what is architecturally expected from entries in the segment
> + * cache.
> + */
> +void hvm_get_segment_register(struct vcpu *v, enum x86_segment seg,
> +                              struct segment_register *reg)
> +{
> +    hvm_funcs.get_segment_register(v, seg, reg);
> +
> +    switch ( seg )
> +    {
> +    case x86_seg_ss:
> +        /* SVM may retain %ss.DB when %ss is loaded with a NULL selector. */
> +        if ( !reg->attr.fields.p )
> +            reg->attr.fields.db = 0;

Removed SVM code relied on type being zero to indicate a NULL segment.
Looking at P bit is the correct way and I think it's worth mentioning
this in the commit message.


> +}
> +
> +void hvm_set_segment_register(struct vcpu *v, enum x86_segment seg,
> +                              struct segment_register *reg)
> +{
> +    /* Set G to match the limit field.  VT-x cares, while SVM doesn't. */
> +    if ( reg->attr.fields.p )
> +        reg->attr.fields.g = !!(reg->limit >> 20);
> +
> +    switch ( seg )
> +    {
> +    case x86_seg_cs:
> +        ASSERT(reg->attr.fields.p);                  /* Usable. */
> +        ASSERT(reg->attr.fields.s);                  /* User segment. */
> +        ASSERT((reg->base >> 32) == 0);              /* Upper bits clear. */
> +        break;
> +
> +    case x86_seg_ss:
> +        if ( reg->attr.fields.p )
> +        {
> +            ASSERT(reg->attr.fields.s);              /* User segment. */
> +            ASSERT(!(reg->attr.fields.type & 0x8));  /* Data segment. */
> +            ASSERT(reg->attr.fields.type & 0x2);     /* Writeable. */
> +            ASSERT((reg->base >> 32) == 0);          /* Upper bits clear. */
> +        }
> +        break;

SVM requires attributes of any NULL segment to be zero. I don't know
about Intel but if it's the same then should we ASSERT this as well?


-boris


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