Re: [PATCH V2 00/10] PKS: Add Protection Keys Supervisor (PKS) support

Ira Weiny
2020-11-04T17:46:55Z

On Tue, Nov 03, 2020 at 12:36:16AM +0100, Thomas Gleixner wrote:
> On Mon, Nov 02 2020 at 12:53, ira weiny wrote:
> > Fenghua Yu (2):
> > x86/pks: Enable Protection Keys Supervisor (PKS)
> > x86/pks: Add PKS kernel API
> >
> > Ira Weiny (7):
> > x86/pkeys: Create pkeys_common.h
> > x86/fpu: Refactor arch_set_user_pkey_access() for PKS support
> > x86/pks: Preserve the PKRS MSR on context switch
> > x86/entry: Pass irqentry_state_t by reference
> > x86/entry: Preserve PKRS MSR across exceptions
> > x86/fault: Report the PKRS state on fault
> > x86/pks: Add PKS test code
> >
> > Thomas Gleixner (1):
> > x86/entry: Move nmi entry/exit into common code
>
> So the actual patch ordering is:
>
> x86/pkeys: Create pkeys_common.h
> x86/fpu: Refactor arch_set_user_pkey_access() for PKS support
> x86/pks: Enable Protection Keys Supervisor (PKS)
> x86/pks: Preserve the PKRS MSR on context switch
> x86/pks: Add PKS kernel API
>
> x86/entry: Move nmi entry/exit into common code
> x86/entry: Pass irqentry_state_t by reference
>
> x86/entry: Preserve PKRS MSR across exceptions
> x86/fault: Report the PKRS state on fault
> x86/pks: Add PKS test code
>
> This is the wrong ordering, really.
>
> x86/entry: Move nmi entry/exit into common code
>
> is a general cleanup and has absolutely nothing to do with PKRS.So this
> wants to go first.
>

Sorry, yes this should be a pre-patch.

> Also:
>
> x86/entry: Move nmi entry/exit into common code
> [from other email]
> > x86/entry: Pass irqentry_state_t by reference
> > >
> > >
>
> is a prerequisite for the rest. So why is it in the middle of the
> series?

It is in the middle because passing by reference is not needed until additional
information is added to irqentry_state_t which is done immediately after this
patch by:

x86/entry: Preserve PKRS MSR across exceptions

I debated squashing the 2 but it made review harder IMO. But I thought keeping
them in order together made a lot of sense.

>
> And then you enable all that muck _before_ it is usable:
>

Strictly speaking you are correct, sorry. I will reorder the series.

>
> Bisectability is overrrated, right?

Agreed, bisectability is important. I thought I had it covered but I was
wrong.

>
> Once again: Read an understand Documentation/process/*
>
> Aside of that using a spell checker is not optional.

Agreed.

In looking closer at the entry code I've found a couple of other instances I'll
add another precursor patch.

I've also found other errors with the series which I should have caught. My
apologies I made some last minute changes which I should have checked more
thoroughly.

Thanks,
Ira