| Message ID | 1614370277-23235-9-git-send-email-megha.dey@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Introduce dev-msi and interrupt message store | expand |
On Fri, 26 Feb 2021 20:11:12 +0000, Megha Dey <megha.dey@intel.com> wrote: > > Introduce a new function pointer in the irq_chip structure(irq_set_auxdata) > which is responsible for updating data which is stored in a shared register > or data storage. For example, the idxd driver uses the auxiliary data API > to enable/set and disable PASID field that is in the IMS entry (introduced > in a later patch) and that data are not typically present in MSI entry. > > Reviewed-by: Tony Luck <tony.luck@intel.com> > Signed-off-by: Megha Dey <megha.dey@intel.com> > --- > include/linux/interrupt.h | 2 ++ > include/linux/irq.h | 4 ++++ > kernel/irq/manage.c | 32 ++++++++++++++++++++++++++++++++ > 3 files changed, 38 insertions(+) > > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h > index 967e257..461ed1c 100644 > --- a/include/linux/interrupt.h > +++ b/include/linux/interrupt.h > @@ -496,6 +496,8 @@ extern int irq_get_irqchip_state(unsigned int irq, enum irqchip_irq_state which, > extern int irq_set_irqchip_state(unsigned int irq, enum irqchip_irq_state which, > bool state); > > +int irq_set_auxdata(unsigned int irq, unsigned int which, u64 val); > + > #ifdef CONFIG_IRQ_FORCED_THREADING > # ifdef CONFIG_PREEMPT_RT > # define force_irqthreads (true) > diff --git a/include/linux/irq.h b/include/linux/irq.h > index 2efde6a..fc19f32 100644 > --- a/include/linux/irq.h > +++ b/include/linux/irq.h > @@ -491,6 +491,8 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) > * irq_request_resources > * @irq_compose_msi_msg: optional to compose message content for MSI > * @irq_write_msi_msg: optional to write message content for MSI > + * @irq_set_auxdata: Optional function to update auxiliary data e.g. in > + * shared registers > * @irq_get_irqchip_state: return the internal state of an interrupt > * @irq_set_irqchip_state: set the internal state of a interrupt > * @irq_set_vcpu_affinity: optional to target a vCPU in a virtual machine > @@ -538,6 +540,8 @@ struct irq_chip { > void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg); > void (*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg); > > + int (*irq_set_auxdata)(struct irq_data *data, unsigned int which, u64 auxval); > + > int (*irq_get_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool *state); > int (*irq_set_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool state); > > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index 85ede4e..68ff559 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -2860,3 +2860,35 @@ bool irq_check_status_bit(unsigned int irq, unsigned int bitmask) > return res; > } > EXPORT_SYMBOL_GPL(irq_check_status_bit); > + > +/** > + * irq_set_auxdata - Set auxiliary data > + * @irq: Interrupt to update > + * @which: Selector which data to update > + * @auxval: Auxiliary data value > + * > + * Function to update auxiliary data for an interrupt, e.g. to update data > + * which is stored in a shared register or data storage (e.g. IMS). > + */ > +int irq_set_auxdata(unsigned int irq, unsigned int which, u64 val) This looks to me like a massively generalised version of irq_set_irqchip_state(), only without any defined semantics when it comes to the 'which' state, making it look like the irqchip version of an ioctl. We also have the irq_set_vcpu_affinity() callback that is used to perpetrate all sort of sins (and I have abused this interface more than I should admit it). Can we try and converge on a single interface that allows for "side-band state" to be communicated, with documented state? > +{ > + struct irq_desc *desc; > + struct irq_data *data; > + unsigned long flags; > + int res = -ENODEV; > + > + desc = irq_get_desc_buslock(irq, &flags, 0); > + if (!desc) > + return -EINVAL; > + > + for (data = &desc->irq_data; data; data = irqd_get_parent_data(data)) { > + if (data->chip->irq_set_auxdata) { > + res = data->chip->irq_set_auxdata(data, which, val); And this is where things can break: because you don't define what 'which' is, you can end-up with two stacked layers clashing in their interpretation of 'which', potentially doing the wrong thing. Short of having a global, cross architecture definition of all the possible states, this is frankly dodgy. Thanks, M.
On Thu, Mar 25 2021 at 17:23, Marc Zyngier wrote: >> +/** >> + * irq_set_auxdata - Set auxiliary data >> + * @irq: Interrupt to update >> + * @which: Selector which data to update >> + * @auxval: Auxiliary data value >> + * >> + * Function to update auxiliary data for an interrupt, e.g. to update data >> + * which is stored in a shared register or data storage (e.g. IMS). >> + */ >> +int irq_set_auxdata(unsigned int irq, unsigned int which, u64 val) > > This looks to me like a massively generalised version of > irq_set_irqchip_state(), only without any defined semantics when it > comes to the 'which' state, making it look like the irqchip version of > an ioctl. > > We also have the irq_set_vcpu_affinity() callback that is used to > perpetrate all sort of sins (and I have abused this interface more > than I should admit it). > > Can we try and converge on a single interface that allows for > "side-band state" to be communicated, with documented state? > >> +{ >> + struct irq_desc *desc; >> + struct irq_data *data; >> + unsigned long flags; >> + int res = -ENODEV; >> + >> + desc = irq_get_desc_buslock(irq, &flags, 0); >> + if (!desc) >> + return -EINVAL; >> + >> + for (data = &desc->irq_data; data; data = irqd_get_parent_data(data)) { >> + if (data->chip->irq_set_auxdata) { >> + res = data->chip->irq_set_auxdata(data, which, val); > > And this is where things can break: because you don't define what > 'which' is, you can end-up with two stacked layers clashing in their > interpretation of 'which', potentially doing the wrong thing. > > Short of having a global, cross architecture definition of all the > possible states, this is frankly dodgy. My bad, I suggested this in the first place. So what you suggest is to make 'which' an enum and have that in include/linux/whatever.h so we end up with unique identifiers accross architectures, irqdomains and whatever, right? That makes a lot of sense. Though that leaves the question of the data type for 'val'. While u64 is probably good enough for most stuff, anything which needs more than that is left out (again). union as arguments are horrible especially if you need the counterpart irq_get_auxdata() for which you need a pointer and then you can't do a forward declaration. Something like this might work though and avoid to make the pointer business unconditional: struct irq_auxdata { union { u64 val; struct foo *foo; }; }; Thanks, tglx
On Thu, 25 Mar 2021 18:59:48 +0000, Thomas Gleixner <tglx@linutronix.de> wrote: > > On Thu, Mar 25 2021 at 17:23, Marc Zyngier wrote: > >> +{ > >> + struct irq_desc *desc; > >> + struct irq_data *data; > >> + unsigned long flags; > >> + int res = -ENODEV; > >> + > >> + desc = irq_get_desc_buslock(irq, &flags, 0); > >> + if (!desc) > >> + return -EINVAL; > >> + > >> + for (data = &desc->irq_data; data; data = irqd_get_parent_data(data)) { > >> + if (data->chip->irq_set_auxdata) { > >> + res = data->chip->irq_set_auxdata(data, which, val); > > > > And this is where things can break: because you don't define what > > 'which' is, you can end-up with two stacked layers clashing in their > > interpretation of 'which', potentially doing the wrong thing. > > > > Short of having a global, cross architecture definition of all the > > possible states, this is frankly dodgy. > > My bad, I suggested this in the first place. > > So what you suggest is to make 'which' an enum and have that in > include/linux/whatever.h so we end up with unique identifiers accross > architectures, irqdomains and whatever, right? Exactly. As long as 'which' is unique and unambiguous, we can solve the stacking issue (which is oddly starting to smell like the ghost of the SVR3 STREAMS... /me runs ;-). Once we have that, I can start killing the irq_set_vcpu_affinity() abuse I have been guilty of over the past years. Even more, we could kill irq_set_vcpu_affinity() altogether, because we have a generic way of passing side-band information from a driver down to the IRQ stack. > That makes a lot of sense. > > Though that leaves the question of the data type for 'val'. While u64 is > probably good enough for most stuff, anything which needs more than that > is left out (again). union as arguments are horrible especially if you > need the counterpart irq_get_auxdata() for which you need a pointer and > then you can't do a forward declaration. Something like this might work > though and avoid to make the pointer business unconditional: > > struct irq_auxdata { > union { > u64 val; > struct foo *foo; > }; > }; I guess that at some point, irq_get_auxdata() will become a requirement so we might as well bite the bullet now, and the above looks like a good start to me. Thanks, M.
On Fri, Mar 26 2021 at 10:32, Marc Zyngier wrote: > On Thu, 25 Mar 2021 18:59:48 +0000, > Thomas Gleixner <tglx@linutronix.de> wrote: >> Though that leaves the question of the data type for 'val'. While u64 is >> probably good enough for most stuff, anything which needs more than that >> is left out (again). union as arguments are horrible especially if you >> need the counterpart irq_get_auxdata() for which you need a pointer and >> then you can't do a forward declaration. Something like this might work >> though and avoid to make the pointer business unconditional: >> >> struct irq_auxdata { >> union { >> u64 val; >> struct foo *foo; >> }; >> }; > > I guess that at some point, irq_get_auxdata() will become a > requirement so we might as well bite the bullet now, and the above > looks like a good start to me. And because it's some time until rustification, we can come up with something nasty like the below: #define IRQ_AUXTYPE(t, m) IRQ_AUXTYPE_ ## t enum irq_auxdata_type { IRQ_AUXTYPE(U64, val64), IRQ_AUXTYPE(IRQSTATE, istate), IRQ_AUXTYPE(VCPU_AFFINITY, vaff), }; struct irq_auxdata { union { u64 val64; u8 istate; struct vcpu_aff *vaff; }; }; This does not protect you from accessing the wrong union member, but it allows an analyzer to map IRQ_AUXTYPE_U64 to irq_auxdata::val64 and then check both the call site and the implementation whether they fiddle with some other member of irq_auxdata or do weird casts. Maybe it's just nuts and has no value, but I had the urge to write some nasty macros. Thanks, tglx
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h index 967e257..461ed1c 100644 --- a/include/linux/interrupt.h +++ b/include/linux/interrupt.h @@ -496,6 +496,8 @@ extern int irq_get_irqchip_state(unsigned int irq, enum irqchip_irq_state which, extern int irq_set_irqchip_state(unsigned int irq, enum irqchip_irq_state which, bool state); +int irq_set_auxdata(unsigned int irq, unsigned int which, u64 val); + #ifdef CONFIG_IRQ_FORCED_THREADING # ifdef CONFIG_PREEMPT_RT # define force_irqthreads (true) diff --git a/include/linux/irq.h b/include/linux/irq.h index 2efde6a..fc19f32 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -491,6 +491,8 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) * irq_request_resources * @irq_compose_msi_msg: optional to compose message content for MSI * @irq_write_msi_msg: optional to write message content for MSI + * @irq_set_auxdata: Optional function to update auxiliary data e.g. in + * shared registers * @irq_get_irqchip_state: return the internal state of an interrupt * @irq_set_irqchip_state: set the internal state of a interrupt * @irq_set_vcpu_affinity: optional to target a vCPU in a virtual machine @@ -538,6 +540,8 @@ struct irq_chip { void (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg *msg); void (*irq_write_msi_msg)(struct irq_data *data, struct msi_msg *msg); + int (*irq_set_auxdata)(struct irq_data *data, unsigned int which, u64 auxval); + int (*irq_get_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool *state); int (*irq_set_irqchip_state)(struct irq_data *data, enum irqchip_irq_state which, bool state); diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index 85ede4e..68ff559 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -2860,3 +2860,35 @@ bool irq_check_status_bit(unsigned int irq, unsigned int bitmask) return res; } EXPORT_SYMBOL_GPL(irq_check_status_bit); + +/** + * irq_set_auxdata - Set auxiliary data + * @irq: Interrupt to update + * @which: Selector which data to update + * @auxval: Auxiliary data value + * + * Function to update auxiliary data for an interrupt, e.g. to update data + * which is stored in a shared register or data storage (e.g. IMS). + */ +int irq_set_auxdata(unsigned int irq, unsigned int which, u64 val) +{ + struct irq_desc *desc; + struct irq_data *data; + unsigned long flags; + int res = -ENODEV; + + desc = irq_get_desc_buslock(irq, &flags, 0); + if (!desc) + return -EINVAL; + + for (data = &desc->irq_data; data; data = irqd_get_parent_data(data)) { + if (data->chip->irq_set_auxdata) { + res = data->chip->irq_set_auxdata(data, which, val); + break; + } + } + + irq_put_desc_busunlock(desc, flags); + return res; +} +EXPORT_SYMBOL_GPL(irq_set_auxdata);