Re: [Xen-devel] [V11 PATCH 09/21] PVH xen: domain create, context switch related code changes

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.08.13 at 03:18, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> Changes in V11:
>   - set cr3 to page_to_maddr and not page_to_mfn.
>   - reject non-zero cr1 value for pvh.
>   - Do not check for pvh in destroy_gdt, but put the check in callers.
>   - Set _VPF_in_reset for PVH also.

These are clearly too many changes to retain...

> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> Acked-by: Keir Fraser <keir@xxxxxxx>
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Tim Deegan <tim@xxxxxxx>
> PV-HVM-Regression-Tested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

... all these tags. This practice of yours was already questionable
on one or two earlier patches, but I guess we can tolerate it there.

> @@ -892,8 +896,19 @@ int arch_set_info_guest(
>          /* handled below */;
>      else if ( !compat )
>      {
> +        /* PVH 32bitfixme. */
> +        if ( is_pvh_vcpu(v) )
> +        {
> +            v->arch.cr3 = page_to_maddr(cr3_page);
> +            v->arch.hvm_vcpu.guest_cr[3] = c.nat->ctrlreg[3];
> +        }
> +
>          v->arch.guest_table = pagetable_from_page(cr3_page);
> -        if ( c.nat->ctrlreg[1] )
> +
> +        if ( c.nat->ctrlreg[1] && is_pvh_vcpu(v) )
> +            rc = -EINVAL;
> +
> +        if ( c.nat->ctrlreg[1] && is_pv_vcpu(v) )
>          {
>              cr3_gfn = xen_cr3_to_pfn(c.nat->ctrlreg[1]);
>              cr3_page = get_page_from_gfn(d, cr3_gfn, NULL, P2M_ALLOC);

To many changes to if() conditions. I think it could be brought down
to

    else if ( !compat )
    {
        v->arch.guest_table = pagetable_from_page(cr3_page);

        /* PVH 32bitfixme. */
        if ( is_pvh_vcpu(v) )
        {
            v->arch.cr3 = page_to_maddr(cr3_page);
            v->arch.hvm_vcpu.guest_cr[3] = c.nat->ctrlreg[3];
            if ( c.nat->ctrlreg[1] )
                rc = -EINVAL;
        }
        else if ( c.nat->ctrlreg[1] )
        {
            cr3_gfn = xen_cr3_to_pfn(c.nat->ctrlreg[1]);
            cr3_page = get_page_from_gfn(d, cr3_gfn, NULL, P2M_ALLOC);

making the flow much more clear (at least to me).

> @@ -953,6 +969,13 @@ int arch_set_info_guest(
>  
>      update_cr3(v);
>  
> +    if ( is_pvh_vcpu(v) )
> +    {
> +        /* Set VMCS fields. */
> +        if ( (rc = pvh_vcpu_boot_set_info(v, c.nat)) != 0 )
> +            return rc;
> +    }

Combine the two if()-s and drop or generalize the VMX-specific
comment.

Jan


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