| Message ID | 20240515-comic-sketch-3b40e6676f55@spud (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] RISC-V: fix Andes errata build issues | expand |
On Wed, May 15, 2024 at 05:09:34PM +0100, Conor Dooley wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > Commit e47c37c24024 ("riscv: Introduce vendor variants of extension > helpers") added includes for the new vendor_extensions.h header in > the T-Head and SiFive errata handling code but didn't do so for Andes, > resulting in allmodconfig build issues when commit 589e2fc85850 > ("riscv: Convert xandespmu to use the vendor extension framework") > added a user of a macro defined there. > > Fixes: 589e2fc85850 ("riscv: Convert xandespmu to use the vendor extension framework") > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > v2: Fixup commit references in the commit message > > CC: Paul Walmsley <paul.walmsley@sifive.com> > CC: Palmer Dabbelt <palmer@dabbelt.com> > CC: Conor Dooley <conor.dooley@microchip.com> > CC: Charlie Jenkins <charlie@rivosinc.com> > CC: linux-riscv@lists.infradead.org > CC: linux-kernel@vger.kernel.org > --- > arch/riscv/errata/andes/errata.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/riscv/errata/andes/errata.c b/arch/riscv/errata/andes/errata.c > index a5d96a7a4682..fc1a34faa5f3 100644 > --- a/arch/riscv/errata/andes/errata.c > +++ b/arch/riscv/errata/andes/errata.c > @@ -17,6 +17,7 @@ > #include <asm/processor.h> > #include <asm/sbi.h> > #include <asm/vendorid_list.h> > +#include <asm/vendor_extensions.h> > > #define ANDES_AX45MP_MARCHID 0x8000000000008a45UL > #define ANDES_AX45MP_MIMPID 0x500UL > -- > 2.43.0 > I was going to fix this in my next version but was waiting for the reviews on the thead stuff. I wasn't anticipating these patches to be able to jump the queue :) Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>
On Wed, May 15, 2024 at 09:49:24AM -0700, Charlie Jenkins wrote: > On Wed, May 15, 2024 at 05:09:34PM +0100, Conor Dooley wrote: > > From: Conor Dooley <conor.dooley@microchip.com> > > > > Commit e47c37c24024 ("riscv: Introduce vendor variants of extension > > helpers") added includes for the new vendor_extensions.h header in > > the T-Head and SiFive errata handling code but didn't do so for Andes, > > resulting in allmodconfig build issues when commit 589e2fc85850 > > ("riscv: Convert xandespmu to use the vendor extension framework") > > added a user of a macro defined there. > > > > Fixes: 589e2fc85850 ("riscv: Convert xandespmu to use the vendor extension framework") > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > I was going to fix this in my next version but was waiting for the > reviews on the thead stuff. I wasn't anticipating these patches to be > able to jump the queue :) Yah, the reason for that is I asked him to take the non-vector parts of the series as 6.10 material so that we'd have less stuff movin' around in cpufeatures.c so that Clement's Zc* + validation changes wouldn't run into a bunch of conflicts etc. Same reason that I pushed for getting Andy's vector subset stuff merged today, but that mighta been before you hopped in. Cheers, Conor.
On Wed, May 15, 2024 at 05:56:30PM +0100, Conor Dooley wrote: > On Wed, May 15, 2024 at 09:49:24AM -0700, Charlie Jenkins wrote: > > On Wed, May 15, 2024 at 05:09:34PM +0100, Conor Dooley wrote: > > > From: Conor Dooley <conor.dooley@microchip.com> > > > > > > Commit e47c37c24024 ("riscv: Introduce vendor variants of extension > > > helpers") added includes for the new vendor_extensions.h header in > > > the T-Head and SiFive errata handling code but didn't do so for Andes, > > > resulting in allmodconfig build issues when commit 589e2fc85850 > > > ("riscv: Convert xandespmu to use the vendor extension framework") > > > added a user of a macro defined there. > > > > > > Fixes: 589e2fc85850 ("riscv: Convert xandespmu to use the vendor extension framework") > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > > > > I was going to fix this in my next version but was waiting for the > > reviews on the thead stuff. I wasn't anticipating these patches to be > > able to jump the queue :) > > Yah, the reason for that is I asked him to take the non-vector parts of > the series as 6.10 material so that we'd have less stuff movin' around > in cpufeatures.c so that Clement's Zc* + validation changes wouldn't run > into a bunch of conflicts etc. Same reason that I pushed for getting > Andy's vector subset stuff merged today, but that mighta been before you > hopped in. > > Cheers, > Conor. Yes I was a couple minutes late to the meeting, whoops. The subset of patches that was pulled into for-next is odd to me because there is some of the thead enablement code as part of the vendor extension enablement so that there was a user for it. Since the subset on Palmer's for-next does not have the rest of the thead code there is only a half-implementation of the thead code, it allows the kernel to probe for xtheadvector but it doesn't probe anywhere. In my opinion, a better solution would be for me to get rid of the thead code entirely from those patches. So that there is still a user, I can replace the thead code with the andes versions. Since Palmer already pulled in those changes maybe it's too late. There is not a critical problem here, but it seems like it's bad practice to introduce code without a user. - Charlie
On Wed, May 15, 2024 at 10:18:43AM -0700, Charlie Jenkins wrote: > On Wed, May 15, 2024 at 05:56:30PM +0100, Conor Dooley wrote: > > On Wed, May 15, 2024 at 09:49:24AM -0700, Charlie Jenkins wrote: > > > On Wed, May 15, 2024 at 05:09:34PM +0100, Conor Dooley wrote: > > > > From: Conor Dooley <conor.dooley@microchip.com> > > > > > > > > Commit e47c37c24024 ("riscv: Introduce vendor variants of extension > > > > helpers") added includes for the new vendor_extensions.h header in > > > > the T-Head and SiFive errata handling code but didn't do so for Andes, > > > > resulting in allmodconfig build issues when commit 589e2fc85850 > > > > ("riscv: Convert xandespmu to use the vendor extension framework") > > > > added a user of a macro defined there. > > > > > > > > Fixes: 589e2fc85850 ("riscv: Convert xandespmu to use the vendor extension framework") > > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > > > > > > > I was going to fix this in my next version but was waiting for the > > > reviews on the thead stuff. I wasn't anticipating these patches to be > > > able to jump the queue :) > > > > Yah, the reason for that is I asked him to take the non-vector parts of > > the series as 6.10 material so that we'd have less stuff movin' around > > in cpufeatures.c so that Clement's Zc* + validation changes wouldn't run > > into a bunch of conflicts etc. Same reason that I pushed for getting > > Andy's vector subset stuff merged today, but that mighta been before you > > hopped in. > > > > Cheers, > > Conor. > > Yes I was a couple minutes late to the meeting, whoops. It's prob at like 0600 for you, so w/e. > The subset of > patches that was pulled into for-next is odd to me because there is some > of the thead enablement code as part of the vendor extension enablement > so that there was a user for it. Since the subset on Palmer's for-next > does not have the rest of the thead code there is only a > half-implementation of the thead code, it allows the kernel to probe for > xtheadvector but it doesn't probe anywhere. I dunno, I think that reporting that the extension is there constitutes a user, it's not gonna be dead code. There's plenty of extensions for which all we do is detect them and nothing more. > In my opinion, a better solution would be for me to get rid of the thead > code entirely from those patches. So that there is still a user, I can > replace the thead code with the andes versions. The Andes stuff is in the subset he applied though, so... > > Since Palmer already pulled in those changes maybe it's too late. There > is not a critical problem here, but it seems like it's bad practice to > introduce code without a user. ...there is actually a "real" user in xandespmu. I did miss that "riscv: Extend cpufeature.c to detect vendor extensions" actually contained the xtheadvector detection though, rather than just the infrastructure. I think it is probably harmless to have it, but shouldn't be too hard to quickly drop the thead bits either I suppose if you're worried about it. Cheers, Conor.
On Wed, May 15, 2024 at 06:30:36PM +0100, Conor Dooley wrote: > On Wed, May 15, 2024 at 10:18:43AM -0700, Charlie Jenkins wrote: > > On Wed, May 15, 2024 at 05:56:30PM +0100, Conor Dooley wrote: > > > On Wed, May 15, 2024 at 09:49:24AM -0700, Charlie Jenkins wrote: > > > > On Wed, May 15, 2024 at 05:09:34PM +0100, Conor Dooley wrote: > > > > > From: Conor Dooley <conor.dooley@microchip.com> > > > > > > > > > > Commit e47c37c24024 ("riscv: Introduce vendor variants of extension > > > > > helpers") added includes for the new vendor_extensions.h header in > > > > > the T-Head and SiFive errata handling code but didn't do so for Andes, > > > > > resulting in allmodconfig build issues when commit 589e2fc85850 > > > > > ("riscv: Convert xandespmu to use the vendor extension framework") > > > > > added a user of a macro defined there. > > > > > > > > > > Fixes: 589e2fc85850 ("riscv: Convert xandespmu to use the vendor extension framework") > > > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > > > > > > > > > > > I was going to fix this in my next version but was waiting for the > > > > reviews on the thead stuff. I wasn't anticipating these patches to be > > > > able to jump the queue :) > > > > > > Yah, the reason for that is I asked him to take the non-vector parts of > > > the series as 6.10 material so that we'd have less stuff movin' around > > > in cpufeatures.c so that Clement's Zc* + validation changes wouldn't run > > > into a bunch of conflicts etc. Same reason that I pushed for getting > > > Andy's vector subset stuff merged today, but that mighta been before you > > > hopped in. > > > > > > Cheers, > > > Conor. > > > > Yes I was a couple minutes late to the meeting, whoops. > > > It's prob at like 0600 for you, so w/e. > > > The subset of > > patches that was pulled into for-next is odd to me because there is some > > of the thead enablement code as part of the vendor extension enablement > > so that there was a user for it. Since the subset on Palmer's for-next > > does not have the rest of the thead code there is only a > > half-implementation of the thead code, it allows the kernel to probe for > > xtheadvector but it doesn't probe anywhere. > > I dunno, I think that reporting that the extension is there constitutes a > user, it's not gonna be dead code. There's plenty of extensions for > which all we do is detect them and nothing more. > > > In my opinion, a better solution would be for me to get rid of the thead > > code entirely from those patches. So that there is still a user, I can > > replace the thead code with the andes versions. > > The Andes stuff is in the subset he applied though, so... > > > > Since Palmer already pulled in those changes maybe it's too late. There > > is not a critical problem here, but it seems like it's bad practice to > > introduce code without a user. > > ...there is actually a "real" user in xandespmu. I did miss that I meant there is no user of the xtheadvector addition. > "riscv: Extend cpufeature.c to detect vendor extensions" actually > contained the xtheadvector detection though, rather than just the > infrastructure. I think it is probably harmless to have it, but > shouldn't be too hard to quickly drop the thead bits either I suppose > if you're worried about it. And the adding vlenb to the DT patches is unrelated to the subset of the series that was pulled into Palmer's for-next so spinning that off into a different series would be more logical. This is kind of a pointless rabbit hole I am getting into, but when we start splitting up series the code contained in the patches start to diverge from the cover letters that end up in the merge commits. - Charlie > > Cheers, > Conor.
On 15 May 2024 18:47:23 IST, Charlie Jenkins <charlie@rivosinc.com> wrote: >On Wed, May 15, 2024 at 06:30:36PM +0100, Conor Dooley wrote: >> On Wed, May 15, 2024 at 10:18:43AM -0700, Charlie Jenkins wrote: >> > On Wed, May 15, 2024 at 05:56:30PM +0100, Conor Dooley wrote: >> > > On Wed, May 15, 2024 at 09:49:24AM -0700, Charlie Jenkins wrote: >> > > > On Wed, May 15, 2024 at 05:09:34PM +0100, Conor Dooley wrote: >> > > > > From: Conor Dooley <conor.dooley@microchip.com> >> > > > > >> > > > > Commit e47c37c24024 ("riscv: Introduce vendor variants of extension >> > > > > helpers") added includes for the new vendor_extensions.h header in >> > > > > the T-Head and SiFive errata handling code but didn't do so for Andes, >> > > > > resulting in allmodconfig build issues when commit 589e2fc85850 >> > > > > ("riscv: Convert xandespmu to use the vendor extension framework") >> > > > > added a user of a macro defined there. >> > > > > >> > > > > Fixes: 589e2fc85850 ("riscv: Convert xandespmu to use the vendor extension framework") >> > > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> >> > > >> > > > >> > > > I was going to fix this in my next version but was waiting for the >> > > > reviews on the thead stuff. I wasn't anticipating these patches to be >> > > > able to jump the queue :) >> > > >> > > Yah, the reason for that is I asked him to take the non-vector parts of >> > > the series as 6.10 material so that we'd have less stuff movin' around >> > > in cpufeatures.c so that Clement's Zc* + validation changes wouldn't run >> > > into a bunch of conflicts etc. Same reason that I pushed for getting >> > > Andy's vector subset stuff merged today, but that mighta been before you >> > > hopped in. >> > > >> > > Cheers, >> > > Conor. >> > >> > Yes I was a couple minutes late to the meeting, whoops. >> >> >> It's prob at like 0600 for you, so w/e. >> >> > The subset of >> > patches that was pulled into for-next is odd to me because there is some >> > of the thead enablement code as part of the vendor extension enablement >> > so that there was a user for it. Since the subset on Palmer's for-next >> > does not have the rest of the thead code there is only a >> > half-implementation of the thead code, it allows the kernel to probe for >> > xtheadvector but it doesn't probe anywhere. >> >> I dunno, I think that reporting that the extension is there constitutes a >> user, it's not gonna be dead code. There's plenty of extensions for >> which all we do is detect them and nothing more. >> >> > In my opinion, a better solution would be for me to get rid of the thead >> > code entirely from those patches. So that there is still a user, I can >> > replace the thead code with the andes versions. >> >> The Andes stuff is in the subset he applied though, so... >> > >> > Since Palmer already pulled in those changes maybe it's too late. There >> > is not a critical problem here, but it seems like it's bad practice to >> > introduce code without a user. >> >> ...there is actually a "real" user in xandespmu. I did miss that > >I meant there is no user of the xtheadvector addition. > >> "riscv: Extend cpufeature.c to detect vendor extensions" actually >> contained the xtheadvector detection though, rather than just the >> infrastructure. I think it is probably harmless to have it, but >> shouldn't be too hard to quickly drop the thead bits either I suppose >> if you're worried about it. > >And the adding vlenb to the DT patches is unrelated to the subset of the >series that was pulled into Palmer's for-next so spinning that off into >a different series would be more logical. This is kind of a pointless >rabbit hole I am getting into, but when we start splitting up series >the code contained in the patches start to diverge from the cover >letters that end up in the merge commits. The vlenb stuff is also one of the things that I want, it's useful for the validation stuff that Clement is adding.
On Wed, May 15, 2024 at 07:21:16PM +0100, Conor Dooley wrote: > > > On 15 May 2024 18:47:23 IST, Charlie Jenkins <charlie@rivosinc.com> wrote: > >On Wed, May 15, 2024 at 06:30:36PM +0100, Conor Dooley wrote: > >> On Wed, May 15, 2024 at 10:18:43AM -0700, Charlie Jenkins wrote: > >> > On Wed, May 15, 2024 at 05:56:30PM +0100, Conor Dooley wrote: > >> > > On Wed, May 15, 2024 at 09:49:24AM -0700, Charlie Jenkins wrote: > >> > > > On Wed, May 15, 2024 at 05:09:34PM +0100, Conor Dooley wrote: > >> > > > > From: Conor Dooley <conor.dooley@microchip.com> > >> > > > > > >> > > > > Commit e47c37c24024 ("riscv: Introduce vendor variants of extension > >> > > > > helpers") added includes for the new vendor_extensions.h header in > >> > > > > the T-Head and SiFive errata handling code but didn't do so for Andes, > >> > > > > resulting in allmodconfig build issues when commit 589e2fc85850 > >> > > > > ("riscv: Convert xandespmu to use the vendor extension framework") > >> > > > > added a user of a macro defined there. > >> > > > > > >> > > > > Fixes: 589e2fc85850 ("riscv: Convert xandespmu to use the vendor extension framework") > >> > > > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > >> > > > >> > > > > >> > > > I was going to fix this in my next version but was waiting for the > >> > > > reviews on the thead stuff. I wasn't anticipating these patches to be > >> > > > able to jump the queue :) > >> > > > >> > > Yah, the reason for that is I asked him to take the non-vector parts of > >> > > the series as 6.10 material so that we'd have less stuff movin' around > >> > > in cpufeatures.c so that Clement's Zc* + validation changes wouldn't run > >> > > into a bunch of conflicts etc. Same reason that I pushed for getting > >> > > Andy's vector subset stuff merged today, but that mighta been before you > >> > > hopped in. > >> > > > >> > > Cheers, > >> > > Conor. > >> > > >> > Yes I was a couple minutes late to the meeting, whoops. > >> > >> > >> It's prob at like 0600 for you, so w/e. > >> > >> > The subset of > >> > patches that was pulled into for-next is odd to me because there is some > >> > of the thead enablement code as part of the vendor extension enablement > >> > so that there was a user for it. Since the subset on Palmer's for-next > >> > does not have the rest of the thead code there is only a > >> > half-implementation of the thead code, it allows the kernel to probe for > >> > xtheadvector but it doesn't probe anywhere. > >> > >> I dunno, I think that reporting that the extension is there constitutes a > >> user, it's not gonna be dead code. There's plenty of extensions for > >> which all we do is detect them and nothing more. > >> > >> > In my opinion, a better solution would be for me to get rid of the thead > >> > code entirely from those patches. So that there is still a user, I can > >> > replace the thead code with the andes versions. > >> > >> The Andes stuff is in the subset he applied though, so... > >> > > >> > Since Palmer already pulled in those changes maybe it's too late. There > >> > is not a critical problem here, but it seems like it's bad practice to > >> > introduce code without a user. > >> > >> ...there is actually a "real" user in xandespmu. I did miss that > > > >I meant there is no user of the xtheadvector addition. > > > >> "riscv: Extend cpufeature.c to detect vendor extensions" actually > >> contained the xtheadvector detection though, rather than just the > >> infrastructure. I think it is probably harmless to have it, but > >> shouldn't be too hard to quickly drop the thead bits either I suppose > >> if you're worried about it. > > > >And the adding vlenb to the DT patches is unrelated to the subset of the > >series that was pulled into Palmer's for-next so spinning that off into > >a different series would be more logical. This is kind of a pointless > >rabbit hole I am getting into, but when we start splitting up series > >the code contained in the patches start to diverge from the cover > >letters that end up in the merge commits. > > The vlenb stuff is also one of the things that I want, it's useful for the validation stuff that Clement is adding. It's definitely useful to have and it's ready, I was wondering if it made more sense for me to send it out as a different series to get it merged in.
diff --git a/arch/riscv/errata/andes/errata.c b/arch/riscv/errata/andes/errata.c index a5d96a7a4682..fc1a34faa5f3 100644 --- a/arch/riscv/errata/andes/errata.c +++ b/arch/riscv/errata/andes/errata.c @@ -17,6 +17,7 @@ #include <asm/processor.h> #include <asm/sbi.h> #include <asm/vendorid_list.h> +#include <asm/vendor_extensions.h> #define ANDES_AX45MP_MARCHID 0x8000000000008a45UL #define ANDES_AX45MP_MIMPID 0x500UL