Re: [PATCH v2 2/3] x86/altp2m: p2m_altp2m_get_or_propagate() should honor present page order

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Tue, Jan 4, 2022 at 11:14 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 04.01.2022 16:00, Tamas K Lengyel wrote:
> > On Tue, Jan 4, 2022 at 4:48 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>
> >> Prior to XSA-304 the only caller merely happened to not use any further
> >> the order value that it passes into the function. Already then this was
> >> a latent issue: The function really should, in the "get" case, hand back
> >> the order the underlying mapping actually uses (or actually the smaller
> >> of the two), such that (going forward) there wouldn't be any action on
> >> unrelated mappings (in particular ones which did already diverge from
> >> the host P2M).
> >>
> >> Similarly in the "propagate" case only the smaller of the two orders
> >> should actually get used for creating the new entry, again to avoid
> >> altering mappings which did already diverge from the host P2M.
> >
> > I don't really understand the reason why you want to return the
> > page_order from the altp2m here. The only check that uses the
> > page_order following is the super-page shattering check for XSA-304
> > but that's done on the hostp2m. So you would want to know what the
> > page_order is on the hosp2m, not the altp2m, no?
>
> From what I see I would say the XSA-304 action is on the altp2m,
> not the host one - the p2m_set_entry() invocation passes "p2m",
> which gets set immediately prior to the call to
> p2m_altp2m_get_or_propagate(). This is also what gets passed into
> the function. It's the host p2m only when !altp2m_active(currd).

Oh, yea, I see. OK, makes sense.

Reviewed-by: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>

>
> > In either case, in all the setups we use altp2m we never use any
> > superpages, the recommendation is to boot with hap_1gb=0 hap_2mb=0. I
> > never trusted the complexity of superpage shattering and its
> > implementation in Xen as it is very hard to follow.
>
> Hmm, interesting. If you're aware of bugs there, would you mind
> letting us know the details? There shouldn't be a need to use
> command line options to make altp2m actually work. If there's an
> incompatibility, and if we can't get that fixed, perhaps use of
> superpages would want suppressing when altp2m gets enabled for a
> guest? Right now, without this being enforced (nor spelled out
> anywhere in, say, a code comment), I don't think we should make
> assumptions like this. Hence the patch.

I don't know of any bugs, it's just the complexity of page-shattering
always had a "smell", thus I opted to recommend superpages being
always disabled. With the added complexity of altp2m it's even more so
the case.

Tamas