Re: [Xen-devel] [PATCH] nestedsvm: fix l2 guest display refresh issue

[Tim Deegan <tim@xxxxxxx>]

At 15:36 +0200 on 06 Jul (1341588980), Christoph Egger wrote:
> Fix l2 guest refresh problem.
> When l2 guest does a write access the l1 hypervisor
> mapped read only then inject VMEXIT(#NPF) into l1
> hypervisor.
> When l2 guest does a write access the host mapped
> as read only then let the host handle the NPF.
> Signed-off-by: Christoph Egger <Christoph.Egger@xxxxxxx>

Thanks for the fix, but I see two problems with this patch:

First, I think that trying to extract the flags from the page walk this
way is the wrong approach.  Everywhere else, we pass a PFEC in to the
walker and let the walker handle the access control logic correctly;
its return code is the set of flags that were missing at _any_ level in
the walk, not just the leaf entry.

In fact nestedhap_walk_L1_p2m() already passes a PFEC in to the walker,
and from my reading of it, passes an uninitialized value, so presumably
it's only luck that you haven't been seeing spurious failures already.

So you could pass the r/w/x bits in to nestedhvm_hap_nested_page_fault()
as you do below, and then have nestedhvm_hap_nested_page_fault() or
nestedhap_walk_L1_p2m() make the right PFEC to pass to the walker.  Do
you think that would work?

 
Second, you've made a bunch of changes to p2m_type_to_flags(); it looks
like an attempt to support p2m_access on AMD, which is excellent but
doesn't relate to the patch description, and is probably too late for
4.2.  Can you split it out into a separate patch, please?

Also, it's not correct.  The EPT implementation of this feature stores
the access type in some available bits of the EPT entry, so it can be
recovered later.  Your p2m_flags_to_access() tries to infer the access
type from the actual access-control bits, but there's not enough
information there to do that -- an entry might have _PAGE_RW clear based
on its p2mt, even if the p2ma is p2m_access_rw, and a magic access type
like p2m_access_n2rwx can't be distinguished from p2m_access_n.  So
you'll need to find 4 bits to stash the p2ma in.  And I think this runs
up against the problem we had with p2mt -- if you're using an IOMMU you
can't use bits 9-11 or 52-62 in a present entry.  So if you want to
supprt the p2m_access_type logic, you have to get rid of the shared
NPT/IOMMU tables.

The change to add _PAGE_NX to grant-map types seems like a good one that
could be split out and go in separately; it brings this code in line
with PV and EPT.  But I think it should use _PAGE_NX_BIT rather than
defining a new flag.

Cheers,

Tim.

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