Hi Boris,
On 11/12/2018 17:44, Borislav Petkov wrote:
> On Mon, Dec 03, 2018 at 06:05:57PM +0000, James Morse wrote:
>> Refactor the estatus queue's pool notification routine from
>> NOTIFY_NMI's handlers. This will allow another notification
>> method to use the estatus queue without duplicating this code.
>>
>> This patch adds rcu_read_lock()/rcu_read_unlock() around the list
>
> s/This patch adds/Add/
>
>> list_for_each_entry_rcu() walker. These aren't strictly necessary as
>> the whole nmi_enter/nmi_exit() window is a spooky RCU read-side
>> critical section.
>>
>> _in_nmi_notify_one() is separate from the rcu-list walker for a later
>> caller that doesn't need to walk a list.
>> +static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
>> +{
>> + int ret = NMI_DONE;
>> +
>> + if (!atomic_add_unless(&ghes_in_nmi, 1, 1))
>> + return ret;
>> +
>> + if (!ghes_estatus_queue_notified(&ghes_nmi))
>> + ret = NMI_HANDLED;
>
> So this reads kinda the other way around, at least to me:
>
> "if the queue was *not* notified, the NMI was handled."
>
> Maybe rename to this:
>
> err = process_queue(&ghes_nmi);
> if (!err)
> ret = NMI_HANDLED;
>
> to make it clearer...
(yup, that's clearer).
But now we've opened pandora's box of naming-things: This thing isn't really
processing anything, its walking a list of 'maybe it was one of these' and
copying anything it finds into the estatus-queue to be handled later.
I've evidently overloaded 'notified' to mean this.
__process_error() doesn't process anything either, it does the add-to-queue.
'spool' is the word that best conveys what's going on here, I should probably
use that 'in_nmi' prefix more to make it clear this has to be nmi safe.
Something like:
ghes_notify_nmi() -> in_nmi_spool_from_list(list) -> in_nmi_queue_one_entry(ghes).
Thanks,
James
