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

["Zhang, Xiantao" <xiantao.zhang@xxxxxxxxx>]

> -----Original Message-----
> From: Tim Deegan [mailto:tim@xxxxxxx]
> Sent: Thursday, December 20, 2012 8:52 PM
> To: Zhang, Xiantao
> Cc: xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx; Nakajima, Jun; Dong, Eddie;
> JBeulich@xxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v3 03/10] nested_ept: Implement guest
> ept's walker
> 
> 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.

Fixed, and will consult host's capability first. 

> > +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?

Okay, although this doesn't fully follow SDM, injecting a EPT misconfiguration 
in this case should be a better way instead of hanging there. 

> > +    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.

Okay, this is also what the version 1 does. 

> > --- 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.


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.

Actually,  it is used in the function nept_rwx_bits_check. :)

Xiantao

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