Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Junaid Shahid <junaids@google.com>, linux-kernel@vger.kernel.org
Cc:     kvm@vger.kernel.org, pbonzini@redhat.com, jmattson@google.com,
        pjt@google.com, oweisse@google.com, alexandre.chartre@oracle.com,
        rppt@linux.ibm.com, dave.hansen@linux.intel.com,
        peterz@infradead.org, luto@kernel.org, linux-mm@kvack.org
Subject: Re: [RFC PATCH 04/47] mm: asi: ASI support in interrupts/exceptions
In-Reply-To: <9f2f1226-f398-f132-06f4-c21a2a2d1033@google.com>
References: <20220223052223.1202152-1-junaids@google.com>
 <20220223052223.1202152-5-junaids@google.com> <87pmmofs83.ffs@tglx>
 <9f2f1226-f398-f132-06f4-c21a2a2d1033@google.com>
Date:   Tue, 15 Mar 2022 13:55:39 +0100
Message-ID: <877d8v74tw.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Junaid,

On Mon, Mar 14 2022 at 19:01, Junaid Shahid wrote:
> On 3/14/22 08:50, Thomas Gleixner wrote:
>> On Tue, Feb 22 2022 at 21:21, Junaid Shahid wrote:
>>>   #define DEFINE_IDTENTRY_RAW(func)					\
>>> -__visible noinstr void func(struct pt_regs *regs)
>>> +static __always_inline void __##func(struct pt_regs *regs);		\
>>> +									\
>>> +__visible noinstr void func(struct pt_regs *regs)			\
>>> +{									\
>>> +	asi_intr_enter();						\
>> 
>> This is wrong. You cannot invoke arbitrary code within a noinstr
>> section.
>> 
>> Please enable CONFIG_VMLINUX_VALIDATION and watch the build result with
>> and without your patches.
>> 
> Thank you for the pointer. It seems that marking asi_intr_enter/exit
> and asi_enter/exit, and the few functions that they in turn call, as
> noinstr would fix this, correct? (Along with removing the VM_BUG_ONs
> from those functions and using notrace/nodebug variants of a couple of
> functions).

you can keep the BUG_ON()s. If such a bug happens the noinstr
correctness is the least of your worries, but it's important to get the
information out, right?

Vs. adding noinstr. Yes, making the full callchain noinstr is going to
cure it, but you really want to think hard whether these calls need to
be in this section of the exception handlers.

These code sections have other constraints aside of being excluded from
instrumentation, the main one being that you cannot use RCU there.

I'm not yet convinced that asi_intr_enter()/exit() need to be invoked in
exactly the places you put it. The changelog does not give any clue
about the why...

Thanks,

        tglx