Re: [PATCH v14 11/17] pvh: Set up more PV stuff in set_info_guest

[George Dunlap <george.dunlap@eu.citrix.com>]

On 04/11/13 16:53, Jan Beulich wrote:
>>>> On 04.11.13 at 13:15, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>> @@ -728,8 +740,21 @@ int arch_set_info_guest(
>>   
>>       if ( has_hvm_container_vcpu(v) )
>>       {
>> -        hvm_set_info_guest(v);
>> -        goto out;
>> +        hvm_set_info_guest(v, compat ? 0 : c.nat->gs_base_kernel);
> I'm afraid this isn't correct - so far gs_base_kernel didn't get used
> for HVM guests, i.e. you're changing behavior here (even if only
> in a - presumably - benign way).
>
>> +
>> +        if ( is_hvm_vcpu(v) || v->is_initialised )
>> +            goto out;
>> +
>> +        cr3_gfn = xen_cr3_to_pfn(c.nat->ctrlreg[3]);
> I'd recommend against using this PV construct - the 32-bit
> counterpart won't be correct to be used here once 32-bit
> support gets added.

So the plan would be that once we support 32-bit, I'd just copy the code 
from below:

     if ( !compat )
         cr3_gfn = xen_cr3_to_pfn(c.nat->ctrlreg[3]);
     else
         cr3_gfn = compat_cr3_to_pfn(c.cmp->ctrlreg[3]);

But since we know that compat is false here, it seems a bit silly to 
have the if() statement.

But there should be a "PVH 32bitfixme" here -- is that enough for now?

>
>> @@ -1426,6 +1426,11 @@ static void vmx_set_info_guest(struct vcpu *v)
>>           __vmwrite(GUEST_INTERRUPTIBILITY_INFO, intr_shadow);
>>       }
>>   
>> +    /* PVH 32bitfixme */
>> +    if ( is_pvh_vcpu(v) )
>> +        __vmwrite(GUEST_GS_BASE, gs_base_kernel);
> Oh, I see, you suppress this here. I'd really suggest adjusting the
> caller, then you don't need to do anything here afaict.

What do you mean "adjusting the caller"?  What we want for HVM guests is 
for this field to be entirely left alone, isn't it?

If we set GUEST_GS_BASE unconditionally here, the only way to effect "no 
change" is to read it and pass in the existing value, which seems kind 
of pointless.

  -George