From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_2 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D4157C433E3 for ; Mon, 27 Jul 2020 13:51:42 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A12F12074F for ; Mon, 27 Jul 2020 13:51:42 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="GO84Glxq" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A12F12074F Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=huawei.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=fsJHEFgvY1L4iaxCHFMAbUUyiyRBY2kC9Cm2yNsWYmk=; b=GO84GlxqM8OTuoa7SixlzrRMG E0AHzFHd5cJ9A+dauLYZGEpa1KhYLLw6oKqru47QNVC4JQyqPTUWyBiCEnM6xHlgb/xi9c0RVqVPo IrSU6eQWcWM28l3c5UGItmWbzquWo7uH96yP402FUFhSQpz+Chb80GqyE75qe38jmsqSwnGEXigxp aQ6mBZQsJLQ9hCGcfElFT5hHunI9Bbqn/HzS3y0Jd53SPqgxtJwKmxHG/IOQhgTHmRCGt2wuqr8PA q344etzXN+Nfi7RwI3ZsXj0kqPxJttpJrbsIsYe99d2AM3Gy3qSQrapFPR9TidNGKSZtd7dbqSUBw Om0sAMvqw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1k03W9-0000pG-84; Mon, 27 Jul 2020 13:50:13 +0000 Received: from lhrrgout.huawei.com ([185.176.76.210] helo=huawei.com) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1k03W5-0000nh-JJ for linux-arm-kernel@lists.infradead.org; Mon, 27 Jul 2020 13:50:10 +0000 Received: from lhreml710-chm.china.huawei.com (unknown [172.18.7.106]) by Forcepoint Email with ESMTP id 3412278B24B1416C347A; Mon, 27 Jul 2020 14:50:06 +0100 (IST) Received: from localhost (10.227.96.57) by lhreml710-chm.china.huawei.com (10.201.108.61) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1913.5; Mon, 27 Jul 2020 14:50:06 +0100 Date: Mon, 27 Jul 2020 14:50:04 +0100 From: Jonathan Cameron To: Gavin Shan Subject: Re: [PATCH v2 14/17] drivers/firmware/sdei: Move struct sdei_event to header file Message-ID: <20200727145004.00003321@huawei.com> In-Reply-To: <64923e7c-7fb6-2b21-e578-58aad4e7f3c1@redhat.com> References: <20200722095740.28560-1-gshan@redhat.com> <20200722095740.28560-15-gshan@redhat.com> <20200723161949.00000061@Huawei.com> <4b37f5ba-541e-3269-681f-1ae94b975606@redhat.com> <20200727100232.00000819@Huawei.com> <64923e7c-7fb6-2b21-e578-58aad4e7f3c1@redhat.com> Organization: Huawei tech. R&D (UK) Ltd. X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; i686-w64-mingw32) MIME-Version: 1.0 X-Originating-IP: [10.227.96.57] X-ClientProxiedBy: lhreml719-chm.china.huawei.com (10.201.108.70) To lhreml710-chm.china.huawei.com (10.201.108.61) X-CFilter-Loop: Reflected X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200727_095009_901600_25AD6DF9 X-CRM114-Status: GOOD ( 35.43 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mark.rutland@arm.com, catalin.marinas@arm.com, james.morse@arm.com, shan.gavin@gmail.com, will@kernel.org, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, 27 Jul 2020 19:59:24 +1000 Gavin Shan wrote: > Hi Jonathan, > > On 7/27/20 7:02 PM, Jonathan Cameron wrote: > > On Mon, 27 Jul 2020 10:46:52 +1000 > > Gavin Shan wrote: > >> On 7/24/20 1:19 AM, Jonathan Cameron wrote: > >>> On Wed, 22 Jul 2020 19:57:37 +1000 > >>> Gavin Shan wrote: > >>> > >>>> This moves struct sdei_event to the header file so that it can be > >>>> dereferenced by external modules. This is needed by the code to > >>>> virtualize SDEI functionality, as part of the arm64/kvm. > >>>> > >>>> Signed-off-by: Gavin Shan > >> > >> [...] > >> > >>>> --- > >>>> v2: Derived from "drivers/firmware/sdei: Identify event by struct" > >>>> --- > >>>> drivers/firmware/arm_sdei.c | 20 ------------ > >>>> include/linux/arm_sdei.h | 61 ++++++++++++++++++++++++------------- > >>>> 2 files changed, 40 insertions(+), 41 deletions(-) > >>>> > >>>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c > >>>> index a52dcff59a20..bdd2de0149c0 100644 > >>>> --- a/drivers/firmware/arm_sdei.c > >>>> +++ b/drivers/firmware/arm_sdei.c > >>>> @@ -44,26 +44,6 @@ static asmlinkage void (*sdei_firmware_call)(unsigned long function_id, > >>>> /* entry point from firmware to arch asm code */ > >>>> static unsigned long sdei_entry_point; > >>>> > >>>> -struct sdei_event { > >>>> - /* These three are protected by the sdei_list_lock */ > >>>> - struct list_head list; > >>>> - bool reregister; > >>>> - bool reenable; > >>>> - > >>>> - u32 event_num; > >>>> - u8 type; > >>>> - u8 priority; > >>>> - > >>>> - /* This pointer is handed to firmware as the event argument. */ > >>>> - union { > >>>> - /* Shared events */ > >>>> - struct sdei_registered_event *registered; > >>>> - > >>>> - /* CPU private events */ > >>>> - struct sdei_registered_event __percpu *private_registered; > >>>> - }; > >>>> -}; > >>>> - > >>>> /* Take the mutex for any API call or modification. Take the mutex first. */ > >>>> static DEFINE_MUTEX(sdei_events_lock); > >>>> > >>>> diff --git a/include/linux/arm_sdei.h b/include/linux/arm_sdei.h > >>>> index 0a241c5c911d..fdc2f868d84b 100644 > >>>> --- a/include/linux/arm_sdei.h > >>>> +++ b/include/linux/arm_sdei.h > >>>> @@ -22,6 +22,46 @@ > >>>> */ > >>>> typedef int (sdei_event_callback)(u32 event, struct pt_regs *regs, void *arg); > >>>> > >>>> +/* > >>>> + * This struct represents an event that has been registered. The driver > >>>> + * maintains a list of all events, and which ones are registered. (Private > >>>> + * events have one entry in the list, but are registered on each CPU). > >>>> + * A pointer to this struct is passed to firmware, and back to the event > >>>> + * handler. The event handler can then use this to invoke the registered > >>>> + * callback, without having to walk the list. > >>>> + * > >>>> + * For CPU private events, this structure is per-cpu. > >>>> + */ > >>>> +struct sdei_registered_event { > >>>> + /* For use by arch code: */ > >>>> + struct pt_regs interrupted_regs; > >>>> + > >>>> + sdei_event_callback *callback; > >>>> + void *callback_arg; > >>>> + u32 event_num; > >>>> + u8 priority; > >>>> +}; > >>>> + > >>>> +struct sdei_event { > >>>> + /* These three are protected by the sdei_list_lock */ > >>> > >>> As this patch leaves the sdei_list_lock as local to arm_sdei.c, is this comment still valid? > >>> > >> > >> Yes, the comment is still valid. @sdei_list_lock is used to protect > >> the linked list (@sdei_list) and all elements (@event) in the list. > >> For example, the lock is taken before updating @event->reenabled in > >> function sdei_event_enable(). > > OK. I assume your new KVM code will simply not touch the list. > > That's a bit messy from a 'scope' point of view, but I guess it's not > > worth doing something like: > > > > struct sdei_event_opaque { > > struct list_head list; > > // Whatever else the kvm code doesn't need > > struct sdei_event { > > // The bits that you want to expose more widely (i.e. use in the > > // kvm code. + you ensure that code only ever sees this internal structure. > > > > }; > > > > } > > Yes, your assumption is correct. The list is still managed by > drivers/firmware/arm_sdei.c and it's invisible to the new KVM > code for SDEI virtualization. > > It's worthy to hide those fields in "struct sdei_event" from > external by introducing another struct, from the point of "scope". > But it's not free to maintain another struct in this case. I would > say lets avoid introducing another struct if you agree. I'm fine either way. Jonathan > > >> > >>>> + struct list_head list; > >>>> + bool reregister; > >>>> + bool reenable; > >>>> + > >>>> + u32 event_num; > >>>> + u8 type; > >>>> + u8 priority; > >>>> + > >>>> + /* This pointer is handed to firmware as the event argument. */ > >>>> + union { > >>>> + /* Shared events */ > >>>> + struct sdei_registered_event *registered; > >>>> + > >>>> + /* CPU private events */ > >>>> + struct sdei_registered_event __percpu *private_registered; > >>>> + }; > >>>> +}; > >>>> + > >>>> /* > >>>> * Register your callback to claim an event. The event must be described > >>>> * by firmware. > >>>> @@ -51,27 +91,6 @@ static inline int sdei_mask_local_cpu(void) { return 0; } > >>>> static inline int sdei_unmask_local_cpu(void) { return 0; } > >>>> #endif /* CONFIG_ARM_SDE_INTERFACE */ > >>>> > >>>> - > >>>> -/* > >>>> - * This struct represents an event that has been registered. The driver > >>>> - * maintains a list of all events, and which ones are registered. (Private > >>>> - * events have one entry in the list, but are registered on each CPU). > >>>> - * A pointer to this struct is passed to firmware, and back to the event > >>>> - * handler. The event handler can then use this to invoke the registered > >>>> - * callback, without having to walk the list. > >>>> - * > >>>> - * For CPU private events, this structure is per-cpu. > >>>> - */ > >>>> -struct sdei_registered_event { > >>>> - /* For use by arch code: */ > >>>> - struct pt_regs interrupted_regs; > >>>> - > >>>> - sdei_event_callback *callback; > >>>> - void *callback_arg; > >>>> - u32 event_num; > >>>> - u8 priority; > >>>> -}; > >>>> - > >>>> /* The arch code entry point should then call this when an event arrives. */ > >>>> int notrace sdei_event_handler(struct pt_regs *regs, > >>>> struct sdei_registered_event *arg); > > Thanks, > Gavin > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel