Re: [PATCH RFC v12 15/21] pvh: Set up more PV stuff in set_info_guest

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

On 18/09/13 16:17, Jan Beulich wrote:
>>>> On 13.09.13 at 18:25, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>> @@ -737,7 +737,31 @@ int arch_set_info_guest(
>>       if ( has_hvm_container_vcpu(v) )
>>       {
>>           hvm_set_info_guest(v);
>> -        goto out;
>> +
>> +        if ( is_hvm_vcpu(v) || v->is_initialised )
>> +            goto out;
>> +
>> +        /* PVH */
>> +        if ( c.nat->ctrlreg[1] )
>> +            return -EINVAL;
>> +
>> +        ASSERT(!compat);
> This lacks a pvh-32bit-fixme annotation.

Ack.

>
>> +
>> +        cr3_gfn = xen_cr3_to_pfn(c.nat->ctrlreg[3]);
>> +        cr3_page = get_page_from_gfn(d, cr3_gfn, NULL, P2M_ALLOC);
>> +
>> +        v->arch.cr3 = page_to_maddr(cr3_page);
>> +        v->arch.hvm_vcpu.guest_cr[3] = c.nat->ctrlreg[3];
>> +
>> +        if ( paging_mode_enabled(d) )
> Is the opposite really possible? I think this ought to be an
> assertion.

No, it shouldn't be.  I thought about doing it, but at some point I had 
made so many substantive changes that I decided to be more 
conservative.  I'll change it to an assert.

>
>> +int vmx_pvh_vcpu_boot_set_info(struct vcpu *v,
>> +                               struct vcpu_guest_context *ctxtp)
>> +{
>> +    if ( ctxtp->ldt_base || ctxtp->ldt_ents ||
>> +         ctxtp->user_regs.cs || ctxtp->user_regs.ss || ctxtp->user_regs.es ||
>> +         ctxtp->user_regs.ds || ctxtp->user_regs.fs || ctxtp->user_regs.gs ||
>> +         *ctxtp->gdt_frames || ctxtp->gdt_ents ||
> Don't know why I didn't spot this earlier, but the gdt_frames check
> is pointless when gdt_ents is verified to be zero.

You know, looking at this again -- is there a reason we can't just put 
this in hvm_set_info_guest()?  It's already only called from that one 
place, only a few lines before.  There doesn't really seem to be a need 
to have yet another function for just a few lines.

>
>> --- a/xen/include/public/arch-x86/xen.h
>> +++ b/xen/include/public/arch-x86/xen.h
>> @@ -150,6 +150,10 @@ typedef uint64_t tsc_timestamp_t; /* RDTSC timestamp */
>>   /*
>>    * The following is all CPU context. Note that the fpu_ctxt block is filled
>>
>>    * in by FXSAVE if the CPU has feature FXSR; otherwise FSAVE is used.
>> + *
>> + * PVH 64bit: In the vcpu boot path, for vmcs context, only gs_base_kernel
>> + *            is honored. Other fields like gdt, ldt, and selectors must be
>> + *            zeroed. See vmx_pvh_vcpu_boot_set_info.
> In the current form I hope the comment is misleading: Surely
> general purpose and FPU registers get honored too?
>
> And referring to an internal function from a public header is sort
> of bogus too.

In any case the comment about how a hypercall behaves should be near the 
hypercall.  I'll move this over.

  -George