Re: [Xen-devel] [PATCH v3 03/10] nested_ept: Implement guest ept's walker

[Tim Deegan <tim@xxxxxxx>]

At 23:43 +0800 on 20 Dec (1356047024), Xiantao Zhang wrote:
> From: Zhang Xiantao <xiantao.zhang@xxxxxxxxx>
> 
> Implment guest EPT PT walker, some logic is based on shadow's
> ia32e PT walker. During the PT walking, if the target pages are
> not in memory, use RETRY mechanism and get a chance to let the
> target page back.
> 
> Signed-off-by: Zhang Xiantao <xiantao.zhang@xxxxxxxxx>

This is much nicer than v1, thanks.  I have some comments below, and the
whole thing needs to be checked for whitespace mangling.

> +static bool_t nept_rwx_bits_check(ept_entry_t e) {
> +    /*write only or write/execute only*/
> +    uint8_t rwx_bits = e.epte & EPTE_RWX_MASK;
> +
> +    if ( rwx_bits == ept_access_w || rwx_bits == ept_access_wx )
> +        return 1;
> +
> +    if ( rwx_bits == ept_access_x && !(NEPT_VPID_CAP_BITS &
> +                        VMX_EPT_EXEC_ONLY_SUPPORTED))

In a later patch you add VMX_EPT_EXEC_ONLY_SUPPORTED to this field.  How
can that work when running on a CPU that doesn't support exec-only?  The
nested-ept tables will have exec-only mapping in them which the CPU will
reject.

> +done:
> +    ret = EPT_TRANSLATE_SUCCEED;
> +    goto out;
> +
> +map_err:
> +    if ( rc == _PAGE_PAGED )
> +        ret = EPT_TRANSLATE_RETRY;
> +    else
> +        ret = EPT_TRANSLATE_ERR_PAGE;

What does this error code mean?  The caller just retries the faulting
instruction when it sees it, which sounds wrong.  Why not just return
EPT_TRANSLATE_MISCONFIG if the guest uses an unmappable frame for EPT
tables?

> +    default:
> +        rc = EPT_TRANSLATE_UNSUPPORTED;
> +        gdprintk(XENLOG_ERR, "Unsupported ept translation type!:%d\n", rc);

Just BUG() here and get rid of EPT_TRANSLATE_UNSUPPORTED and
NESTEDHVM_PAGEFAULT_UNHANDLED.  The function that provided rc is right
above and we can see it hasn't got any other return values.

> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -4582,7 +4582,7 @@ static mfn_t emulate_gva_to_mfn(struct vcpu *v,
>      /* Translate the GFN to an MFN */
>      ASSERT(!paging_locked_by_me(v->domain));
>      mfn = get_gfn(v->domain, _gfn(gfn), &p2mt);
> -        
> +

This stray change should be dropped. 

> +typedef enum {
> +    ept_access_n     = 0, /* No access permissions allowed */
> +    ept_access_r     = 1,
> +    ept_access_w     = 2,
> +    ept_access_rw    = 3,
> +    ept_access_x     = 4,
> +    ept_access_rx    = 5,
> +    ept_access_wx    = 6,
> +    ept_access_all   = 7,
> +} ept_access_t;

This enum isn't used anywhere.  

Cheers,

Tim.

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