[v2,14/17] drivers/firmware/sdei: Move struct sdei_event to header file
diff mbox series

Message ID 20200722095740.28560-15-gshan@redhat.com
State New
Headers show
Series
  • Refactor SDEI client driver
Related show

Commit Message

Gavin Shan July 22, 2020, 9:57 a.m. UTC
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(-)

Comments

Jonathan Cameron July 23, 2020, 3:19 p.m. UTC | #1
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>
Hi Gavin,

One question inline.

Jonathan

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

> +	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);
Gavin Shan July 27, 2020, 12:46 a.m. UTC | #2
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().

>> +	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
Jonathan Cameron July 27, 2020, 9:02 a.m. UTC | #3
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
>
Gavin Shan July 27, 2020, 9:59 a.m. UTC | #4
Hi Jonathan,

On 7/27/20 7:02 PM, Jonathan Cameron wrote:
> On Mon, 27 Jul 2020 10:46:52 +1000
> Gavin Shan <gshan@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@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.
> 
> 	};
> 
> }

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
Jonathan Cameron July 27, 2020, 1:50 p.m. UTC | #5
On Mon, 27 Jul 2020 19:59:24 +1000
Gavin Shan <gshan@redhat.com> wrote:

> Hi Jonathan,
> 
> On 7/27/20 7:02 PM, Jonathan Cameron wrote:
> > On Mon, 27 Jul 2020 10:46:52 +1000
> > Gavin Shan <gshan@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@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.
> > 
> > 	};
> > 
> > }  
> 
> 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
>
Gavin Shan July 28, 2020, 2:52 a.m. UTC | #6
Hi Jonathan,

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

Patch
diff mbox series

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 */
+	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);