Re: [PATCH] x86: Always have CR4.PKE set in HVM context

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.04.2021 12:21, Andrew Cooper wrote:
> On 30/04/2021 10:08, Jan Beulich wrote:
>> On 30.04.2021 00:12, Andrew Cooper wrote:
>>> The sole user of read_pkru() is the emulated pagewalk, and guarded behind
>>> guest_pku_enabled() which restricts the path to HVM (hap, even) context 
>>> only.
>>>
>>> The commentary in read_pkru() concerning _PAGE_GNTTAB overlapping with
>>> _PAGE_PKEY_BITS is only applicable to PV guests.
>>>
>>> The context switch path, via write_ptbase() unconditionally writes CR4 on 
>>> any
>>> context switch.
>>>
>>> Therefore, we can guarantee to separate CR4.PKE between PV and HVM context 
>>> at
>>> no extra cost.  Set PKE in mmu_cr4_features on boot, so it becomes set in 
>>> HVM
>>> context, and clear it in pv_make_cr4().
>>>
>>> Rename read_pkru() to rdpkru() now that it is a simple wrapper around the
>>> instruction.  This saves two CR4 writes on every pagewalk, which typically
>>> occur more than one per emulation.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> The changes looks perfectly fine to me, but I still feel urged to make
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>> conditional upon my "x86emul: support RDPKRU/WRPKRU" (submitted exactly
>> half a year ago) going in first, unless there were to be review comments
>> making extensive rework necessary. In the absence of such review
>> feedback, I consider it pretty inappropriate for me to do rebasing work
>> (no matter that this would be largely mechanical afaics) here for a
>> patch which has been pending and effectively ignored for so long. (The
>> main thing that immediately struck me as odd was "The sole user of
>> read_pkru() is ...".)
> 
> So the observation about "sole user" occurred to me too, right after I
> sent this.  Then I thought "surely Jan has spotted this and done a patch
> for it".
> 
> Presumably you're talking about "x86emul: support RDPKRU/WRPKRU" from
> Oct 30th ?  I found that via the archives, but I literally don't have a
> copy in my inbox.

Odd. Was that then the time of your severe email issues, and were your
IT folks not even able to make sure pending email got delivered
afterwards (or at least enumerate what emails got discarded)? If I had
got a reply saying the mail couldn't be delivered, I surely would have
resent it.

Just to be sure - you seem to have got a copy of "x86emul: de-duplicate
scatters to the same linear address", as I seem to recall you responding
there, albeit not with an ack or anything I could actually act upon (and
this might have been in irc). That was sent just a few days later, and
suffers a similar stall. And while in a ping I did say I would commit it
without ack, I'm really hesitant to do so there. I've put it on next
week's community meeting's agenda, just in case.

> If I do the rebase, are you happy for this patch to stay as it is (so
> the complicated change concerning context switching doesn't get more
> complicated), and so we're not knowingly adding new constructs which
> need immediate changes?

Well, the answer is not just "yes", but in reality I wouldn't mind
doing the rebasing myself, if only I knew it wasn't for the purpose of
waiting another half year for an ack (or otherwise).

Jan