Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 9 Aug 2022 11:38:03 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     Borislav Petkov <bp@alien8.de>
CC:     Rik van Riel <riel@surriel.com>,
        Dave Hansen <dave.hansen@intel.com>,
        Dave Hansen <dave.hansen@linux.intel.com>, <x86@kernel.org>,
        <linux-kernel@vger.kernel.org>, <kernel-team@fb.com>
Subject: Re: [RFC PATCH 2/5] entry: Add calls for save/restore auxiliary
 pt_regs
Message-ID: <YvKpi/CVHko50PNQ@iweiny-desk3>
References: <20220805173009.3128098-1-ira.weiny@intel.com>
 <20220805173009.3128098-3-ira.weiny@intel.com>
 <YvJNe2rzXfcogFFX@zn.tnic>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <YvJNe2rzXfcogFFX@zn.tnic>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?8bCah3Kkjwikub2iyZ/eFhxL+XZSdf/X6o//iC25SJFVjHZpXqkCpwXHmDZP?=
 =?us-ascii?Q?9QRpR1rg0fbMd/vtetQVWS2VxrnUC/lor/8mnc83AD/Nmjx70q1xTOZ2PM8x?=
 =?us-ascii?Q?0aqXbkf1TPTx/PX8J+kNn/U8ardgYQ7i2mvoUCac6eS2C9Uo7Q/djWlOE2k4?=
 =?us-ascii?Q?7obHaQnPo9Ei1EBJPfLkcF2pRIxVZWONjAqK+1SOm6pjQKXGgB29eyuyet6o?=
 =?us-ascii?Q?A2x6IMvrw6aDcfXvNZT9am2Y12/OIHcssvzendlu3rm0l8TctZlWvm0+s6p0?=
 =?us-ascii?Q?i67JMqkp41BpZCJ2v26Y9d3LrU+2t/N/DTFQZm3teSVKjNDv7D8wGyspJtLu?=
 =?us-ascii?Q?uyt9wGvFX8qDok2d2jcTabvgg38Ojv7kDGpwIDd8p/uriFfNcm50hHjfHR/H?=
 =?us-ascii?Q?Hmf7nLxC7zaL3Yzb9APCJvEgCOuZQYvzBymifp5FkROIuxVcC+pUEvu3sHbq?=
 =?us-ascii?Q?bkh30Lvzfmtqzp5zVdWddF1gGSNQOV2GnSXUyAz9DlSdJbHOSPX5V4ioyqC3?=
 =?us-ascii?Q?ADgN6sNSrGEiY1V28maTosH5h4a8McDikexe7dXwb76fD54XDrXkSz8Mk1dc?=
 =?us-ascii?Q?w1INudtOyKevRs/UE43bH+GedfoCzqos8DWXKXqs/ENC/9NWuAFPniPf5nuF?=
 =?us-ascii?Q?9ROOwWAQOOF2wCGoebOiM6oZrXBOuWOe9ZdTObWall4wySoJZ4Kzr0IEpFFz?=
 =?us-ascii?Q?Um3H+4svVq4tApjox5LJtjuzD3xETHxbJCZ+6tK8aQebzGrKoJAs9DPRXcB6?=
 =?us-ascii?Q?RxqYNkUNbmVtLHtL8/wf+7KXJgfmeeH6m9SUVYv5Kudk2rTBUdpvQ4jUTc/W?=
 =?us-ascii?Q?wSpuzLEK/yWV4YnkvglX/rEepph6Z6WXMlIKi5+8Jf45gXCcJ+1C5tG/lWtU?=
 =?us-ascii?Q?X9KQtuweZ6Fd3+1rPXj3+S76+bw8eLfeEi9C8qoL6yb9zLgiYVnmi+4p3Z8c?=
 =?us-ascii?Q?cb50kLldQb5hIbHGNru/zOtGj095C+SXIf4sMzaY5UnmVarKbPteZcFUvLom?=
 =?us-ascii?Q?M/hPGeGn0VuAu4oujMsYDqFeo2uw1LNq7RDyQvIudc9lDcenAl8qIc9GSwFd?=
 =?us-ascii?Q?DJNMnl62zb8J8NS2VSZfxVZethV0ob+yPJU9EE37hL510rpzXxBxagQXJvBq?=
 =?us-ascii?Q?MvXo0YeSzbeIyImgBua1x43XOY2TahJMZ5j6aty+Xt625MYUsSrSDa9rxWgN?=
 =?us-ascii?Q?3h4t+u5w6YWWsx/EbwFN4t6mQ07W1X5536IfsNc2Y6MXSlp+2BOKkyuLsrWS?=
 =?us-ascii?Q?mUACbEKXXHq2Eg4ncF+edF7FOMw6SFgYFsRk7zZC0V9kPfHydc0NnJmD7Kk3?=
 =?us-ascii?Q?EkUhtOkiJyzOxF2l2MpDztSOrCWJVmd00unl5VFKGrT2ebQ2/OC1Fw3O0eOI?=
 =?us-ascii?Q?BcVPDJDkWwtPiUNCPsfBCsZBsOpRDl2C5d9FxY5Iwfmi4bsp13nU1omsTQK8?=
 =?us-ascii?Q?kPPGmLN89SZb/3fsOmEngiwx+qbltGfzqwERXi662qcH/Rlfn/gKakZM6/BP?=
 =?us-ascii?Q?lyMRuAHoIdJpaddXm3BkjA64LontYmqIAA5cNQsBXRMnM7Zj0EUHnOVpyTm2?=
 =?us-ascii?Q?TqejBzXn7/YNnXoNsYmKRje4AuMkGCmqEfcjS53p?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 07c07127-d85b-4f8c-cfff-08da7a364e07
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6311.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Aug 2022 18:38:08.3709
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: HoYIzybU+lpgXt52CycZXFJzmi5LWdxotTd5oLqKC+Pq3jwBnevzKOTfV3Mlh9M/j17R2QrE/WQL/bpDOnf1qg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR11MB3604
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Aug 09, 2022 at 02:05:15PM +0200, Borislav Petkov wrote:
> On Fri, Aug 05, 2022 at 10:30:06AM -0700, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > Some architectures have auxiliary pt_regs space available to store
> > information on the stack during exceptions.  This information is easier
> > to obtain and store within C code rather than in arch specific assembly.
> 
> There are others?

