Re: [PATCH v6 5/6] Optionally flush L1D on context switch

[Singh, Balbir]

On Wed, 2020-05-13 at 18:16 +0200, Thomas Gleixner wrote:
> Balbir Singh <sblbir@xxxxxxxxxx> writes:
> 
> This part:
> 
> > --- a/include/uapi/linux/prctl.h
> > +++ b/include/uapi/linux/prctl.h
> > @@ -238,4 +238,8 @@ struct prctl_mm_map {
> >  #define PR_SET_IO_FLUSHER            57
> >  #define PR_GET_IO_FLUSHER            58
> > 
> > +/* Flush L1D on context switch (mm) */
> > +#define PR_SET_L1D_FLUSH             59
> > +#define PR_GET_L1D_FLUSH             60
> 
> ...
> 
> > @@ -2514,6 +2524,16 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned
> > long, arg2, unsigned long, arg3,
> > 
> >               error = (current->flags & PR_IO_FLUSHER) ==
> > PR_IO_FLUSHER;
> >               break;
> > +     case PR_SET_L1D_FLUSH:
> > +             if (arg3 || arg4 || arg5)
> > +                     return -EINVAL;
> > +             error = arch_prctl_l1d_flush_set(me, arg2);
> > +             break;
> > +     case PR_GET_L1D_FLUSH:
> > +             if (arg2 || arg3 || arg4 || arg5)
> > +                     return -EINVAL;
> > +             error = arch_prctl_l1d_flush_get(me);
> > +             break;
> >       default:
> >               error = -EINVAL;
> >               break;
> 
> wants to be split into a separate patch, really. Then we get a proper
> subject lines with proper subsystem prefixes. This part also lacks a
> description in Documentation/userspace-api/ and function prototypes
> for
> the arch_prctl* functions.
> 
> But looking at this deeper (yes I should have noticed earlier):
> 
>     Why do we need yet another PRCTL?
> 
> We already have PR_SET_SPECULATION_CTRL/PR_GET_SPECULATION_CTRL. That
> L1D flush thingy fits into this category, right?

It does, I thought about it for a while when I was changing the code and
left it aside because, looking at the definition

1    PR_SPEC_ENABLE         The speculation feature is enabled,
mitigation is disabled.
2    PR_SPEC_DISABLE        The speculation feature is disabled,
mitigation is enabled.

With L1D flush, there is no overriding of the feature as such (as in
enable when the mitigation is disabled and vice-versa). I am happy to
reconsider my initial thought though.


> 
> This makes even more sense if you think about the second use case for
> L1D flush, i.e. the flush when a untrusted task comes in. If we ever
> want to support that case then this will be imposed by seccomp and
> then
> we'd need yet another interface there.
> 

Yep, I see what you mean


> And for this reason we should also name that current opt-in thingy:
> L1D_FLUSH_OUT in the prctl and also for the TIF bits.
> 
> Hmm? Kees?
> 
> I've applied the first 4 patches to:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git x86/mm
> 
> so the polishing I did gets preserved and you don't have to resend the
> whole pile.
> 

Thanks, I think your change to patch 6 makes sense as well. Let me
respin this based on what you think of the argument above

Balbir