Re: [Xen-devel] [PATCH v1] Fix p2m_set_suppress_ve

[Tamas K Lengyel <tamas.k.lengyel@xxxxxxxxx>]

On Wed, Apr 3, 2019 at 10:06 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
> >>> On 03.04.19 at 17:48, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> > On 4/3/19 6:30 PM, Jan Beulich wrote:
> >>>>> On 03.04.19 at 17:17, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> >>> Changes to the hostp2m (also known as altp2m view 0) propagate to all
> >>> existing altp2ms, but they do so in a lazy manner, and also that won't
> >>> happen for altp2ms created after a while. So altp2ms will not
> >>> necessarily know about a page that the hostp2m knows about, which should
> >>> not stop us from setting mem access restrictions or the value of the SVE
> >>> bit.
> >>
> >> But even if the propagation is lazy, a "get" should then return the
> >> propagated value, shouldn't it? Or else callers (like is the case here)
> >> can be mislead. I do recognize that there may be callers who
> >> intentionally _don't_ want the propagated value, but I'd expect this
> >> to be the exception, not the rule. I wonder therefore whether there
> >> shouldn't be a wrapper around ->get_entry() to take care of this for
> >> callers which want the propagated value. Calling the bare hook would
> >> remain as is.
> >
> > But I believe that some hostp2m entries will never be propagated. Sorry
> > that I've not been clear about this. This is what I meant by "won't
> > happen for altp2ms created after a while".
> >
> > Take, for example, the case where there's only the hostp2m for the first
> > 30 minutes of a guest's life, and in this time somebody calls
> > ept_set_entry(). This, in turn, calls p2m_altp2m_propagate_change() if (
> > entry_written && p2m_is_hostp2m(p2m) ).
> >
> > But at this point there are no altp2ms, so there's nowhere to propagate
> > these changes to.
> >
> > Now, at minute 31 we create a new altp2m. The changes will never be
> > propagated here, so altp2m->get_entry() will always (rightfully) return
> > an invalid MFN.
>
> I guess I'm missing something here: Why "rightfully"? If the guest
> runs on this altp2m, it'll die a quick death then, won't it? Yet if
> this is intended to be demand-populate, then why would there be
> automatic propagation for existing altp2m-s (as you explain
> further up, unless I'm getting this wrong)?
>
> > We only want to make an exception for setting the SVE bit or mem access
> > restrictions on an entry in the altp2m, but having get_entry() (or a
> > wrapper) return the hostp2m values and MFN might not necessarily be what
> > current callers expect.
>
> Then I still don't see why you would want to force propagation
> in these cases: If it's generally demand-populate, it can't be right
> to _fully_ populate that slot. You ought to be setting the access
> type or the "suppress #VE" bit alone, without propagating the
> MFN and alike. The later, when the entry actually gets propagated,
> the entered values should be used as overrides to what comes
> from the host p2m.

It is right to fully populate a slot when for example we want
different mem_access permissions in different altp2m views. We can't
wait for the domain to on-demand populate the altp2m view because we
don't know when (and if) that will actually happen. So
p2m_set_altp2m_mem_access already copies the entry over from the
hostp2m to the altp2m and then applies the requested mem_access
setting. This is done even if the mem_access setting is the same as it
was in the hostp2m.

Doing the same behavior for p2m_set_suppress_ve I think is consistent,
albeit you could easily overcome the issue by first simply setting a
mem_access permission on the gfn to trigger the aforementioned copy to
the altp2m before you try to set the ve settings.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel