Re: [Xen-devel] [PATCH 09/17] PVH xen: create PVH vmcs, and also initialization

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.04.13 at 23:25, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> This patch mainly contains code to create a VMCS for PVH guest, and HVM
> specific vcpu/domain creation code.
> 
> Changes in V2:
>   - Avoid call to hvm_do_resume() at call site rather than return in it.
>   - Return for PVH vmx_do_resume prior to intel debugger stuff.
> 
> Changes in V3:
>   - Cleanup pvh_construct_vmcs().
>   - Fix formatting in few places, adding XENLOG_G_ERR to printing.
>   - Do not load the CS selector for PVH here, but try to do that in Linux.
> 
> Changes in V4:
>   - Remove VM_ENTRY_LOAD_DEBUG_CTLS clearing.
>   - Add 32bit kernel changes mark.
>   - Verify pit_init call for PVH.

Verify in what way?

> +static int pvh_vcpu_initialise(struct vcpu *v)
> +{
> +    int rc;
> +
> +    if ( (rc = hvm_funcs.vcpu_initialise(v)) != 0 )
> +        return rc;
> +
> +    softirq_tasklet_init(&v->arch.hvm_vcpu.assert_evtchn_irq_tasklet,
> +                         (void(*)(unsigned long))hvm_assert_evtchn_irq,
> +                         (unsigned long)v);
> +
> +    v->arch.hvm_vcpu.hcall_64bit = 1;    /* PVH 32bitfixme */
> +    v->arch.user_regs.eflags = 2;
> +    v->arch.hvm_vcpu.inject_trap.vector = -1;
> +
> +    if ( (rc = hvm_vcpu_cacheattr_init(v)) != 0 )
> +    {
> +        hvm_funcs.vcpu_destroy(v);
> +        return rc;
> +    }
> +    if ( v->vcpu_id == 0 )
> +        pit_init(v, cpu_khz);

I'm asking in particular because my understanding of "verify" would
be checking of an eventual return value...

> @@ -4512,6 +4582,8 @@ static int hvm_memory_event_traps(long p, uint32_t 
> reason,
>  
>  void hvm_memory_event_cr0(unsigned long value, unsigned long old) 
>  {
> +    if ( is_pvh_vcpu(current) )
> +        return;
>      hvm_memory_event_traps(current->domain->arch.hvm_domain
>                               .params[HVM_PARAM_MEMORY_EVENT_CR0],
>                             MEM_EVENT_REASON_CR0,

So these checks are still there, with no mark of being temporary,
despite having pointed out that they ought to be unnecessary once
full PVH support is there. As with the 32-bit specific changes that
the code currently lacks, such temporary adjustments should be
marked clearly and completely, so subsequently one can locate
them _all_. Just consider what happens if after phase I you get
taken off this project, and someone else would have to complete
it.

> +    /* Host control registers. */
> +    v->arch.hvm_vmx.host_cr0 = read_cr0() | X86_CR0_TS;
> +    __vmwrite(HOST_CR0, v->arch.hvm_vmx.host_cr0);
> +    __vmwrite(HOST_CR4,
> +              mmu_cr4_features | (xsave_enabled(v) ? X86_CR4_OSXSAVE : 0));

Didn't pay attention to this before: Why is this conditional needed?
mmu_cr4_features ought to have X86_CR4_OSXSAVE set/cleared
correctly - after all, set_in_cr4(X86_CR4_OSXSAVE) is supposed to
ensure exactly that.

> +static int pvh_check_requirements(struct vcpu *v)
> +{
> +    u64 required, tmpval = real_cr4_to_pv_guest_cr4(mmu_cr4_features);
> +
> +    if ( !paging_mode_hap(v->domain) )
> +    {
> +        dprintk(XENLOG_G_ERR, "HAP is required for PVH guest.\n");
> +        return -EINVAL;

The use of dprintk() appears to be misguided here (and in most
pre-existing places, so please don't use those as reference. My
take on this is that it should be used when an otherwise
ambiguous message is being printed solely for debugging
purposes.

Furthermore the log level of these messages surely shouldn't be
"error" - the tools are supposed to handle the returned error with
due verbosity, and hence the messages here are really just for
debugging purposes, i.e. not higher than XENLOG_G_INFO (i.e.
hidden entirely by default).

> +    guest_pat = MSR_IA32_CR_PAT_RESET;
> +    __vmwrite(GUEST_PAT, guest_pat);

What's the point of having the local variable "guest_pat" here?

> @@ -916,30 +1183,7 @@ static int construct_vmcs(struct vcpu *v)
>          __vmwrite(GUEST_INTR_STATUS, 0);
>      }
>  
> -    /* Host data selectors. */
> -    __vmwrite(HOST_SS_SELECTOR, __HYPERVISOR_DS);
> -    __vmwrite(HOST_DS_SELECTOR, __HYPERVISOR_DS);
> -    __vmwrite(HOST_ES_SELECTOR, __HYPERVISOR_DS);
> -    __vmwrite(HOST_FS_SELECTOR, 0);
> -    __vmwrite(HOST_GS_SELECTOR, 0);
> -    __vmwrite(HOST_FS_BASE, 0);
> -    __vmwrite(HOST_GS_BASE, 0);
> -
> -    /* Host control registers. */
> -    v->arch.hvm_vmx.host_cr0 = read_cr0() | X86_CR0_TS;
> -    __vmwrite(HOST_CR0, v->arch.hvm_vmx.host_cr0);
> -    __vmwrite(HOST_CR4,
> -              mmu_cr4_features | (xsave_enabled(v) ? X86_CR4_OSXSAVE : 0));

Oh, I see this had been this way already, dating back to c/s
20260:ad35f39e5fdc - Dexuan - what was the point here? The host
CR4 value depending on guest properties (even if only apparently,
since xsave_enabled() simply returns cpu_has_xsave, along with
doing a few assertions) seems conceptually wrong.

> @@ -1259,8 +1503,10 @@ void vmx_do_resume(struct vcpu *v)
>  
>          vmx_clear_vmcs(v);
>          vmx_load_vmcs(v);
> -        hvm_migrate_timers(v);
> -        hvm_migrate_pirqs(v);
> +        if ( !is_pvh_vcpu(v) ) {

Formatting.


> @@ -110,6 +116,12 @@ static int vmx_vcpu_initialise(struct vcpu *v)
>  
>      vpmu_initialise(v);
>  
> +    if (is_pvh_vcpu(v) ) 

Ditto.

> @@ -1031,6 +1043,27 @@ static void vmx_update_host_cr3(struct vcpu *v)
>      vmx_vmcs_exit(v);
>  }
>  
> +/*
> + * PVH guest never causes CR3 write vmexit. This called during the guest 
> setup.
> + */
> +static void vmx_update_pvh_cr(struct vcpu *v, unsigned int cr)
> +{
> +    vmx_vmcs_enter(v);
> +    switch ( cr )
> +    {
> +        case 3:
> +            __vmwrite(GUEST_CR3, v->arch.hvm_vcpu.guest_cr[3]);
> +            hvm_asid_flush_vcpu(v);
> +            break;
> +
> +        default:
> +            dprintk(XENLOG_ERR,
> +                   "PVH: d%d v%d unexpected cr%d update at rip:%lx\n",
> +                   v->domain->domain_id, v->vcpu_id, cr, 
> __vmread(GUEST_RIP));
> +    }

Again. Even if not written down formally, the fundamental rule is:
"In general you should copy the style of the surrounding code. If
you are unsure please ask." And surrounding code here is making
clear that the case statements ought to have the same indentation
as the switch one.

Please go over your patches before submitting to eliminate all
formatting issues.

Jan

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