| Message ID | 1439477277-6157-4-git-send-email-rric@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 13/08/15 15:47, Robert Richter wrote: > From: Robert Richter <rrichter@cavium.com> > > This patch implements Cavium ThunderX erratum 23154. > > The gicv3 of ThunderX requires a modified version for reading the IAR > status to ensure data synchronization. Since this is in the fast-path > and called with each interrupt, runtime patching is used using jump > label patching for smallest overhead (no-op). This is the same > technique as used for tracepoints. > > v2: > * implement code in a single asm() to keep instruction sequence > * added comment to the code that explains the erratum > * apply workaround also if running as guest, thus check MIDR > > Signed-off-by: Robert Richter <rrichter@cavium.com> > --- > drivers/irqchip/irq-gic-v3.c | 45 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 44 insertions(+), 1 deletion(-) > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > index 1a91902be0b1..2f80a11621a0 100644 > --- a/drivers/irqchip/irq-gic-v3.c > +++ b/drivers/irqchip/irq-gic-v3.c > @@ -107,7 +107,7 @@ static void gic_redist_wait_for_rwp(void) > } > > /* Low level accessors */ > -static u64 __maybe_unused gic_read_iar(void) > +static u64 gic_read_iar_common(void) > { > u64 irqstat; > > @@ -115,6 +115,38 @@ static u64 __maybe_unused gic_read_iar(void) > return irqstat; > } > > +/* > + * Cavium ThunderX erratum 23154 > + * > + * The gicv3 of ThunderX requires a modified version for reading the > + * IAR status to ensure data synchronization (access to icc_iar1_el1 > + * is not sync'ed before and after). > + */ > +static u64 gic_read_iar_cavium_thunderx(void) > +{ > + u64 irqstat; > + > + asm volatile( > + "nop;nop;nop;nop\n\t" > + "nop;nop;nop;nop\n\t" > + "mrs_s %0, " __stringify(ICC_IAR1_EL1) "\n\t" > + "nop;nop;nop;nop" > + : "=r" (irqstat)); > + mb(); > + > + return irqstat; > +} > + > +struct static_key is_cavium_thunderx = STATIC_KEY_INIT_FALSE; > + > +static u64 __maybe_unused gic_read_iar(void) > +{ > + if (static_key_false(&is_cavium_thunderx)) > + return gic_read_iar_common(); > + else > + return gic_read_iar_cavium_thunderx(); > +} > + > static void __maybe_unused gic_write_pmr(u64 val) > { > asm volatile("msr_s " __stringify(ICC_PMR_EL1) ", %0" : : "r" (val)); > @@ -766,8 +798,19 @@ static const struct irq_domain_ops gic_irq_domain_ops = { > .free = gic_irq_domain_free, > }; > > +static void gicv3_enable_cavium_thunderx(void *data) > +{ > + static_key_slow_inc(&is_cavium_thunderx); > +} > + > static const struct gic_capabilities gicv3_errata[] = { > { > + .desc = "GIC: Cavium erratum 23154", > + .iidr = 0xa100034c, /* ThunderX pass 1.x */ > + .iidr_mask = 0xffff0fff, > + .init = gicv3_enable_cavium_thunderx, > + }, I'm even more puzzled. You're working around a CPU bug based on the ITS ID registers? Or have you swapped the detection methods for the two errata? M.
Marc, thanks for your quick review. On 13.08.15 16:11:15, Marc Zyngier wrote: > On 13/08/15 15:47, Robert Richter wrote: > > From: Robert Richter <rrichter@cavium.com> > > static const struct gic_capabilities gicv3_errata[] = { > > { > > + .desc = "GIC: Cavium erratum 23154", > > + .iidr = 0xa100034c, /* ThunderX pass 1.x */ > > + .iidr_mask = 0xffff0fff, > > + .init = gicv3_enable_cavium_thunderx, > > + }, > > I'm even more puzzled. You're working around a CPU bug based on the ITS > ID registers? Or have you swapped the detection methods for the two errata? :/ Right, I mixed this up... Must have starred on this for too long. Will fix that. Wrt midr: Originally this was written to support iidr. I wanted to keep the version check in the driver of the hw, an implementation outside of drivers/irqchip looked not appropriate here as it would rely then on arch arm64 only. This is the main reason. Apart from that, I think an implmentation based on struct arm64_cpu_capabilities, etc. would require much rework compared to my current easy implementation, e.g: * binding flags to callbacks and actually run them, * handing over private driver data (base addr for iidr detection) to a capabilty's match function. Overall this looked bloated. Now, that the MIDR also needs to be checked, it looked better to me to keep the gic hw detection at a single location in the driver. This also allows us to check a combination of midr and iidr values. I hope this sounds reasonable? -Robert
On 13/08/15 17:17, Robert Richter wrote: > Marc, > > thanks for your quick review. > > On 13.08.15 16:11:15, Marc Zyngier wrote: >> On 13/08/15 15:47, Robert Richter wrote: >>> From: Robert Richter <rrichter@cavium.com> > >>> static const struct gic_capabilities gicv3_errata[] = { >>> { >>> + .desc = "GIC: Cavium erratum 23154", >>> + .iidr = 0xa100034c, /* ThunderX pass 1.x */ >>> + .iidr_mask = 0xffff0fff, >>> + .init = gicv3_enable_cavium_thunderx, >>> + }, >> >> I'm even more puzzled. You're working around a CPU bug based on the ITS >> ID registers? Or have you swapped the detection methods for the two errata? > > :/ Right, I mixed this up... Must have starred on this for too long. > Will fix that. > > Wrt midr: Originally this was written to support iidr. I wanted to > keep the version check in the driver of the hw, an implementation > outside of drivers/irqchip looked not appropriate here as it would > rely then on arch arm64 only. This is the main reason. Apart from > that, I think an implmentation based on struct arm64_cpu_capabilities, > etc. would require much rework compared to my current easy > implementation, e.g: > > * binding flags to callbacks and actually run them, > > * handing over private driver data (base addr for iidr detection) to > a capabilty's match function. > > Overall this looked bloated. Now, that the MIDR also needs to be > checked, it looked better to me to keep the gic hw detection at a > single location in the driver. This also allows us to check a > combination of midr and iidr values. > > I hope this sounds reasonable? +Will. The point I was trying to make is that a CPU interface bug is a CPU bug, and that it feels quite weird weird to have the detection in the GIC. Will, what do you think? Also, I don't really buy the combined MIDR/GITS_IIDR detection. These are two *very* distinct pieces of HW that are not even directly connected (the redistributors are in between). I wouldn't mind having something like: struct gic_capabilities { const char *desc; void (*init)(void *data); u32 iidr; u32 iidr_mask; int feature; }; where "feature" is a one of things declared in cpufeature.h, and that would condition the capability (I love the name!) if that really happens. I don't think we're there yet. As for the complexity of implementation, testing a flag in the probe function and tingling a static key is not really a big deal. Thanks, M.
On 13.08.15 17:54:41, Marc Zyngier wrote: > On 13/08/15 17:17, Robert Richter wrote: > > Marc, > > > > thanks for your quick review. > > > > On 13.08.15 16:11:15, Marc Zyngier wrote: > >> On 13/08/15 15:47, Robert Richter wrote: > >>> From: Robert Richter <rrichter@cavium.com> > > > >>> static const struct gic_capabilities gicv3_errata[] = { > >>> { > >>> + .desc = "GIC: Cavium erratum 23154", > >>> + .iidr = 0xa100034c, /* ThunderX pass 1.x */ > >>> + .iidr_mask = 0xffff0fff, > >>> + .init = gicv3_enable_cavium_thunderx, > >>> + }, > >> > >> I'm even more puzzled. You're working around a CPU bug based on the ITS > >> ID registers? Or have you swapped the detection methods for the two errata? > > > > :/ Right, I mixed this up... Must have starred on this for too long. > > Will fix that. > > > > Wrt midr: Originally this was written to support iidr. I wanted to > > keep the version check in the driver of the hw, an implementation > > outside of drivers/irqchip looked not appropriate here as it would > > rely then on arch arm64 only. This is the main reason. Apart from > > that, I think an implmentation based on struct arm64_cpu_capabilities, > > etc. would require much rework compared to my current easy > > implementation, e.g: > > > > * binding flags to callbacks and actually run them, > > > > * handing over private driver data (base addr for iidr detection) to > > a capabilty's match function. > > > > Overall this looked bloated. Now, that the MIDR also needs to be > > checked, it looked better to me to keep the gic hw detection at a > > single location in the driver. This also allows us to check a > > combination of midr and iidr values. > > > > I hope this sounds reasonable? > > +Will. > > The point I was trying to make is that a CPU interface bug is a CPU bug, > and that it feels quite weird weird to have the detection in the GIC. > Will, what do you think? > > Also, I don't really buy the combined MIDR/GITS_IIDR detection. These > are two *very* distinct pieces of HW that are not even directly > connected (the redistributors are in between). > > I wouldn't mind having something like: > > struct gic_capabilities { > const char *desc; > void (*init)(void *data); > u32 iidr; > u32 iidr_mask; > int feature; > }; Yes, once we leave this in the driver it is much easier. But why do the read_cpuid_id() in cpu_errata.c and not in his file? The value/mask pairs will be then on complete different locations for the same kind of hw depending on midr/iidr. And the only reason for using midr is not, that it's a cpu, but just that it needs to be applied to guests too and this is the only way to find out the real hw, otherwise we would use iidr. Apart from the fact that this looks inconsistent having one errata^Mfeature flag for one errata, but not for the other. And only because one is useing midr for hw detection and the other iidr. > where "feature" is a one of things declared in cpufeature.h, and that > would condition the capability (I love the name!) if that really > happens. I don't think we're there yet. Yeah, some "Newspeak". :) -Robert > > As for the complexity of implementation, testing a flag in the probe > function and tingling a static key is not really a big deal. > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny...
On 13/08/15 18:11, Robert Richter wrote: > On 13.08.15 17:54:41, Marc Zyngier wrote: >> On 13/08/15 17:17, Robert Richter wrote: >>> Marc, >>> >>> thanks for your quick review. >>> >>> On 13.08.15 16:11:15, Marc Zyngier wrote: >>>> On 13/08/15 15:47, Robert Richter wrote: >>>>> From: Robert Richter <rrichter@cavium.com> >>> >>>>> static const struct gic_capabilities gicv3_errata[] = { >>>>> { >>>>> + .desc = "GIC: Cavium erratum 23154", >>>>> + .iidr = 0xa100034c, /* ThunderX pass 1.x */ >>>>> + .iidr_mask = 0xffff0fff, >>>>> + .init = gicv3_enable_cavium_thunderx, >>>>> + }, >>>> >>>> I'm even more puzzled. You're working around a CPU bug based on the ITS >>>> ID registers? Or have you swapped the detection methods for the two errata? >>> >>> :/ Right, I mixed this up... Must have starred on this for too long. >>> Will fix that. >>> >>> Wrt midr: Originally this was written to support iidr. I wanted to >>> keep the version check in the driver of the hw, an implementation >>> outside of drivers/irqchip looked not appropriate here as it would >>> rely then on arch arm64 only. This is the main reason. Apart from >>> that, I think an implmentation based on struct arm64_cpu_capabilities, >>> etc. would require much rework compared to my current easy >>> implementation, e.g: >>> >>> * binding flags to callbacks and actually run them, >>> >>> * handing over private driver data (base addr for iidr detection) to >>> a capabilty's match function. >>> >>> Overall this looked bloated. Now, that the MIDR also needs to be >>> checked, it looked better to me to keep the gic hw detection at a >>> single location in the driver. This also allows us to check a >>> combination of midr and iidr values. >>> >>> I hope this sounds reasonable? >> >> +Will. >> >> The point I was trying to make is that a CPU interface bug is a CPU bug, >> and that it feels quite weird weird to have the detection in the GIC. >> Will, what do you think? >> >> Also, I don't really buy the combined MIDR/GITS_IIDR detection. These >> are two *very* distinct pieces of HW that are not even directly >> connected (the redistributors are in between). >> >> I wouldn't mind having something like: >> >> struct gic_capabilities { >> const char *desc; >> void (*init)(void *data); >> u32 iidr; >> u32 iidr_mask; >> int feature; >> }; > > Yes, once we leave this in the driver it is much easier. But why do > the read_cpuid_id() in cpu_errata.c and not in his file? The > value/mask pairs will be then on complete different locations for the > same kind of hw depending on midr/iidr. And the only reason for using > midr is not, that it's a cpu, but just that it needs to be applied to > guests too and this is the only way to find out the real hw, otherwise > we would use iidr. But that's the thing! Using GITS_IIDR would be the complete wrong thing to check for a CPU interface, which is what your ICC_IAR1_EL1 workaround is about. > Apart from the fact that this looks inconsistent > having one errata^Mfeature flag for one errata, but not for the > other. And only because one is useing midr for hw detection and the > other iidr. Because they are architecturally two different things. I strongly suspect that you could take the ThunderX core, remove Cavium's own implementation of the GIC and slap another GIC implementation in front of it, and you would have the exact same CPU interface bug, because that's where the issue is. Thanks, M.
On 14.08.15 09:28:12, Marc Zyngier wrote: > On 13/08/15 18:11, Robert Richter wrote: > > On 13.08.15 17:54:41, Marc Zyngier wrote: > >> On 13/08/15 17:17, Robert Richter wrote: > >>> Marc, > >>> > >>> thanks for your quick review. > >>> > >>> On 13.08.15 16:11:15, Marc Zyngier wrote: > >>>> On 13/08/15 15:47, Robert Richter wrote: > >>>>> From: Robert Richter <rrichter@cavium.com> > >>> > >>>>> static const struct gic_capabilities gicv3_errata[] = { > >>>>> { > >>>>> + .desc = "GIC: Cavium erratum 23154", > >>>>> + .iidr = 0xa100034c, /* ThunderX pass 1.x */ > >>>>> + .iidr_mask = 0xffff0fff, > >>>>> + .init = gicv3_enable_cavium_thunderx, > >>>>> + }, > >>>> > >>>> I'm even more puzzled. You're working around a CPU bug based on the ITS > >>>> ID registers? Or have you swapped the detection methods for the two errata? > >>> > >>> :/ Right, I mixed this up... Must have starred on this for too long. > >>> Will fix that. > >>> > >>> Wrt midr: Originally this was written to support iidr. I wanted to > >>> keep the version check in the driver of the hw, an implementation > >>> outside of drivers/irqchip looked not appropriate here as it would > >>> rely then on arch arm64 only. This is the main reason. Apart from > >>> that, I think an implmentation based on struct arm64_cpu_capabilities, > >>> etc. would require much rework compared to my current easy > >>> implementation, e.g: > >>> > >>> * binding flags to callbacks and actually run them, > >>> > >>> * handing over private driver data (base addr for iidr detection) to > >>> a capabilty's match function. > >>> > >>> Overall this looked bloated. Now, that the MIDR also needs to be > >>> checked, it looked better to me to keep the gic hw detection at a > >>> single location in the driver. This also allows us to check a > >>> combination of midr and iidr values. > >>> > >>> I hope this sounds reasonable? > >> > >> +Will. > >> > >> The point I was trying to make is that a CPU interface bug is a CPU bug, > >> and that it feels quite weird weird to have the detection in the GIC. > >> Will, what do you think? > >> > >> Also, I don't really buy the combined MIDR/GITS_IIDR detection. These > >> are two *very* distinct pieces of HW that are not even directly > >> connected (the redistributors are in between). > >> > >> I wouldn't mind having something like: > >> > >> struct gic_capabilities { > >> const char *desc; > >> void (*init)(void *data); > >> u32 iidr; > >> u32 iidr_mask; > >> int feature; > >> }; > > > > Yes, once we leave this in the driver it is much easier. But why do > > the read_cpuid_id() in cpu_errata.c and not in his file? The > > value/mask pairs will be then on complete different locations for the > > same kind of hw depending on midr/iidr. And the only reason for using > > midr is not, that it's a cpu, but just that it needs to be applied to > > guests too and this is the only way to find out the real hw, otherwise > > we would use iidr. > > But that's the thing! Using GITS_IIDR would be the complete wrong thing > to check for a CPU interface, which is what your ICC_IAR1_EL1 workaround > is about. > > > Apart from the fact that this looks inconsistent > > having one errata^Mfeature flag for one errata, but not for the > > other. And only because one is useing midr for hw detection and the > > other iidr. > > Because they are architecturally two different things. I strongly > suspect that you could take the ThunderX core, remove Cavium's own > implementation of the GIC and slap another GIC implementation in front > of it, and you would have the exact same CPU interface bug, because > that's where the issue is. Ok, I am going to send an update based on your suggestions. Thanks, -Robert
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index 1a91902be0b1..2f80a11621a0 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -107,7 +107,7 @@ static void gic_redist_wait_for_rwp(void) } /* Low level accessors */ -static u64 __maybe_unused gic_read_iar(void) +static u64 gic_read_iar_common(void) { u64 irqstat; @@ -115,6 +115,38 @@ static u64 __maybe_unused gic_read_iar(void) return irqstat; } +/* + * Cavium ThunderX erratum 23154 + * + * The gicv3 of ThunderX requires a modified version for reading the + * IAR status to ensure data synchronization (access to icc_iar1_el1 + * is not sync'ed before and after). + */ +static u64 gic_read_iar_cavium_thunderx(void) +{ + u64 irqstat; + + asm volatile( + "nop;nop;nop;nop\n\t" + "nop;nop;nop;nop\n\t" + "mrs_s %0, " __stringify(ICC_IAR1_EL1) "\n\t" + "nop;nop;nop;nop" + : "=r" (irqstat)); + mb(); + + return irqstat; +} + +struct static_key is_cavium_thunderx = STATIC_KEY_INIT_FALSE; + +static u64 __maybe_unused gic_read_iar(void) +{ + if (static_key_false(&is_cavium_thunderx)) + return gic_read_iar_common(); + else + return gic_read_iar_cavium_thunderx(); +} + static void __maybe_unused gic_write_pmr(u64 val) { asm volatile("msr_s " __stringify(ICC_PMR_EL1) ", %0" : : "r" (val)); @@ -766,8 +798,19 @@ static const struct irq_domain_ops gic_irq_domain_ops = { .free = gic_irq_domain_free, }; +static void gicv3_enable_cavium_thunderx(void *data) +{ + static_key_slow_inc(&is_cavium_thunderx); +} + static const struct gic_capabilities gicv3_errata[] = { { + .desc = "GIC: Cavium erratum 23154", + .iidr = 0xa100034c, /* ThunderX pass 1.x */ + .iidr_mask = 0xffff0fff, + .init = gicv3_enable_cavium_thunderx, + }, + { } };