On 30/04/2021 11:42, Jan Beulich wrote: > 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.coop...@citrix.com> >>> The changes looks perfectly fine to me, but I still feel urged to make >>> Reviewed-by: Jan Beulich <jbeul...@suse.com> >>> 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.
I do recall that patch. > >> 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). The patch itself looks entirely fine. Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com> The only observation I've got is that the other instructions in Grp7 probably want a blanket conversion from generate_exception_if(vex.pfx, EXC_UD); to use the unimplemented_insn path instead, but that's clearly further work. I'll commit this patch, and the rebase delta on yours ought to just be the naming of the helpers. ~Andrew
