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

[Gavin Shan]

Hi Jonathan,

On 7/27/20 7:02 PM, Jonathan Cameron wrote:
> On Mon, 27 Jul 2020 10:46:52 +1000
> Gavin Shan <gshan at redhat.com> wrote:
>> On 7/24/20 1:19 AM, Jonathan Cameron wrote:
>>> On Wed, 22 Jul 2020 19:57:37 +1000
>>> Gavin Shan <gshan at 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 at 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.
> 
> 	};
> 
> }

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.

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