Other archs?  not now.

> 
> Because I would've done this whole thing in arch/x86/ only...

Thomas did a lot of work to make the entry code generic.  So I was keeping that
work consistent.  This also helps to ensure I did not miss any places.

> 
> > diff --git a/kernel/entry/common.c b/kernel/entry/common.c
> > index 8c0f334c4b75..a70a0f314aee 100644
> > --- a/kernel/entry/common.c
> > +++ b/kernel/entry/common.c
> > @@ -317,7 +317,7 @@ noinstr irqentry_state_t irqentry_enter(struct pt_regs *regs)
> >  
> >  	if (user_mode(regs)) {
> >  		irqentry_enter_from_user_mode(regs);
> > -		return ret;
> > +		goto aux_save;
> 
> Why do you have to goto and do the instrumentation sandwitch around it
> at the goto label?
> 
> Why not simply do
> 
> 	if (user_mode(regs)) {
> 		irqentry_enter_from_user_mode(regs);
> 		arch_save_aux_pt_regs(regs);
> 		return ret;

I don't believe this is correct because instrumentation is not enabled here.
See below.[1]

There are 3 exit paths from irqentry_enter().  All must call
arch_save_aux_pt_regs().  I felt the maintenance of the code would be easier if
I did not scatter those calls through the function and simply exited 1
place.[1]

FWICT calling instrumentation_{begin,end}() is a noop in production code.  So
there is no real cost to calling instrumentation_begin() -> end -> begin ->
end.  And the goto seemed low enough overhead.  Given the current discussion I
admit I may have made the wrong choice.  But I think I would add some comments
to the below to help future developers.

Ira

[1]

--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -317,6 +317,9 @@ noinstr irqentry_state_t irqentry_enter(struct pt_regs *regs)

        if (user_mode(regs)) {
                irqentry_enter_from_user_mode(regs);
+               instrumentation_begin();
+               arch_save_aux_pt_regs(regs);
+               instrumentation_end();
                return ret;
        }

@@ -353,6 +356,7 @@ noinstr irqentry_state_t irqentry_enter(struct pt_regs *regs)
                ct_irq_enter();
                instrumentation_begin();
                trace_hardirqs_off_finish();
+               arch_save_aux_pt_regs(regs);
                instrumentation_end();

                ret.exit_rcu = true;
@@ -369,6 +373,7 @@ noinstr irqentry_state_t irqentry_enter(struct pt_regs *regs)
        instrumentation_begin();
        rcu_irq_enter_check_tick();
        trace_hardirqs_off_finish();
+       arch_save_aux_pt_regs(regs);
        instrumentation_end();

        return ret;