Re: [PATCH 02/10] nEPT: MMU context for nested EPT

[Avi Kivity <avi@redhat.com>]

On 11/10/2011 10:05 PM, Nadav Har'El wrote:
> On Thu, Nov 10, 2011, Avi Kivity wrote about "Re: [PATCH 02/10] nEPT: MMU context for nested EPT":
> > This is all correct, but the code in question parses the EPT12 table
> > using the ia32 page table format.  They're sufficiently similar so that
> > it works, but it isn't correct.
> > 
> > Bit 0: EPT readable, ia32 present
> > Bit 1: Writable; ia32 meaning dependent on cr0.wp
> > Bit 2: EPT executable, ia32 user (so, this implementation will interpret
> > a non-executable EPT mapping, if someone could find a use for it, as a
> > L2 kernel only mapping)
> >....
>
> This is a very good point.
>
> I was under the mistaken (?) impression that the page-table shadowing
> code will just copy these bits as-is from the shadowed table (EPT12) to the
> shadow table (EPT02), without caring what they actually mean. 

No, for two reasons.  First, the shadow bits are the result of
multiplexing guest and host permissions, for example either the guest of
host may write-protect a page.  Second, the shadow and guest ptes may be
in different formats (ept vs ia32).

> I knew we had
> a problem when building, not copying, PTEs, and hence the patch to
> link_shadow_page).

In fact that happens to accidentally work, no?  Intermediate ptes are
always present/write/user, which translates to read/write/execute in EPT.

> Also I realized we sometimes need to actually walk the TDP EPT12+cr3 (e.g.,
> to see if an EPT violation is L1's fault), but I thought this was just the
> normal TDP walk, which already knows how to correctly read the EPT
> table.
>
> > walk_addr() will also write to bits 6/7, which the L1 won't expect.
>
> I didn't notice this :(
>
> Back to the drawing board, I guess. I need to figure out exactly what
> needs to be fixed, and how to do this with the least obtrusive changes to
> the existing use case (normal shadow page tables, and nested EPT).

Don't optimize for least changes, optimize for best result afterwards.

We need a third variant of walk_addr_generic that parses EPT format
PTEs.  Whether that's best done by writing paging_ept.h or modifying
paging_tmpl.h, I don't know.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.