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

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

On 07/11/13 16:10, Jan Beulich wrote:
>>>> On 07.11.13 at 16:51, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>> 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?
> Sure, but that wasn't my point. I was recommending against
> *_cr3_to_pfn() altogether here. Just use the control register
> value shifted right by 12 bits.

Oh, right, I see.

>
>>>> @@ -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.
> Oh, right, I didn't pay attention to the calling path also being
> used for HVM, and us not wanting to write zero here in that
> case. Or maybe I did, but concluded that the code can be used
> only for initial state setup, in which case writing zero would be
> benign.

I guess we could do that... but since we're talking about changing the 
interface here anyway, it's kind of nitpicking at this point.

  -George