| Message ID | 20200907181659.92449-2-seanga2@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Andes |
| Headers | show |
| Series | riscv: Correctly handle IPIs already pending upon boot | expand |
Hi Sean > Clearing MIP doesn't do anything. Whoops. The following commits should > tackle this problem in a more robust manner. I still not catch your points about that this commit 947263033 really help to fix pending IPIs not clean problem on k210 platform at that time, but you just said it do nothing and remove it here currently. Thanks, Rick > > This reverts commit 9472630337e7c4ac442066b5a752aaa8c3b4d4a6. > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > --- > > arch/riscv/cpu/start.S | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S > index bf9fdf369b..e3222b1ea7 100644 > --- a/arch/riscv/cpu/start.S > +++ b/arch/riscv/cpu/start.S > @@ -65,8 +65,6 @@ _start: > #else > li t0, SIE_SSIE > #endif > - /* Clear any pending IPIs */ > - csrc MODE_PREFIX(ip), t0 > csrs MODE_PREFIX(ie), t0 > #endif > > -- > 2.28.0 >
On 9/9/20 3:50 AM, Rick Chen wrote: > Hi Sean > >> Clearing MIP doesn't do anything. Whoops. The following commits should >> tackle this problem in a more robust manner. > > I still not catch your points about that this commit 947263033 really > help to fix pending IPIs not clean problem on k210 platform at that > time, but you just said it do nothing and remove it here currently. After refactoring the original smp patch to remove the null check, I neglected to re-test with a debug uart enabled. Without doing that test, it is impossible to catch the pending IPI bug. The secondary hart will crash before the serial driver is initialized. I didn't do that test at the time, because I was mostly worried about the secondary hart corrupting the device tree and causing the boot to fail. That failure mode was fixed by 40686c394. So I saw that and thought that the pending IPI issue was solved as well. --Sean >> >> This reverts commit 9472630337e7c4ac442066b5a752aaa8c3b4d4a6. >> >> Signed-off-by: Sean Anderson <seanga2@gmail.com> >> --- >> >> arch/riscv/cpu/start.S | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S >> index bf9fdf369b..e3222b1ea7 100644 >> --- a/arch/riscv/cpu/start.S >> +++ b/arch/riscv/cpu/start.S >> @@ -65,8 +65,6 @@ _start: >> #else >> li t0, SIE_SSIE >> #endif >> - /* Clear any pending IPIs */ >> - csrc MODE_PREFIX(ip), t0 >> csrs MODE_PREFIX(ie), t0 >> #endif >> >> -- >> 2.28.0 >>
Hi Sean > On 9/9/20 3:50 AM, Rick Chen wrote: > > Hi Sean > > > >> Clearing MIP doesn't do anything. Whoops. The following commits should > >> tackle this problem in a more robust manner. > > > > I still not catch your points about that this commit 947263033 really > > help to fix pending IPIs not clean problem on k210 platform at that > > time, but you just said it do nothing and remove it here currently. > > After refactoring the original smp patch to remove the null check, I > neglected to re-test with a debug uart enabled. Without doing that test, > it is impossible to catch the pending IPI bug. The secondary hart will > crash before the serial driver is initialized. I didn't do that test at > the time, because I was mostly worried about the secondary hart > corrupting the device tree and causing the boot to fail. That failure > mode was fixed by 40686c394. So I saw that and thought that the pending > IPI issue was solved as well. Can you describe more clearly why with a debug uart enabled will trigger the pending IPI bug ? And why the pending IPI be raised and not clear before jump to U-Boot ? Why the csrc MODE_PREFIX(ip), t0 don't help to fix this bug ? Thanks, Rick > > --Sean > > >> > >> This reverts commit 9472630337e7c4ac442066b5a752aaa8c3b4d4a6. > >> > >> Signed-off-by: Sean Anderson <seanga2@gmail.com> > >> --- > >> > >> arch/riscv/cpu/start.S | 2 -- > >> 1 file changed, 2 deletions(-) > >> > >> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S > >> index bf9fdf369b..e3222b1ea7 100644 > >> --- a/arch/riscv/cpu/start.S > >> +++ b/arch/riscv/cpu/start.S > >> @@ -65,8 +65,6 @@ _start: > >> #else > >> li t0, SIE_SSIE > >> #endif > >> - /* Clear any pending IPIs */ > >> - csrc MODE_PREFIX(ip), t0 > >> csrs MODE_PREFIX(ie), t0 > >> #endif > >> > >> -- > >> 2.28.0 > >> >
On 9/10/20 2:39 AM, Rick Chen wrote: > Hi Sean > >> On 9/9/20 3:50 AM, Rick Chen wrote: >>> Hi Sean >>> >>>> Clearing MIP doesn't do anything. Whoops. The following commits should >>>> tackle this problem in a more robust manner. >>> >>> I still not catch your points about that this commit 947263033 really >>> help to fix pending IPIs not clean problem on k210 platform at that >>> time, but you just said it do nothing and remove it here currently. >> >> After refactoring the original smp patch to remove the null check, I >> neglected to re-test with a debug uart enabled. Without doing that test, >> it is impossible to catch the pending IPI bug. The secondary hart will >> crash before the serial driver is initialized. I didn't do that test at >> the time, because I was mostly worried about the secondary hart >> corrupting the device tree and causing the boot to fail. That failure >> mode was fixed by 40686c394. So I saw that and thought that the pending >> IPI issue was solved as well. > > Can you describe more clearly why with a debug uart enabled will > trigger the pending IPI bug ? It doesn't trigger the bug, it always happens. However, if there is no debug uart nothing gets printed, because it occurs before the serial driver is initialized. > > And why the pending IPI be raised and not clear before jump to U-Boot ? I don't know. Presumably the boot rom raises it to signal to jump to U-Boot and never clears it. > > Why the > csrc MODE_PREFIX(ip), t0 > don't help to fix this bug ? The problem is that MSIP in MIP is not necessarily writable. > Each lower privilege level has a separate software interrupt-pending > bit (HSIP, SSIP, USIP), which can be both read and written by CSR > accesses from code running on the local hart at the associated or any > higher privilege level. The machine-level MSIP bits are written by > accesses to memory- mapped control registers, which are used by remote > harts to provide machine-mode interprocessor interrupts. > Interprocessor interrupts for lower privilege levels are implemented > through ABI, SBI or HBI calls to the AEE, SEE or HEE respectively, > which might ultimately result in a machine- mode write to the > receiving hart’s MSIP bit. A hart can write its own MSIP bit using the > same memory-mapped control register. Contrast for example with SSIP in SIP > Three types of interrupts are defined: software interrupts, timer > interrupts, and external interrupts. A supervisor-level software > interrupt is triggered on the current hart by writing 1 to its > supervisor software interrupt-pending (SSIP) bit in the sip register. > A pending supervisor-level software interrupt can be cleared by > writing 0 to the SSIP bit in sip. Supervisor-level software interrupts > are disabled when the SSIE bit in the sie register is clear. I originally added this at your suggestion, but I never ended up testing it separately from the IPI cleanup patch. --Sean
Hi Sean, On Tue, Sep 8, 2020 at 2:17 AM Sean Anderson <seanga2@gmail.com> wrote: > > Clearing MIP doesn't do anything. Whoops. The following commits should Which following commits? > tackle this problem in a more robust manner. > > This reverts commit 9472630337e7c4ac442066b5a752aaa8c3b4d4a6. > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > --- > > arch/riscv/cpu/start.S | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S > index bf9fdf369b..e3222b1ea7 100644 > --- a/arch/riscv/cpu/start.S > +++ b/arch/riscv/cpu/start.S > @@ -65,8 +65,6 @@ _start: > #else > li t0, SIE_SSIE > #endif > - /* Clear any pending IPIs */ > - csrc MODE_PREFIX(ip), t0 Did you mean the clearing MIP.MSIP actually does nothing, but the following commit is the correct fix? commit 40686c394e533fec765fe237936e353c84e73fff Author: Sean Anderson <seanga2@gmail.com> Date: Wed Jun 24 06:41:18 2020 -0400 riscv: Clean up IPI initialization code The previous IPI code initialized the device whenever the first call was made to a riscv_*_ipi function. This made it difficult to determine when the IPI device was initialized. This patch introduces a new function riscv_init_ipi. It is called once during arch_cpu_init_dm. In SPL, it is called in spl_invoke_opensbi. Before this point, no riscv_*_ipi functions should be called. Signed-off-by: Sean Anderson <seanga2@gmail.com> Reviewed-by: Rick Chen <rick@andestech.com> > csrs MODE_PREFIX(ie), t0 > #endif > Regards, Bin
On 9/11/20 3:38 AM, Bin Meng wrote: > Hi Sean, > > On Tue, Sep 8, 2020 at 2:17 AM Sean Anderson <seanga2@gmail.com> wrote: >> >> Clearing MIP doesn't do anything. Whoops. The following commits should > > Which following commits? > >> tackle this problem in a more robust manner. >> >> This reverts commit 9472630337e7c4ac442066b5a752aaa8c3b4d4a6. >> >> Signed-off-by: Sean Anderson <seanga2@gmail.com> >> --- >> >> arch/riscv/cpu/start.S | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S >> index bf9fdf369b..e3222b1ea7 100644 >> --- a/arch/riscv/cpu/start.S >> +++ b/arch/riscv/cpu/start.S >> @@ -65,8 +65,6 @@ _start: >> #else >> li t0, SIE_SSIE >> #endif >> - /* Clear any pending IPIs */ >> - csrc MODE_PREFIX(ip), t0 > > Did you mean the clearing MIP.MSIP actually does nothing, but the > following commit is the correct fix? Yes, but we also need [PATCH 3/7] riscv: Use NULL as a sentinel value for smp_call_function so the secondary harts know not to take any IPIs not raised by U-Boot. > > commit 40686c394e533fec765fe237936e353c84e73fff > Author: Sean Anderson <seanga2@gmail.com> > Date: Wed Jun 24 06:41:18 2020 -0400 > > riscv: Clean up IPI initialization code > > The previous IPI code initialized the device whenever the first call was > made to a riscv_*_ipi function. This made it difficult to determine when > the IPI device was initialized. This patch introduces a new function > riscv_init_ipi. It is called once during arch_cpu_init_dm. In SPL, it is > called in spl_invoke_opensbi. Before this point, no riscv_*_ipi functions > should be called. > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > Reviewed-by: Rick Chen <rick@andestech.com> > >> csrs MODE_PREFIX(ie), t0 >> #endif >> --Sean
On Fri, Sep 11, 2020 at 6:22 PM Sean Anderson <seanga2@gmail.com> wrote: > > On 9/11/20 3:38 AM, Bin Meng wrote: > > Hi Sean, > > > > On Tue, Sep 8, 2020 at 2:17 AM Sean Anderson <seanga2@gmail.com> wrote: > >> > >> Clearing MIP doesn't do anything. Whoops. The following commits should > > > > Which following commits? > > > >> tackle this problem in a more robust manner. > >> > >> This reverts commit 9472630337e7c4ac442066b5a752aaa8c3b4d4a6. > >> > >> Signed-off-by: Sean Anderson <seanga2@gmail.com> > >> --- > >> > >> arch/riscv/cpu/start.S | 2 -- > >> 1 file changed, 2 deletions(-) > >> > >> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S > >> index bf9fdf369b..e3222b1ea7 100644 > >> --- a/arch/riscv/cpu/start.S > >> +++ b/arch/riscv/cpu/start.S > >> @@ -65,8 +65,6 @@ _start: > >> #else > >> li t0, SIE_SSIE > >> #endif > >> - /* Clear any pending IPIs */ > >> - csrc MODE_PREFIX(ip), t0 > > > > Did you mean the clearing MIP.MSIP actually does nothing, but the > > following commit is the correct fix? > > Yes, but we also need Is MIP.MSIP read-only on K210? > > [PATCH 3/7] riscv: Use NULL as a sentinel value for smp_call_function > > so the secondary harts know not to take any IPIs not raised by U-Boot. > > > > > commit 40686c394e533fec765fe237936e353c84e73fff > > Author: Sean Anderson <seanga2@gmail.com> > > Date: Wed Jun 24 06:41:18 2020 -0400 > > > > riscv: Clean up IPI initialization code > > > > The previous IPI code initialized the device whenever the first call was > > made to a riscv_*_ipi function. This made it difficult to determine when > > the IPI device was initialized. This patch introduces a new function > > riscv_init_ipi. It is called once during arch_cpu_init_dm. In SPL, it is > > called in spl_invoke_opensbi. Before this point, no riscv_*_ipi functions > > should be called. > > > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > > Reviewed-by: Rick Chen <rick@andestech.com> > > > >> csrs MODE_PREFIX(ie), t0 > >> #endif Regards, Bin
On 9/11/20 10:45 AM, Bin Meng wrote: > On Fri, Sep 11, 2020 at 6:22 PM Sean Anderson <seanga2@gmail.com> wrote: >> >> On 9/11/20 3:38 AM, Bin Meng wrote: >>> Hi Sean, >>> >>> On Tue, Sep 8, 2020 at 2:17 AM Sean Anderson <seanga2@gmail.com> wrote: >>>> >>>> Clearing MIP doesn't do anything. Whoops. The following commits should >>> >>> Which following commits? >>> >>>> tackle this problem in a more robust manner. >>>> >>>> This reverts commit 9472630337e7c4ac442066b5a752aaa8c3b4d4a6. >>>> >>>> Signed-off-by: Sean Anderson <seanga2@gmail.com> >>>> --- >>>> >>>> arch/riscv/cpu/start.S | 2 -- >>>> 1 file changed, 2 deletions(-) >>>> >>>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S >>>> index bf9fdf369b..e3222b1ea7 100644 >>>> --- a/arch/riscv/cpu/start.S >>>> +++ b/arch/riscv/cpu/start.S >>>> @@ -65,8 +65,6 @@ _start: >>>> #else >>>> li t0, SIE_SSIE >>>> #endif >>>> - /* Clear any pending IPIs */ >>>> - csrc MODE_PREFIX(ip), t0 >>> >>> Did you mean the clearing MIP.MSIP actually does nothing, but the >>> following commit is the correct fix? >> >> Yes, but we also need > > Is MIP.MSIP read-only on K210? I think so. See [1] where only ssip, stip, and seip are written (and new_mip is not otherwise used). The spec doesn't require MIP.MSIP to be writable at all. --Sean [1] https://github.com/chipsalliance/rocket-chip/blob/master/src/main/scala/rocket/CSR.scala#L859
HI Sean > On 9/11/20 10:45 AM, Bin Meng wrote: > > On Fri, Sep 11, 2020 at 6:22 PM Sean Anderson <seanga2@gmail.com> wrote: > >> > >> On 9/11/20 3:38 AM, Bin Meng wrote: > >>> Hi Sean, > >>> > >>> On Tue, Sep 8, 2020 at 2:17 AM Sean Anderson <seanga2@gmail.com> wrote: > >>>> > >>>> Clearing MIP doesn't do anything. Whoops. The following commits should > >>> > >>> Which following commits? > >>> > >>>> tackle this problem in a more robust manner. > >>>> > >>>> This reverts commit 9472630337e7c4ac442066b5a752aaa8c3b4d4a6. > >>>> > >>>> Signed-off-by: Sean Anderson <seanga2@gmail.com> > >>>> --- > >>>> > >>>> arch/riscv/cpu/start.S | 2 -- > >>>> 1 file changed, 2 deletions(-) > >>>> > >>>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S > >>>> index bf9fdf369b..e3222b1ea7 100644 > >>>> --- a/arch/riscv/cpu/start.S > >>>> +++ b/arch/riscv/cpu/start.S > >>>> @@ -65,8 +65,6 @@ _start: > >>>> #else > >>>> li t0, SIE_SSIE > >>>> #endif > >>>> - /* Clear any pending IPIs */ > >>>> - csrc MODE_PREFIX(ip), t0 > >>> > >>> Did you mean the clearing MIP.MSIP actually does nothing, but the > >>> following commit is the correct fix? > >> > >> Yes, but we also need > > > > Is MIP.MSIP read-only on K210? Since clear mip will not affect anything in K210 and it is writable for other RISC-V platforms. I will prefer to keep this instruction stay here for standard startup initialization. Thanks, Rick > > I think so. See [1] where only ssip, stip, and seip are written (and > new_mip is not otherwise used). The spec doesn't require MIP.MSIP to be > writable at all. > > --Sean > > [1] https://github.com/chipsalliance/rocket-chip/blob/master/src/main/scala/rocket/CSR.scala#L859
On 9/13/20 11:10 PM, Rick Chen wrote: > HI Sean > >> On 9/11/20 10:45 AM, Bin Meng wrote: >>> On Fri, Sep 11, 2020 at 6:22 PM Sean Anderson <seanga2@gmail.com> wrote: >>>> >>>> On 9/11/20 3:38 AM, Bin Meng wrote: >>>>> Hi Sean, >>>>> >>>>> On Tue, Sep 8, 2020 at 2:17 AM Sean Anderson <seanga2@gmail.com> wrote: >>>>>> >>>>>> Clearing MIP doesn't do anything. Whoops. The following commits should >>>>> >>>>> Which following commits? >>>>> >>>>>> tackle this problem in a more robust manner. >>>>>> >>>>>> This reverts commit 9472630337e7c4ac442066b5a752aaa8c3b4d4a6. >>>>>> >>>>>> Signed-off-by: Sean Anderson <seanga2@gmail.com> >>>>>> --- >>>>>> >>>>>> arch/riscv/cpu/start.S | 2 -- >>>>>> 1 file changed, 2 deletions(-) >>>>>> >>>>>> diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S >>>>>> index bf9fdf369b..e3222b1ea7 100644 >>>>>> --- a/arch/riscv/cpu/start.S >>>>>> +++ b/arch/riscv/cpu/start.S >>>>>> @@ -65,8 +65,6 @@ _start: >>>>>> #else >>>>>> li t0, SIE_SSIE >>>>>> #endif >>>>>> - /* Clear any pending IPIs */ >>>>>> - csrc MODE_PREFIX(ip), t0 >>>>> >>>>> Did you mean the clearing MIP.MSIP actually does nothing, but the >>>>> following commit is the correct fix? >>>> >>>> Yes, but we also need >>> >>> Is MIP.MSIP read-only on K210? > > Since clear mip will not affect anything in K210 and it is writable > for other RISC-V platforms. Does it do anything for other RISC-V platforms? I checked the manuals for the Sifive fu540 and fe310 and the Nuclei Bumblebee (the core Gigadevice's GD32VF103 series), and none of them do anything with writes to MIP. Does Andes do anything with writes in their cores? At the very least I think the comment should be changed so as not to mislead readers. > I will prefer to keep this instruction stay here for standard startup > initialization. I would prefer not to, since the rest of this series should handle the original intent of this commit in a much more robust manner. --Sean
diff --git a/arch/riscv/cpu/start.S b/arch/riscv/cpu/start.S index bf9fdf369b..e3222b1ea7 100644 --- a/arch/riscv/cpu/start.S +++ b/arch/riscv/cpu/start.S @@ -65,8 +65,6 @@ _start: #else li t0, SIE_SSIE #endif - /* Clear any pending IPIs */ - csrc MODE_PREFIX(ip), t0 csrs MODE_PREFIX(ie), t0 #endif
Clearing MIP doesn't do anything. Whoops. The following commits should tackle this problem in a more robust manner. This reverts commit 9472630337e7c4ac442066b5a752aaa8c3b4d4a6. Signed-off-by: Sean Anderson <seanga2@gmail.com> --- arch/riscv/cpu/start.S | 2 -- 1 file changed, 2 deletions(-)