[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] 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. > +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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |