[PATCH v2 14/17] drivers/firmware/sdei: Move struct sdei_event to header file
Jonathan Cameron
Jonathan.Cameron at huawei.com
Mon Jul 27 09:50:04 EDT 2020
On Mon, 27 Jul 2020 19:59:24 +1000
Gavin Shan <gshan at 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 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.
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
>
More information about the linux-arm-kernel
mailing list