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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.11.13 at 16:51, George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
> On 04/11/13 16:53, Jan Beulich wrote:
>>>>> On 04.11.13 at 13:15, George Dunlap <george.dunlap@xxxxxxxxxxxxx> 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.

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

Jan


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