Re: [PATCH] Fix up vmx_set_segment for booting older guests.

[Chris Lalancette <clalance@redhat.com>]

Avi Kivity wrote:
>> +	else
>> +		vmcs_writel(sf->base, var->base);
>>   	vmcs_write32(sf->limit, var->limit);
>>    
> 
> I think the correct fix is to zero extend in vmcs_writel() rather than 
> here.  But as far as I can tell, it already does.  Where does the sign 
> extension occur?  Perhaps in userspace?

Very good question Avi.  I should have dug a bit deeper before posting.  I
traced this further back, and here's what it looks like is going on:

arch/x86/kvm/x86.c:kvm_load_segment_descriptor() is responsible for loading the
CPU segment descriptor into the VMCS area.  It does this by calling
load_segment_descriptor_to_kvm_desct(), doing a few minor transformations of the
data, then calling kvm_set_segment() to load it into the VMCS.

The problem arises in load_segment_descriptor_to_kvm_desct() ->
seg_desct_to_kvm_desct().  seg_desct_to_kvm_desct() takes the struct desc_struct
(in this case, base0 == 0x0, base1 == 0x0, and base2 == 0xc0), then calls
get_desc_base() and stores the result in the struct kvm_segment.  The return
value from get_desc_base is It's here that the sign-extension occurs, which
eventually causes that VM entry failure.

get_desc_base() sign-extends because of some complicated u8 to unsigned rules
that I'm not completely sure of.  The below patch fixes my original issue, but
I'm not at all sure that this is the right thing to do.  I could also change
get_desc_base() itself to do the casting, which should do the right thing for
all callers, but I'm not sure if that's what all callers want.  Anybody else
have an opinion?


diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a93ba29..b58bda2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3997,7 +3997,7 @@ static void kvm_set_segment(struct kvm_vcpu *vcpu,
 static void seg_desct_to_kvm_desct(struct desc_struct *seg_desc, u16 selector,
                                   struct kvm_segment *kvm_desct)
 {
-       kvm_desct->base = get_desc_base(seg_desc);
+       kvm_desct->base = (unsigned)get_desc_base(seg_desc);
        kvm_desct->limit = get_desc_limit(seg_desc);
        if (seg_desc->g) {
                kvm_desct->limit <<= 12;


-- 
Chris Lalancette