| Message ID | 20200722095740.28560-15-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor SDEI client driver | expand |
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);
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
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 >
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
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 >
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
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);
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(-)