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

[Gavin Shan]

Hi Jonathan,

On 7/27/20 11:50 PM, Jonathan Cameron wrote:
> On Mon, 27 Jul 2020 19:59:24 +1000
> Gavin Shan <gshan at redhat.com> wrote:
>> 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:

[...]

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

Thanks again for your feedback. I think about it again and it's
really nice idea to narrow the scopes of the fields in the struct.
I will split original struct sdei_event into struct sdei_event and
struct sdei_internal_event in v3, which will be posted shortly.

As the naems indicate, struct sdei_event can be dereferenced by
external modules like arm64/kvm in the future, while struct
sdei_internal_event is used by the SDEI client driver only :)

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

[...]

Thanks,
Gavin