Re: [PATCH v2 14/17] drivers/firmware/sdei: Move struct sdei_event to header file

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 27 Jul 2020 10:46:52 +1000
Gavin Shan <gshan@redhat.com> wrote:

> Hi Jonathan,
> 
> On 7/24/20 1:19 AM, Jonathan Cameron wrote:
> > On Wed, 22 Jul 2020 19:57:37 +1000
> > Gavin Shan <gshan@redhat.com> 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 <gshan@redhat.com>  
> 
> [...]
> 
> >> ---
> >> 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.

	};

}
> 
> >> +	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