sh_unshadow_for_p2m_change() vs p2m_set_entry()

sh_unshadow_for_p2m_change() vs p2m_set_entry()

[Jan Beulich]

Tim,

I'm afraid you're still my best guess to hopefully get an insight
on issues like this one.

While doing IOMMU superpage work I was, just in the background,
considering in how far the superpage re-coalescing to be used there
couldn't be re-used for P2M / EPT / NPT. Which got me to think about
shadow mode's using of p2m-pt.c: That's purely software use of the
tables in that case, isn't it? In which case hardware support for
superpages shouldn't matter at all.

The only place where I could spot that P2M superpages would actually
make a difference to shadow code was sh_unshadow_for_p2m_change().
That one appears to have been dealing with 2M pages (more below)
already at the time of 0ca1669871f8a ("P2M: check whether hap mode
is enabled before using 2mb pages"), so I wonder what "potential
errors when hap is disabled" this commit's description might be
talking about. (Really, if it's software use of the tables only, in
principle even 512Gb superpages could be made use of there. But of
course sh_unshadow_for_p2m_change() wouldn't really like this, just
like it doesn't like 1Gb pages. So that's purely a theoretical
consideration.)

As to sh_unshadow_for_p2m_change()'s readiness for at least 2Mb
pages: The 4k page handling there differs from the 2M one primarily
in the p2m type checks: "p2m_is_valid(...) || p2m_is_grant(...)"
vs "p2m_is_valid(...)" plus later "!p2m_is_ram(...)", the first
three acting on the type taken from the old PTE, while the latter
acts on the type in the new (split) PTEs. Shouldn't the exact same
checks be used in both cases (less the p2m_is_grant() check which
can't be true for superpages)? IOW isn't !p2m_is_ram() at least
superfluous (and perhaps further redundant with the subsequent
!mfn_eq(l1e_get_mfn(npte[i]), omfn))? Instead I'm missing an
entry-is-present check, without which l1e_get_mfn(npte[i]) looks
risky at best. Or is p2m_is_ram() considered a sufficient
replacement, making assumptions on the behavior of a lot of other
code?

The 2M logic also first checks _PAGE_PRESENT (and _PAGE_PSE), while
the 4k logic appears to infer that the old page was present from
p2m_is_{valid,grant}().

And isn't this 2M page handling code (because of the commit pointed
at above) dead right now anyway? If not, where would P2M superpages
come from?

Thanks much,
Jan

Re: sh_unshadow_for_p2m_change() vs p2m_set_entry()

[Tim Deegan]

Hi,

At 13:31 +0200 on 24 Sep (1632490304), Jan Beulich wrote:
> I'm afraid you're still my best guess to hopefully get an insight
> on issues like this one.

I'm now very rusty on all this but I'll do my best!  I suspect I'll
just be following you through the code.

> While doing IOMMU superpage work I was, just in the background,
> considering in how far the superpage re-coalescing to be used there
> couldn't be re-used for P2M / EPT / NPT. Which got me to think about
> shadow mode's using of p2m-pt.c: That's purely software use of the
> tables in that case, isn't it? In which case hardware support for
> superpages shouldn't matter at all.

ISTR at the time we used the same table for p2m and NPT.
If that's gone away, then yes, we could have superpages
in the p2m without caring about hardware support.

> The only place where I could spot that P2M superpages would actually
> make a difference to shadow code was sh_unshadow_for_p2m_change().
> That one appears to have been dealing with 2M pages (more below)
> already at the time of 0ca1669871f8a ("P2M: check whether hap mode
> is enabled before using 2mb pages"), so I wonder what "potential
> errors when hap is disabled" this commit's description might be
> talking about.

Sorry, I have no idea what the "potential errors" were here. :(

> As to sh_unshadow_for_p2m_change()'s readiness for at least 2Mb
> pages: The 4k page handling there differs from the 2M one primarily
> in the p2m type checks: "p2m_is_valid(...) || p2m_is_grant(...)"
> vs "p2m_is_valid(...)" plus later "!p2m_is_ram(...)", the first
> three acting on the type taken from the old PTE, while the latter
> acts on the type in the new (split) PTEs.

So I think the type tests on the old entry are correct - as you say,
p2m_is_grant() only applies to 4k entries and otherwise they're the
same.

> Shouldn't the exact same
> checks be used in both cases (less the p2m_is_grant() check which
> can't be true for superpages)? IOW isn't !p2m_is_ram() at least
> superfluous (and perhaps further redundant with the subsequent
> !mfn_eq(l1e_get_mfn(npte[i]), omfn))? Instead I'm missing an
> entry-is-present check, without which l1e_get_mfn(npte[i]) looks
> risky at best. Or is p2m_is_ram() considered a sufficient
> replacement, making assumptions on the behavior of a lot of other
> code?

AFAICT, p2m_is_ram(p2m_flags_to_type(l1e_get_flags(npte[i]))) implies
that npte[i] has a valid MFN in it, but I agree that it would be better
to check _PAGE_PRESENT.

And I think in general it would make sense to factor out the old->new
checks and make them the same between the L1 and L2.  I don't see
anything obviously wrong with the current code but the refactored code
would be more obviously right.

> The 2M logic also first checks _PAGE_PRESENT (and _PAGE_PSE), while
> the 4k logic appears to infer that the old page was present from
> p2m_is_{valid,grant}().

I think the p2m_type check is the right one (rather than
_PAGE_PRESENT), since that's the one that the p2m lookups will obey
when causing things to get shadowed.  But the extra _PAGE_PSE check
should stay.

> And isn't this 2M page handling code (because of the commit pointed
> at above) dead right now anyway? If not, where would P2M superpages
> come from?

Yep, that sounds right - p2m_set_entry still only sets 4k entries on
shadow domains, so I think this code is dead right now.  If you had
asked me I would not have remembered that was the case.

Cheers,

Tim.

Re: sh_unshadow_for_p2m_change() vs p2m_set_entry()

[Jan Beulich]

On 27.09.2021 22:25, Tim Deegan wrote:
> At 13:31 +0200 on 24 Sep (1632490304), Jan Beulich wrote:
>> I'm afraid you're still my best guess to hopefully get an insight
>> on issues like this one.
> 
> I'm now very rusty on all this but I'll do my best!  I suspect I'll
> just be following you through the code.

Thanks much!

>> While doing IOMMU superpage work I was, just in the background,
>> considering in how far the superpage re-coalescing to be used there
>> couldn't be re-used for P2M / EPT / NPT. Which got me to think about
>> shadow mode's using of p2m-pt.c: That's purely software use of the
>> tables in that case, isn't it? In which case hardware support for
>> superpages shouldn't matter at all.
> 
> ISTR at the time we used the same table for p2m and NPT.
> If that's gone away, then yes, we could have superpages
> in the p2m without caring about hardware support.

No, that code is still used two ways, but it can't be used for the
same domain in both of these ways. IOW I'm wondering whether the
check for 2M pages to be usable shouldn't be "!hap || hap_2mb", as
opposed to the 1G check continuing to be "hap && hap_1gb". Of
course once I make that change, I may end up learning what
"potential errors" that other commit was talking about ...

As to the further parts of your reply, I guess I'll try to
transform this (largely supporting my observations) and the above
into one or more patches then.

Jan

Re: sh_unshadow_for_p2m_change() vs p2m_set_entry()

[Jan Beulich]

On 27.09.2021 22:25, Tim Deegan wrote:
> At 13:31 +0200 on 24 Sep (1632490304), Jan Beulich wrote:
>> The 2M logic also first checks _PAGE_PRESENT (and _PAGE_PSE), while
>> the 4k logic appears to infer that the old page was present from
>> p2m_is_{valid,grant}().
> 
> I think the p2m_type check is the right one (rather than
> _PAGE_PRESENT), since that's the one that the p2m lookups will obey
> when causing things to get shadowed.  But the extra _PAGE_PSE check
> should stay.

Actually, having transformed things into patch form, I'm now puzzled
by you explicitly saying this. Wasn't this check wrong in the first
place? I don't see anything preventing an L1 page table getting
zapped (or replaced by a 2M mapping) all in one go. The full range
of GFNs would need checking in this case as well, just like in the
opposite case (2M mapping getting replaced by an L1 pt).

This scenario may not be overly likely right now, but would become
more likely once we start re-coalescing large pages (which I'm
planning to investigate using the same scheme currently proposed
on the IOMMU side, to see whether this actually happens frequently
enough to be worthwhile).

Jan

Re: sh_unshadow_for_p2m_change() vs p2m_set_entry()

[Tim Deegan]

At 07:59 +0200 on 01 Oct (1633075173), Jan Beulich wrote:
> On 27.09.2021 22:25, Tim Deegan wrote:
> > At 13:31 +0200 on 24 Sep (1632490304), Jan Beulich wrote:
> >> The 2M logic also first checks _PAGE_PRESENT (and _PAGE_PSE), while
> >> the 4k logic appears to infer that the old page was present from
> >> p2m_is_{valid,grant}().
> > 
> > I think the p2m_type check is the right one (rather than
> > _PAGE_PRESENT), since that's the one that the p2m lookups will obey
> > when causing things to get shadowed.  But the extra _PAGE_PSE check
> > should stay.
> 
> Actually, having transformed things into patch form, I'm now puzzled
> by you explicitly saying this. Wasn't this check wrong in the first
> place? I don't see anything preventing an L1 page table getting
> zapped (or replaced by a 2M mapping) all in one go.

ISTR that this couldn't happen, but I don't remember exactly exactly
why.  It may just be that the p2m update code didn't have that path,
but it's a bit fragile to rely on that.

> The full range
> of GFNs would need checking in this case as well, just like in the
> opposite case (2M mapping getting replaced by an L1 pt).

Yes.  Any case where we're inserting or removing an L1 table would
need to check all 512 L1Es.

Cheers,

Tim.

Re: sh_unshadow_for_p2m_change() vs p2m_set_entry()

[Jan Beulich]

On 01.10.2021 13:22, Tim Deegan wrote:
> At 07:59 +0200 on 01 Oct (1633075173), Jan Beulich wrote:
>> On 27.09.2021 22:25, Tim Deegan wrote:
>>> At 13:31 +0200 on 24 Sep (1632490304), Jan Beulich wrote:
>>>> The 2M logic also first checks _PAGE_PRESENT (and _PAGE_PSE), while
>>>> the 4k logic appears to infer that the old page was present from
>>>> p2m_is_{valid,grant}().
>>>
>>> I think the p2m_type check is the right one (rather than
>>> _PAGE_PRESENT), since that's the one that the p2m lookups will obey
>>> when causing things to get shadowed.  But the extra _PAGE_PSE check
>>> should stay.
>>
>> Actually, having transformed things into patch form, I'm now puzzled
>> by you explicitly saying this. Wasn't this check wrong in the first
>> place? I don't see anything preventing an L1 page table getting
>> zapped (or replaced by a 2M mapping) all in one go.
> 
> ISTR that this couldn't happen, but I don't remember exactly exactly
> why.  It may just be that the p2m update code didn't have that path,
> but it's a bit fragile to rely on that.

I'm guessing that this is what the vague "potential errors when hap is
disabled" may have been referring to in that pretty old commit that I
had dug out.

Jan