[Xen-devel] RE: [PATCH][XEN-PAGING][2/2] Enable hardware assisted paging and AMD-V nested paging support

["Huang2, Wei" <Wei.Huang2@xxxxxxx>]

Tim,

I fixed the patch based on your comments. The changes are listed as
following:

1. It calls is_hvm_domain()/is_hvm_vcpu() for checking before hap is
turned on.
2. It always calls shadow_vcpu_init().
3. It moves npt_detect() to svm.c.
4. Instead of calling npt_update_paging_mode(), it uses
hvm_paging_enabled(), hvm_pae_enabled(), hvm_long_mode_enabled() to
update guest's mode. These functions are not architecture dependent.
5. It moves page-guest32.h to upper directory. This file is shared by
both shadow code and hap code.
6. I also cleaned up the code to fix minor issues.

I have tested the code and it did not break shadow code.

Thanks,

-Wei

-----Original Message-----
From: Tim Deegan [mailto:Tim.Deegan@xxxxxxxxxxxxx] 
Sent: Tuesday, February 27, 2007 5:31 AM
To: Huang2, Wei
Cc: Xen Devel; Tim Deegan
Subject: Re: [PATCH][XEN-PAGING][2/2] Enable hardware assisted paging
and AMD-V nested paging support

Hi, 

Thanks for your patch.  Brief comments inline:

Content-Description: hap_npt_patch.txt
> +static int svm_do_nested_pgfault(unsigned long va, struct
cpu_user_regs *regs)
> +{
> +    unsigned long gpa = va;
> +    if (mmio_space(gpa)) {
> +        handle_mmio(gpa);
> +        return 1;
> +    }
> +
> +    /* P2M table needs to be fixed. */
> +    return paging_fault(va, regs);
> +}
> +

#VMEXIT(NPF) returns a guest physical address, which (a) might be longer
than an unsigned long (PAE guest on PAE host) and (b) probably shouldn't
be called "va" in the prototype. :)

You'll either need to extend paging_fault() to take a wider type 
or (since your current implementation calls domain_crash()
unconditionally) just not call it at all.

>  void paging_domain_init(struct domain *d)
>  {
>      p2m_init(d);
> -    shadow_domain_init(d);
> +
> +    if ( opt_hap_enabled && hap_capable_system )
> +        hap_domain_init(d);
> +    else
> +        shadow_domain_init(d);
>  }

shadow_domain_init() should be called regardless of whether the domain
will be shadowed; it just sets up some locks and list_heads.

( opt_hap_enabled && hap_capable_system ) is not enough to detect
whether to use HAP; you need to test is_hvm_domain(d) as well.

>  /* vcpu paging struct initialization goes here */
>  void paging_vcpu_init(struct vcpu *v)
>  {
> -    shadow_vcpu_init(v);
> +    if ( opt_hap_enabled && hap_capable_system )
> +        hap_vcpu_init(v);
> +    else
> +        shadow_vcpu_init(v);
>  }

... && is_hvm_vcpu(v).  Likewise in the rest of the file.

> +    if ( unlikely(d == current->domain) ) {
> +        gdprintk(XENLOG_INFO, "Don't try to do a npt op on
yourself!\n");
> +        return -EINVAL;
> +    }

ITYM "a HAP op". :)

> +void hap_update_paging_modes(struct vcpu *v)
> +{
> +    struct domain *d;
> +
> +    HERE_I_AM;
> +
> +    d = v->domain;
> +    hap_lock(d);
> +
> +    /* update paging mode based on guest state */
> +    npt_update_guest_paging_mode(v);

This is in an architecture-independent file: there needs to be another
layer of indirection here.  Plumb through as a hvm_ function?

> +/************************************************/
> +/*          AMD NESTED PAGING FEATURES          */
> +/************************************************/
> +void npt_detect(void)
> +{
> +    u32 eax, ebx, ecx, edx;
> +
> +    /* check CPUID for nested paging support */
> +    cpuid(0x8000000A, &eax, &ebx, &ecx, &edx);
> +    if ( edx & 0x01 ) { /* nested paging */
> +        hap_capable_system = 1;
> +    }
> +    else if ( opt_hap_enabled ) {
> +        printk(" nested paging is not supported by this CPU.\n");
> +        hap_capable_system = 0; /* no nested paging, we disable flag.
*/
> +    }
> +}
> +
> +/* update paging mode pointer for AMD nested paging */
> +void npt_update_guest_paging_mode(struct vcpu *v)
> +{
> +    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> +    u64 cr0_value = vmcb->cr0;
> +    u64 cr4_value = vmcb->cr4;
> +    u64 efer_value = vmcb->efer;
> +
> +    if ( (cr0_value & X86_CR0_PE) && (cr0_value & X86_CR0_PG)) {
> +        if ( (efer_value & EFER_LME) && (cr4_value & X86_CR4_PAE) )
> +            v->arch.paging.mode = &hap_paging_long_mode;
> +        else if ( cr4_value & X86_CR4_PAE)
> +            v->arch.paging.mode = &hap_paging_pae_mode;
> +        else
> +            v->arch.paging.mode = &hap_paging_protected_mode;
> +    }
> +    else {
> +        v->arch.paging.mode = &hap_paging_real_mode;
> +    }
> +}

These should be in their own file, or in arch/x86/hvm/svm/ somewhere.

> +#define NPT_GUEST_CR3_SHIFT_NON_PAE 12 /* both legacy mode and long
mode */
> +#define NPT_GUEST_CR3_SHIFT_PAE 5 /* PAE mode */

Call these HAP_GUEST_CR3_SHIFT_*?  Or, since you don't use them, leave
them out.

> +#include "../shadow/page-guest32.h"

Move this file into arch/x86/mm/ if it's going to be shared.

Cheers,

Tim.

P.S.  Another quick question: have you looked at virtualizing gate A20?

-- 
Tim Deegan <Tim.Deegan@xxxxxxxxxxxxx>, XenSource UK Limited
Registered office c/o EC2Y 5EB, UK; company number 05334508

Attachment: hap_npt_patch.txt
Description: hap_npt_patch.txt

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel