* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
@ 2022-03-30 16:11 ` Palmer Dabbelt
0 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2022-03-30 16:11 UTC (permalink / raw)
To: phantom, linux-riscv
Cc: idan.horowitz, Alistair Francis, qemu-riscv, qemu-devel
[re-ordering the top post]
+linux-riscv, as this may very well be a kernel bug
On Wed, 30 Mar 2022 05:38:30 PDT (-0700), phantom@zju.edu.cn wrote:
>> -----Original Messages-----
>> From: "Idan Horowitz" <idan.horowitz@gmail.com>
>> Sent Time: 2022-03-30 15:35:19 (Wednesday)
>> To: "Atish Patra" <atishp@atishpatra.org>
>> Cc: phantom@zju.edu.cn, "open list:RISC-V" <qemu-riscv@nongnu.org>, "Alistair Francis" <Alistair.Francis@wdc.com>, "qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>
>> Subject: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
>>
>> On Wed, 30 Mar 2022 at 10:28, Atish Patra <atishp@atishpatra.org> wrote:
>> >
>> > I tested on v5.17 built from defconfig for rv64.
>> >
>> > Here is the kernel code executing sfence.vma
>> > https://elixir.bootlin.com/linux/v5.17/source/arch/riscv/kernel/head.S#L122
>> >
>>
>> I believe this is a kernel bug and not a QEMU one. They perform a
>> write to the SATP with the same ASID as the one used before (0) and
I seem to remember ASID 0 being a special one, with some global-ness,
but I couldn't find that in a quick poke into the spec. As below, I'm
going to read through this a bit more...
(Also, I haven't had any coffee yet)
>> then expect it to be used, without performing an sfence.vma following
>> it.
>> This was exposed by my change, as previously the write to the satp was
>> performed in the same TB block as the sfence.vma *before it*, which
>> meant the TLB was not filled in between the previous sfence and the
>> write to SATP following it.
>> I was able to reproduce the issue with the Fedora Rawhide image in the
>> wiki, and I was able to resolve it by artificially forcing a TLB flush
>> following all writes to SATP.
>> I think the correct course of action here is to:
>> 1. Report the issue to the linux kernel mailing list and/or contribute
>> a patch that adds said missing sfence.vma following the SATP write.
>> (Atish: Are you able to test if adding an sfence.vma in your kernel
>> build fixes the issue?)
If it's a kernel bug we should fix it, but I'm not entirely convinced
that's the case. I can confirm that the following makes Linux boot
again
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index ec07f991866a..83373c2bd7d0 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -121,6 +121,7 @@ relocate:
or a0, a0, a1
sfence.vma
csrw CSR_SATP, a0
+ sfence.vma
.align 2
1:
/* Set trap vector to spin forever to help debug */
It's been a while since I read through the rules here so I'm going to go
read them again, but IIRC that shouldn't be necessary: that first
sfence.vma should be sufficiently global to ensure all prior writes to
the page tables are visible, regardless of what's in SATP. That said, I
remember there being a lot of subtly here in the spec wording so I'm
going to go read the spec again.
>> 2. Restore the patch
Presumably you mean "revert" here? That might be the right way to go,
just to avoid breaking users (even if we fix the kernel bug, it'll take
a while to get everyone to update). That said, this smells like the
sort of thing that's going to crop up at arbitrary times in dynamic
systems so while a revert looks like it'd work around the boot issue we
might be making more headaches for folks down the road.
> I agree with you partly, my test case is actually from linux kernel, I notice
> the strange sfence.vma before write satp during write our teaching kernel.
> I think, the strange code is used to bypass the qemu bug that Idan patched.
> Because in hardware, if the stap is empty, sfence.vma will do nothing.
> And that's why nobody report it.
IIUC the sfence.vma before the SATP write is very explicitly necessary:
without that fence old mappings could be utilized directly after the
SATP write, so we might not even be able to fetch the next instruction.
> Before patch, qemu won't end a BB after sfence (but jump and CSR write do).
> So, the kernel author reodered write stap and sfence.vma to make sfence.vma
> place in the same BB with write satp, instead of the following write stvec.
> (If don't reorder, sfence.vma will place in the same BB with write stvec,
> that will crash kernel, see my origin analysis).
>
> However, in hardware, since tlb is empty, put the first sfence.vma before or
> after write satp is not really matters.
> In qemu, as Atish's log shown, we should do a imply invisible sfence.vma after
> write stap to invalid qemu's translation cache.
The ISA manual is quite explicit about SATP not enforcing these
orderings. If what I remember about ASID 0 is true then I do think we'd
need one, though, to avoid the aliasing -- hopefully I'll make a bit
more sense soon, though...
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
@ 2022-03-30 16:11 ` Palmer Dabbelt
0 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2022-03-30 16:11 UTC (permalink / raw)
To: phantom, linux-riscv
Cc: Alistair Francis, idan.horowitz, qemu-riscv, qemu-devel
[re-ordering the top post]
+linux-riscv, as this may very well be a kernel bug
On Wed, 30 Mar 2022 05:38:30 PDT (-0700), phantom@zju.edu.cn wrote:
>> -----Original Messages-----
>> From: "Idan Horowitz" <idan.horowitz@gmail.com>
>> Sent Time: 2022-03-30 15:35:19 (Wednesday)
>> To: "Atish Patra" <atishp@atishpatra.org>
>> Cc: phantom@zju.edu.cn, "open list:RISC-V" <qemu-riscv@nongnu.org>, "Alistair Francis" <Alistair.Francis@wdc.com>, "qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>
>> Subject: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
>>
>> On Wed, 30 Mar 2022 at 10:28, Atish Patra <atishp@atishpatra.org> wrote:
>> >
>> > I tested on v5.17 built from defconfig for rv64.
>> >
>> > Here is the kernel code executing sfence.vma
>> > https://elixir.bootlin.com/linux/v5.17/source/arch/riscv/kernel/head.S#L122
>> >
>>
>> I believe this is a kernel bug and not a QEMU one. They perform a
>> write to the SATP with the same ASID as the one used before (0) and
I seem to remember ASID 0 being a special one, with some global-ness,
but I couldn't find that in a quick poke into the spec. As below, I'm
going to read through this a bit more...
(Also, I haven't had any coffee yet)
>> then expect it to be used, without performing an sfence.vma following
>> it.
>> This was exposed by my change, as previously the write to the satp was
>> performed in the same TB block as the sfence.vma *before it*, which
>> meant the TLB was not filled in between the previous sfence and the
>> write to SATP following it.
>> I was able to reproduce the issue with the Fedora Rawhide image in the
>> wiki, and I was able to resolve it by artificially forcing a TLB flush
>> following all writes to SATP.
>> I think the correct course of action here is to:
>> 1. Report the issue to the linux kernel mailing list and/or contribute
>> a patch that adds said missing sfence.vma following the SATP write.
>> (Atish: Are you able to test if adding an sfence.vma in your kernel
>> build fixes the issue?)
If it's a kernel bug we should fix it, but I'm not entirely convinced
that's the case. I can confirm that the following makes Linux boot
again
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index ec07f991866a..83373c2bd7d0 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -121,6 +121,7 @@ relocate:
or a0, a0, a1
sfence.vma
csrw CSR_SATP, a0
+ sfence.vma
.align 2
1:
/* Set trap vector to spin forever to help debug */
It's been a while since I read through the rules here so I'm going to go
read them again, but IIRC that shouldn't be necessary: that first
sfence.vma should be sufficiently global to ensure all prior writes to
the page tables are visible, regardless of what's in SATP. That said, I
remember there being a lot of subtly here in the spec wording so I'm
going to go read the spec again.
>> 2. Restore the patch
Presumably you mean "revert" here? That might be the right way to go,
just to avoid breaking users (even if we fix the kernel bug, it'll take
a while to get everyone to update). That said, this smells like the
sort of thing that's going to crop up at arbitrary times in dynamic
systems so while a revert looks like it'd work around the boot issue we
might be making more headaches for folks down the road.
> I agree with you partly, my test case is actually from linux kernel, I notice
> the strange sfence.vma before write satp during write our teaching kernel.
> I think, the strange code is used to bypass the qemu bug that Idan patched.
> Because in hardware, if the stap is empty, sfence.vma will do nothing.
> And that's why nobody report it.
IIUC the sfence.vma before the SATP write is very explicitly necessary:
without that fence old mappings could be utilized directly after the
SATP write, so we might not even be able to fetch the next instruction.
> Before patch, qemu won't end a BB after sfence (but jump and CSR write do).
> So, the kernel author reodered write stap and sfence.vma to make sfence.vma
> place in the same BB with write satp, instead of the following write stvec.
> (If don't reorder, sfence.vma will place in the same BB with write stvec,
> that will crash kernel, see my origin analysis).
>
> However, in hardware, since tlb is empty, put the first sfence.vma before or
> after write satp is not really matters.
> In qemu, as Atish's log shown, we should do a imply invisible sfence.vma after
> write stap to invalid qemu's translation cache.
The ISA manual is quite explicit about SATP not enforcing these
orderings. If what I remember about ASID 0 is true then I do think we'd
need one, though, to avoid the aliasing -- hopefully I'll make a bit
more sense soon, though...
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
2022-03-30 16:11 ` Palmer Dabbelt
(?)
@ 2022-03-30 17:06 ` Palmer Dabbelt
-1 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2022-03-30 17:06 UTC (permalink / raw)
To: phantom
Cc: linux-riscv, idan.horowitz, Alistair Francis, qemu-riscv, qemu-devel
On Wed, 30 Mar 2022 09:11:18 PDT (-0700), Palmer Dabbelt wrote:
> [re-ordering the top post]
>
> +linux-riscv, as this may very well be a kernel bug
>
> On Wed, 30 Mar 2022 05:38:30 PDT (-0700), phantom@zju.edu.cn wrote:
>>> -----Original Messages-----
>>> From: "Idan Horowitz" <idan.horowitz@gmail.com>
>>> Sent Time: 2022-03-30 15:35:19 (Wednesday)
>>> To: "Atish Patra" <atishp@atishpatra.org>
>>> Cc: phantom@zju.edu.cn, "open list:RISC-V" <qemu-riscv@nongnu.org>, "Alistair Francis" <Alistair.Francis@wdc.com>, "qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>
>>> Subject: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
>>>
>>> On Wed, 30 Mar 2022 at 10:28, Atish Patra <atishp@atishpatra.org> wrote:
>>> >
>>> > I tested on v5.17 built from defconfig for rv64.
>>> >
>>> > Here is the kernel code executing sfence.vma
>>> > https://elixir.bootlin.com/linux/v5.17/source/arch/riscv/kernel/head.S#L122
>>> >
>>>
>>> I believe this is a kernel bug and not a QEMU one. They perform a
>>> write to the SATP with the same ASID as the one used before (0) and
>
> I seem to remember ASID 0 being a special one, with some global-ness,
> but I couldn't find that in a quick poke into the spec. As below, I'm
> going to read through this a bit more...
>
> (Also, I haven't had any coffee yet)
>
>>> then expect it to be used, without performing an sfence.vma following
>>> it.
>>> This was exposed by my change, as previously the write to the satp was
>>> performed in the same TB block as the sfence.vma *before it*, which
>>> meant the TLB was not filled in between the previous sfence and the
>>> write to SATP following it.
>>> I was able to reproduce the issue with the Fedora Rawhide image in the
>>> wiki, and I was able to resolve it by artificially forcing a TLB flush
>>> following all writes to SATP.
>>> I think the correct course of action here is to:
>>> 1. Report the issue to the linux kernel mailing list and/or contribute
>>> a patch that adds said missing sfence.vma following the SATP write.
>>> (Atish: Are you able to test if adding an sfence.vma in your kernel
>>> build fixes the issue?)
>
> If it's a kernel bug we should fix it, but I'm not entirely convinced
> that's the case. I can confirm that the following makes Linux boot
> again
>
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index ec07f991866a..83373c2bd7d0 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -121,6 +121,7 @@ relocate:
> or a0, a0, a1
> sfence.vma
> csrw CSR_SATP, a0
> + sfence.vma
> .align 2
> 1:
> /* Set trap vector to spin forever to help debug */
>
> It's been a while since I read through the rules here so I'm going to go
> read them again, but IIRC that shouldn't be necessary: that first
> sfence.vma should be sufficiently global to ensure all prior writes to
> the page tables are visible, regardless of what's in SATP. That said, I
> remember there being a lot of subtly here in the spec wording so I'm
> going to go read the spec again.
>
>>> 2. Restore the patch
>
> Presumably you mean "revert" here? That might be the right way to go,
> just to avoid breaking users (even if we fix the kernel bug, it'll take
> a while to get everyone to update). That said, this smells like the
> sort of thing that's going to crop up at arbitrary times in dynamic
> systems so while a revert looks like it'd work around the boot issue we
> might be making more headaches for folks down the road.
>
>> I agree with you partly, my test case is actually from linux kernel, I notice
>> the strange sfence.vma before write satp during write our teaching kernel.
>> I think, the strange code is used to bypass the qemu bug that Idan patched.
>> Because in hardware, if the stap is empty, sfence.vma will do nothing.
>> And that's why nobody report it.
>
> IIUC the sfence.vma before the SATP write is very explicitly necessary:
> without that fence old mappings could be utilized directly after the
> SATP write, so we might not even be able to fetch the next instruction.
>
>> Before patch, qemu won't end a BB after sfence (but jump and CSR write do).
>> So, the kernel author reodered write stap and sfence.vma to make sfence.vma
>> place in the same BB with write satp, instead of the following write stvec.
>> (If don't reorder, sfence.vma will place in the same BB with write stvec,
>> that will crash kernel, see my origin analysis).
>>
>> However, in hardware, since tlb is empty, put the first sfence.vma before or
>> after write satp is not really matters.
>> In qemu, as Atish's log shown, we should do a imply invisible sfence.vma after
>> write stap to invalid qemu's translation cache.
>
> The ISA manual is quite explicit about SATP not enforcing these
> orderings. If what I remember about ASID 0 is true then I do think we'd
> need one, though, to avoid the aliasing -- hopefully I'll make a bit
> more sense soon, though...
OK, I must have just been crazy -- there's nothing special about ASID=0.
Looks to me like flushing the TLB on SATP writes is necessary, I just
sent a patch with a more concrete description of why.
Thanks!
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
@ 2022-03-30 17:06 ` Palmer Dabbelt
0 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2022-03-30 17:06 UTC (permalink / raw)
To: phantom
Cc: linux-riscv, idan.horowitz, Alistair Francis, qemu-riscv, qemu-devel
On Wed, 30 Mar 2022 09:11:18 PDT (-0700), Palmer Dabbelt wrote:
> [re-ordering the top post]
>
> +linux-riscv, as this may very well be a kernel bug
>
> On Wed, 30 Mar 2022 05:38:30 PDT (-0700), phantom@zju.edu.cn wrote:
>>> -----Original Messages-----
>>> From: "Idan Horowitz" <idan.horowitz@gmail.com>
>>> Sent Time: 2022-03-30 15:35:19 (Wednesday)
>>> To: "Atish Patra" <atishp@atishpatra.org>
>>> Cc: phantom@zju.edu.cn, "open list:RISC-V" <qemu-riscv@nongnu.org>, "Alistair Francis" <Alistair.Francis@wdc.com>, "qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>
>>> Subject: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
>>>
>>> On Wed, 30 Mar 2022 at 10:28, Atish Patra <atishp@atishpatra.org> wrote:
>>> >
>>> > I tested on v5.17 built from defconfig for rv64.
>>> >
>>> > Here is the kernel code executing sfence.vma
>>> > https://elixir.bootlin.com/linux/v5.17/source/arch/riscv/kernel/head.S#L122
>>> >
>>>
>>> I believe this is a kernel bug and not a QEMU one. They perform a
>>> write to the SATP with the same ASID as the one used before (0) and
>
> I seem to remember ASID 0 being a special one, with some global-ness,
> but I couldn't find that in a quick poke into the spec. As below, I'm
> going to read through this a bit more...
>
> (Also, I haven't had any coffee yet)
>
>>> then expect it to be used, without performing an sfence.vma following
>>> it.
>>> This was exposed by my change, as previously the write to the satp was
>>> performed in the same TB block as the sfence.vma *before it*, which
>>> meant the TLB was not filled in between the previous sfence and the
>>> write to SATP following it.
>>> I was able to reproduce the issue with the Fedora Rawhide image in the
>>> wiki, and I was able to resolve it by artificially forcing a TLB flush
>>> following all writes to SATP.
>>> I think the correct course of action here is to:
>>> 1. Report the issue to the linux kernel mailing list and/or contribute
>>> a patch that adds said missing sfence.vma following the SATP write.
>>> (Atish: Are you able to test if adding an sfence.vma in your kernel
>>> build fixes the issue?)
>
> If it's a kernel bug we should fix it, but I'm not entirely convinced
> that's the case. I can confirm that the following makes Linux boot
> again
>
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index ec07f991866a..83373c2bd7d0 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -121,6 +121,7 @@ relocate:
> or a0, a0, a1
> sfence.vma
> csrw CSR_SATP, a0
> + sfence.vma
> .align 2
> 1:
> /* Set trap vector to spin forever to help debug */
>
> It's been a while since I read through the rules here so I'm going to go
> read them again, but IIRC that shouldn't be necessary: that first
> sfence.vma should be sufficiently global to ensure all prior writes to
> the page tables are visible, regardless of what's in SATP. That said, I
> remember there being a lot of subtly here in the spec wording so I'm
> going to go read the spec again.
>
>>> 2. Restore the patch
>
> Presumably you mean "revert" here? That might be the right way to go,
> just to avoid breaking users (even if we fix the kernel bug, it'll take
> a while to get everyone to update). That said, this smells like the
> sort of thing that's going to crop up at arbitrary times in dynamic
> systems so while a revert looks like it'd work around the boot issue we
> might be making more headaches for folks down the road.
>
>> I agree with you partly, my test case is actually from linux kernel, I notice
>> the strange sfence.vma before write satp during write our teaching kernel.
>> I think, the strange code is used to bypass the qemu bug that Idan patched.
>> Because in hardware, if the stap is empty, sfence.vma will do nothing.
>> And that's why nobody report it.
>
> IIUC the sfence.vma before the SATP write is very explicitly necessary:
> without that fence old mappings could be utilized directly after the
> SATP write, so we might not even be able to fetch the next instruction.
>
>> Before patch, qemu won't end a BB after sfence (but jump and CSR write do).
>> So, the kernel author reodered write stap and sfence.vma to make sfence.vma
>> place in the same BB with write satp, instead of the following write stvec.
>> (If don't reorder, sfence.vma will place in the same BB with write stvec,
>> that will crash kernel, see my origin analysis).
>>
>> However, in hardware, since tlb is empty, put the first sfence.vma before or
>> after write satp is not really matters.
>> In qemu, as Atish's log shown, we should do a imply invisible sfence.vma after
>> write stap to invalid qemu's translation cache.
>
> The ISA manual is quite explicit about SATP not enforcing these
> orderings. If what I remember about ASID 0 is true then I do think we'd
> need one, though, to avoid the aliasing -- hopefully I'll make a bit
> more sense soon, though...
OK, I must have just been crazy -- there's nothing special about ASID=0.
Looks to me like flushing the TLB on SATP writes is necessary, I just
sent a patch with a more concrete description of why.
Thanks!
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
@ 2022-03-30 17:06 ` Palmer Dabbelt
0 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2022-03-30 17:06 UTC (permalink / raw)
To: phantom
Cc: Alistair Francis, linux-riscv, idan.horowitz, qemu-riscv, qemu-devel
On Wed, 30 Mar 2022 09:11:18 PDT (-0700), Palmer Dabbelt wrote:
> [re-ordering the top post]
>
> +linux-riscv, as this may very well be a kernel bug
>
> On Wed, 30 Mar 2022 05:38:30 PDT (-0700), phantom@zju.edu.cn wrote:
>>> -----Original Messages-----
>>> From: "Idan Horowitz" <idan.horowitz@gmail.com>
>>> Sent Time: 2022-03-30 15:35:19 (Wednesday)
>>> To: "Atish Patra" <atishp@atishpatra.org>
>>> Cc: phantom@zju.edu.cn, "open list:RISC-V" <qemu-riscv@nongnu.org>, "Alistair Francis" <Alistair.Francis@wdc.com>, "qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>
>>> Subject: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
>>>
>>> On Wed, 30 Mar 2022 at 10:28, Atish Patra <atishp@atishpatra.org> wrote:
>>> >
>>> > I tested on v5.17 built from defconfig for rv64.
>>> >
>>> > Here is the kernel code executing sfence.vma
>>> > https://elixir.bootlin.com/linux/v5.17/source/arch/riscv/kernel/head.S#L122
>>> >
>>>
>>> I believe this is a kernel bug and not a QEMU one. They perform a
>>> write to the SATP with the same ASID as the one used before (0) and
>
> I seem to remember ASID 0 being a special one, with some global-ness,
> but I couldn't find that in a quick poke into the spec. As below, I'm
> going to read through this a bit more...
>
> (Also, I haven't had any coffee yet)
>
>>> then expect it to be used, without performing an sfence.vma following
>>> it.
>>> This was exposed by my change, as previously the write to the satp was
>>> performed in the same TB block as the sfence.vma *before it*, which
>>> meant the TLB was not filled in between the previous sfence and the
>>> write to SATP following it.
>>> I was able to reproduce the issue with the Fedora Rawhide image in the
>>> wiki, and I was able to resolve it by artificially forcing a TLB flush
>>> following all writes to SATP.
>>> I think the correct course of action here is to:
>>> 1. Report the issue to the linux kernel mailing list and/or contribute
>>> a patch that adds said missing sfence.vma following the SATP write.
>>> (Atish: Are you able to test if adding an sfence.vma in your kernel
>>> build fixes the issue?)
>
> If it's a kernel bug we should fix it, but I'm not entirely convinced
> that's the case. I can confirm that the following makes Linux boot
> again
>
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index ec07f991866a..83373c2bd7d0 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -121,6 +121,7 @@ relocate:
> or a0, a0, a1
> sfence.vma
> csrw CSR_SATP, a0
> + sfence.vma
> .align 2
> 1:
> /* Set trap vector to spin forever to help debug */
>
> It's been a while since I read through the rules here so I'm going to go
> read them again, but IIRC that shouldn't be necessary: that first
> sfence.vma should be sufficiently global to ensure all prior writes to
> the page tables are visible, regardless of what's in SATP. That said, I
> remember there being a lot of subtly here in the spec wording so I'm
> going to go read the spec again.
>
>>> 2. Restore the patch
>
> Presumably you mean "revert" here? That might be the right way to go,
> just to avoid breaking users (even if we fix the kernel bug, it'll take
> a while to get everyone to update). That said, this smells like the
> sort of thing that's going to crop up at arbitrary times in dynamic
> systems so while a revert looks like it'd work around the boot issue we
> might be making more headaches for folks down the road.
>
>> I agree with you partly, my test case is actually from linux kernel, I notice
>> the strange sfence.vma before write satp during write our teaching kernel.
>> I think, the strange code is used to bypass the qemu bug that Idan patched.
>> Because in hardware, if the stap is empty, sfence.vma will do nothing.
>> And that's why nobody report it.
>
> IIUC the sfence.vma before the SATP write is very explicitly necessary:
> without that fence old mappings could be utilized directly after the
> SATP write, so we might not even be able to fetch the next instruction.
>
>> Before patch, qemu won't end a BB after sfence (but jump and CSR write do).
>> So, the kernel author reodered write stap and sfence.vma to make sfence.vma
>> place in the same BB with write satp, instead of the following write stvec.
>> (If don't reorder, sfence.vma will place in the same BB with write stvec,
>> that will crash kernel, see my origin analysis).
>>
>> However, in hardware, since tlb is empty, put the first sfence.vma before or
>> after write satp is not really matters.
>> In qemu, as Atish's log shown, we should do a imply invisible sfence.vma after
>> write stap to invalid qemu's translation cache.
>
> The ISA manual is quite explicit about SATP not enforcing these
> orderings. If what I remember about ASID 0 is true then I do think we'd
> need one, though, to avoid the aliasing -- hopefully I'll make a bit
> more sense soon, though...
OK, I must have just been crazy -- there's nothing special about ASID=0.
Looks to me like flushing the TLB on SATP writes is necessary, I just
sent a patch with a more concrete description of why.
Thanks!
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
2022-03-30 16:11 ` Palmer Dabbelt
(?)
@ 2022-03-30 17:10 ` Idan Horowitz
-1 siblings, 0 replies; 32+ messages in thread
From: Idan Horowitz @ 2022-03-30 17:10 UTC (permalink / raw)
To: Palmer Dabbelt, linux-riscv
Cc: phantom, Alistair Francis, open list:RISC-V,
qemu-devel@nongnu.org Developers
On Wed, 30 Mar 2022 at 19:11, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
>
> Presumably you mean "revert" here? That might be the right way to go,
> just to avoid breaking users (even if we fix the kernel bug, it'll take
> a while to get everyone to update). That said, this smells like the
> sort of thing that's going to crop up at arbitrary times in dynamic
> systems so while a revert looks like it'd work around the boot issue we
> might be making more headaches for folks down the road.
>
The opposite in fact, I did not suggest to revert it, but rather undo
the revert (as Alistair already removed it from the apply-next tree),
since my original patch fixes buggy behaviour that is blocking the
testing of some embedded software on QEMU.
Idan Horowitz
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
@ 2022-03-30 17:10 ` Idan Horowitz
0 siblings, 0 replies; 32+ messages in thread
From: Idan Horowitz @ 2022-03-30 17:10 UTC (permalink / raw)
To: Palmer Dabbelt, linux-riscv
Cc: phantom, Alistair Francis, open list:RISC-V,
qemu-devel@nongnu.org Developers
On Wed, 30 Mar 2022 at 19:11, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
>
> Presumably you mean "revert" here? That might be the right way to go,
> just to avoid breaking users (even if we fix the kernel bug, it'll take
> a while to get everyone to update). That said, this smells like the
> sort of thing that's going to crop up at arbitrary times in dynamic
> systems so while a revert looks like it'd work around the boot issue we
> might be making more headaches for folks down the road.
>
The opposite in fact, I did not suggest to revert it, but rather undo
the revert (as Alistair already removed it from the apply-next tree),
since my original patch fixes buggy behaviour that is blocking the
testing of some embedded software on QEMU.
Idan Horowitz
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
@ 2022-03-30 17:10 ` Idan Horowitz
0 siblings, 0 replies; 32+ messages in thread
From: Idan Horowitz @ 2022-03-30 17:10 UTC (permalink / raw)
To: Palmer Dabbelt, linux-riscv
Cc: open list:RISC-V, phantom, Alistair Francis,
qemu-devel@nongnu.org Developers
On Wed, 30 Mar 2022 at 19:11, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
>
> Presumably you mean "revert" here? That might be the right way to go,
> just to avoid breaking users (even if we fix the kernel bug, it'll take
> a while to get everyone to update). That said, this smells like the
> sort of thing that's going to crop up at arbitrary times in dynamic
> systems so while a revert looks like it'd work around the boot issue we
> might be making more headaches for folks down the road.
>
The opposite in fact, I did not suggest to revert it, but rather undo
the revert (as Alistair already removed it from the apply-next tree),
since my original patch fixes buggy behaviour that is blocking the
testing of some embedded software on QEMU.
Idan Horowitz
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
2022-03-30 17:10 ` Idan Horowitz
(?)
@ 2022-03-31 3:23 ` Alistair Francis
-1 siblings, 0 replies; 32+ messages in thread
From: Alistair Francis @ 2022-03-31 3:23 UTC (permalink / raw)
To: Idan Horowitz
Cc: Palmer Dabbelt, linux-riscv, open list:RISC-V, phantom,
Alistair Francis, qemu-devel@nongnu.org Developers
On Thu, Mar 31, 2022 at 3:11 AM Idan Horowitz <idan.horowitz@gmail.com> wrote:
>
> On Wed, 30 Mar 2022 at 19:11, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >
> >
> > Presumably you mean "revert" here? That might be the right way to go,
> > just to avoid breaking users (even if we fix the kernel bug, it'll take
> > a while to get everyone to update). That said, this smells like the
> > sort of thing that's going to crop up at arbitrary times in dynamic
> > systems so while a revert looks like it'd work around the boot issue we
> > might be making more headaches for folks down the road.
> >
>
> The opposite in fact, I did not suggest to revert it, but rather undo
> the revert (as Alistair already removed it from the apply-next tree),
> since my original patch fixes buggy behaviour that is blocking the
> testing of some embedded software on QEMU.
So, this is a little tricky.
We want to apply the fix, but that will break current users.
Once the fix is merged into Linux we can apply it here. That should
hopefully be right at the start of the 7.1 QEMU development window,
which should give time for the fix to propagate into stable kernels
and not break too many people by the time QEMU is released.
Alistair
>
> Idan Horowitz
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
@ 2022-03-31 3:23 ` Alistair Francis
0 siblings, 0 replies; 32+ messages in thread
From: Alistair Francis @ 2022-03-31 3:23 UTC (permalink / raw)
To: Idan Horowitz
Cc: Palmer Dabbelt, linux-riscv, open list:RISC-V, phantom,
Alistair Francis, qemu-devel@nongnu.org Developers
On Thu, Mar 31, 2022 at 3:11 AM Idan Horowitz <idan.horowitz@gmail.com> wrote:
>
> On Wed, 30 Mar 2022 at 19:11, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >
> >
> > Presumably you mean "revert" here? That might be the right way to go,
> > just to avoid breaking users (even if we fix the kernel bug, it'll take
> > a while to get everyone to update). That said, this smells like the
> > sort of thing that's going to crop up at arbitrary times in dynamic
> > systems so while a revert looks like it'd work around the boot issue we
> > might be making more headaches for folks down the road.
> >
>
> The opposite in fact, I did not suggest to revert it, but rather undo
> the revert (as Alistair already removed it from the apply-next tree),
> since my original patch fixes buggy behaviour that is blocking the
> testing of some embedded software on QEMU.
So, this is a little tricky.
We want to apply the fix, but that will break current users.
Once the fix is merged into Linux we can apply it here. That should
hopefully be right at the start of the 7.1 QEMU development window,
which should give time for the fix to propagate into stable kernels
and not break too many people by the time QEMU is released.
Alistair
>
> Idan Horowitz
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
@ 2022-03-31 3:23 ` Alistair Francis
0 siblings, 0 replies; 32+ messages in thread
From: Alistair Francis @ 2022-03-31 3:23 UTC (permalink / raw)
To: Idan Horowitz
Cc: phantom, open list:RISC-V, qemu-devel@nongnu.org Developers,
Palmer Dabbelt, Alistair Francis, linux-riscv
On Thu, Mar 31, 2022 at 3:11 AM Idan Horowitz <idan.horowitz@gmail.com> wrote:
>
> On Wed, 30 Mar 2022 at 19:11, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >
> >
> > Presumably you mean "revert" here? That might be the right way to go,
> > just to avoid breaking users (even if we fix the kernel bug, it'll take
> > a while to get everyone to update). That said, this smells like the
> > sort of thing that's going to crop up at arbitrary times in dynamic
> > systems so while a revert looks like it'd work around the boot issue we
> > might be making more headaches for folks down the road.
> >
>
> The opposite in fact, I did not suggest to revert it, but rather undo
> the revert (as Alistair already removed it from the apply-next tree),
> since my original patch fixes buggy behaviour that is blocking the
> testing of some embedded software on QEMU.
So, this is a little tricky.
We want to apply the fix, but that will break current users.
Once the fix is merged into Linux we can apply it here. That should
hopefully be right at the start of the 7.1 QEMU development window,
which should give time for the fix to propagate into stable kernels
and not break too many people by the time QEMU is released.
Alistair
>
> Idan Horowitz
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
2022-03-31 3:23 ` Alistair Francis
(?)
@ 2022-03-31 4:36 ` Palmer Dabbelt
-1 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2022-03-31 4:36 UTC (permalink / raw)
To: alistair23
Cc: idan.horowitz, linux-riscv, qemu-riscv, phantom,
Alistair Francis, qemu-devel
On Wed, 30 Mar 2022 20:23:21 PDT (-0700), alistair23@gmail.com wrote:
> On Thu, Mar 31, 2022 at 3:11 AM Idan Horowitz <idan.horowitz@gmail.com> wrote:
>>
>> On Wed, 30 Mar 2022 at 19:11, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> >
>> >
>> > Presumably you mean "revert" here? That might be the right way to go,
>> > just to avoid breaking users (even if we fix the kernel bug, it'll take
>> > a while to get everyone to update). That said, this smells like the
>> > sort of thing that's going to crop up at arbitrary times in dynamic
>> > systems so while a revert looks like it'd work around the boot issue we
>> > might be making more headaches for folks down the road.
>> >
>>
>> The opposite in fact, I did not suggest to revert it, but rather undo
>> the revert (as Alistair already removed it from the apply-next tree),
>> since my original patch fixes buggy behaviour that is blocking the
>> testing of some embedded software on QEMU.
Ah, sorry -- the QEMU tree I was looking at still had the patch in
there, must have just been an old one.
> So, this is a little tricky.
>
> We want to apply the fix, but that will break current users.
>
> Once the fix is merged into Linux we can apply it here. That should
> hopefully be right at the start of the 7.1 QEMU development window,
> which should give time for the fix to propagate into stable kernels
> and not break too many people by the time QEMU is released.
If you think this is a Linux bug then that makes sense, but I think this
is a QEMU bug -- I sent a patch, not sure if it went through as it didn't
make it to lore.
I also think the bug will manifest without the TB exit patch, maybe in
single-step mode and definately if we happen to exit the TB at that
point for other reasons. Assuming my reasoning is correct in that
patch, we may also be hitting this as arbitrary corruption anywhere.
I'd started to write up a "QEMU errata" Linux patch for this, but then
convinced myself that just adding the sfence.vma was insufficient.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
@ 2022-03-31 4:36 ` Palmer Dabbelt
0 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2022-03-31 4:36 UTC (permalink / raw)
To: alistair23
Cc: idan.horowitz, linux-riscv, qemu-riscv, phantom,
Alistair Francis, qemu-devel
On Wed, 30 Mar 2022 20:23:21 PDT (-0700), alistair23@gmail.com wrote:
> On Thu, Mar 31, 2022 at 3:11 AM Idan Horowitz <idan.horowitz@gmail.com> wrote:
>>
>> On Wed, 30 Mar 2022 at 19:11, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> >
>> >
>> > Presumably you mean "revert" here? That might be the right way to go,
>> > just to avoid breaking users (even if we fix the kernel bug, it'll take
>> > a while to get everyone to update). That said, this smells like the
>> > sort of thing that's going to crop up at arbitrary times in dynamic
>> > systems so while a revert looks like it'd work around the boot issue we
>> > might be making more headaches for folks down the road.
>> >
>>
>> The opposite in fact, I did not suggest to revert it, but rather undo
>> the revert (as Alistair already removed it from the apply-next tree),
>> since my original patch fixes buggy behaviour that is blocking the
>> testing of some embedded software on QEMU.
Ah, sorry -- the QEMU tree I was looking at still had the patch in
there, must have just been an old one.
> So, this is a little tricky.
>
> We want to apply the fix, but that will break current users.
>
> Once the fix is merged into Linux we can apply it here. That should
> hopefully be right at the start of the 7.1 QEMU development window,
> which should give time for the fix to propagate into stable kernels
> and not break too many people by the time QEMU is released.
If you think this is a Linux bug then that makes sense, but I think this
is a QEMU bug -- I sent a patch, not sure if it went through as it didn't
make it to lore.
I also think the bug will manifest without the TB exit patch, maybe in
single-step mode and definately if we happen to exit the TB at that
point for other reasons. Assuming my reasoning is correct in that
patch, we may also be hitting this as arbitrary corruption anywhere.
I'd started to write up a "QEMU errata" Linux patch for this, but then
convinced myself that just adding the sfence.vma was insufficient.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
@ 2022-03-31 4:36 ` Palmer Dabbelt
0 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2022-03-31 4:36 UTC (permalink / raw)
To: alistair23
Cc: phantom, qemu-riscv, qemu-devel, Alistair Francis, linux-riscv,
idan.horowitz
On Wed, 30 Mar 2022 20:23:21 PDT (-0700), alistair23@gmail.com wrote:
> On Thu, Mar 31, 2022 at 3:11 AM Idan Horowitz <idan.horowitz@gmail.com> wrote:
>>
>> On Wed, 30 Mar 2022 at 19:11, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> >
>> >
>> > Presumably you mean "revert" here? That might be the right way to go,
>> > just to avoid breaking users (even if we fix the kernel bug, it'll take
>> > a while to get everyone to update). That said, this smells like the
>> > sort of thing that's going to crop up at arbitrary times in dynamic
>> > systems so while a revert looks like it'd work around the boot issue we
>> > might be making more headaches for folks down the road.
>> >
>>
>> The opposite in fact, I did not suggest to revert it, but rather undo
>> the revert (as Alistair already removed it from the apply-next tree),
>> since my original patch fixes buggy behaviour that is blocking the
>> testing of some embedded software on QEMU.
Ah, sorry -- the QEMU tree I was looking at still had the patch in
there, must have just been an old one.
> So, this is a little tricky.
>
> We want to apply the fix, but that will break current users.
>
> Once the fix is merged into Linux we can apply it here. That should
> hopefully be right at the start of the 7.1 QEMU development window,
> which should give time for the fix to propagate into stable kernels
> and not break too many people by the time QEMU is released.
If you think this is a Linux bug then that makes sense, but I think this
is a QEMU bug -- I sent a patch, not sure if it went through as it didn't
make it to lore.
I also think the bug will manifest without the TB exit patch, maybe in
single-step mode and definately if we happen to exit the TB at that
point for other reasons. Assuming my reasoning is correct in that
patch, we may also be hitting this as arbitrary corruption anywhere.
I'd started to write up a "QEMU errata" Linux patch for this, but then
convinced myself that just adding the sfence.vma was insufficient.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
2022-03-31 4:36 ` Palmer Dabbelt
(?)
@ 2022-03-31 5:13 ` Alistair Francis
-1 siblings, 0 replies; 32+ messages in thread
From: Alistair Francis @ 2022-03-31 5:13 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: Idan Horowitz, linux-riscv, open list:RISC-V, phantom,
Alistair Francis, qemu-devel@nongnu.org Developers
On Thu, Mar 31, 2022 at 2:36 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Wed, 30 Mar 2022 20:23:21 PDT (-0700), alistair23@gmail.com wrote:
> > On Thu, Mar 31, 2022 at 3:11 AM Idan Horowitz <idan.horowitz@gmail.com> wrote:
> >>
> >> On Wed, 30 Mar 2022 at 19:11, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >> >
> >> >
> >> > Presumably you mean "revert" here? That might be the right way to go,
> >> > just to avoid breaking users (even if we fix the kernel bug, it'll take
> >> > a while to get everyone to update). That said, this smells like the
> >> > sort of thing that's going to crop up at arbitrary times in dynamic
> >> > systems so while a revert looks like it'd work around the boot issue we
> >> > might be making more headaches for folks down the road.
> >> >
> >>
> >> The opposite in fact, I did not suggest to revert it, but rather undo
> >> the revert (as Alistair already removed it from the apply-next tree),
> >> since my original patch fixes buggy behaviour that is blocking the
> >> testing of some embedded software on QEMU.
>
> Ah, sorry -- the QEMU tree I was looking at still had the patch in
> there, must have just been an old one.
>
> > So, this is a little tricky.
> >
> > We want to apply the fix, but that will break current users.
> >
> > Once the fix is merged into Linux we can apply it here. That should
> > hopefully be right at the start of the 7.1 QEMU development window,
> > which should give time for the fix to propagate into stable kernels
> > and not break too many people by the time QEMU is released.
>
> If you think this is a Linux bug then that makes sense, but I think this
> is a QEMU bug -- I sent a patch, not sure if it went through as it didn't
> make it to lore.
Ah whoops. I saw the patch but didn't read it, then I assumed it was a
Linux bug from your diff earlier.
>
> I also think the bug will manifest without the TB exit patch, maybe in
> single-step mode and definately if we happen to exit the TB at that
> point for other reasons. Assuming my reasoning is correct in that
> patch, we may also be hitting this as arbitrary corruption anywhere.
> I'd started to write up a "QEMU errata" Linux patch for this, but then
> convinced myself that just adding the sfence.vma was insufficient.
Yeah, looking at it now I agree, I'll send a PR for 7.0.
Alistair
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
@ 2022-03-31 5:13 ` Alistair Francis
0 siblings, 0 replies; 32+ messages in thread
From: Alistair Francis @ 2022-03-31 5:13 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: Idan Horowitz, linux-riscv, open list:RISC-V, phantom,
Alistair Francis, qemu-devel@nongnu.org Developers
On Thu, Mar 31, 2022 at 2:36 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Wed, 30 Mar 2022 20:23:21 PDT (-0700), alistair23@gmail.com wrote:
> > On Thu, Mar 31, 2022 at 3:11 AM Idan Horowitz <idan.horowitz@gmail.com> wrote:
> >>
> >> On Wed, 30 Mar 2022 at 19:11, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >> >
> >> >
> >> > Presumably you mean "revert" here? That might be the right way to go,
> >> > just to avoid breaking users (even if we fix the kernel bug, it'll take
> >> > a while to get everyone to update). That said, this smells like the
> >> > sort of thing that's going to crop up at arbitrary times in dynamic
> >> > systems so while a revert looks like it'd work around the boot issue we
> >> > might be making more headaches for folks down the road.
> >> >
> >>
> >> The opposite in fact, I did not suggest to revert it, but rather undo
> >> the revert (as Alistair already removed it from the apply-next tree),
> >> since my original patch fixes buggy behaviour that is blocking the
> >> testing of some embedded software on QEMU.
>
> Ah, sorry -- the QEMU tree I was looking at still had the patch in
> there, must have just been an old one.
>
> > So, this is a little tricky.
> >
> > We want to apply the fix, but that will break current users.
> >
> > Once the fix is merged into Linux we can apply it here. That should
> > hopefully be right at the start of the 7.1 QEMU development window,
> > which should give time for the fix to propagate into stable kernels
> > and not break too many people by the time QEMU is released.
>
> If you think this is a Linux bug then that makes sense, but I think this
> is a QEMU bug -- I sent a patch, not sure if it went through as it didn't
> make it to lore.
Ah whoops. I saw the patch but didn't read it, then I assumed it was a
Linux bug from your diff earlier.
>
> I also think the bug will manifest without the TB exit patch, maybe in
> single-step mode and definately if we happen to exit the TB at that
> point for other reasons. Assuming my reasoning is correct in that
> patch, we may also be hitting this as arbitrary corruption anywhere.
> I'd started to write up a "QEMU errata" Linux patch for this, but then
> convinced myself that just adding the sfence.vma was insufficient.
Yeah, looking at it now I agree, I'll send a PR for 7.0.
Alistair
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
@ 2022-03-31 5:13 ` Alistair Francis
0 siblings, 0 replies; 32+ messages in thread
From: Alistair Francis @ 2022-03-31 5:13 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: phantom, open list:RISC-V, qemu-devel@nongnu.org Developers,
Alistair Francis, linux-riscv, Idan Horowitz
On Thu, Mar 31, 2022 at 2:36 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Wed, 30 Mar 2022 20:23:21 PDT (-0700), alistair23@gmail.com wrote:
> > On Thu, Mar 31, 2022 at 3:11 AM Idan Horowitz <idan.horowitz@gmail.com> wrote:
> >>
> >> On Wed, 30 Mar 2022 at 19:11, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >> >
> >> >
> >> > Presumably you mean "revert" here? That might be the right way to go,
> >> > just to avoid breaking users (even if we fix the kernel bug, it'll take
> >> > a while to get everyone to update). That said, this smells like the
> >> > sort of thing that's going to crop up at arbitrary times in dynamic
> >> > systems so while a revert looks like it'd work around the boot issue we
> >> > might be making more headaches for folks down the road.
> >> >
> >>
> >> The opposite in fact, I did not suggest to revert it, but rather undo
> >> the revert (as Alistair already removed it from the apply-next tree),
> >> since my original patch fixes buggy behaviour that is blocking the
> >> testing of some embedded software on QEMU.
>
> Ah, sorry -- the QEMU tree I was looking at still had the patch in
> there, must have just been an old one.
>
> > So, this is a little tricky.
> >
> > We want to apply the fix, but that will break current users.
> >
> > Once the fix is merged into Linux we can apply it here. That should
> > hopefully be right at the start of the 7.1 QEMU development window,
> > which should give time for the fix to propagate into stable kernels
> > and not break too many people by the time QEMU is released.
>
> If you think this is a Linux bug then that makes sense, but I think this
> is a QEMU bug -- I sent a patch, not sure if it went through as it didn't
> make it to lore.
Ah whoops. I saw the patch but didn't read it, then I assumed it was a
Linux bug from your diff earlier.
>
> I also think the bug will manifest without the TB exit patch, maybe in
> single-step mode and definately if we happen to exit the TB at that
> point for other reasons. Assuming my reasoning is correct in that
> patch, we may also be hitting this as arbitrary corruption anywhere.
> I'd started to write up a "QEMU errata" Linux patch for this, but then
> convinced myself that just adding the sfence.vma was insufficient.
Yeah, looking at it now I agree, I'll send a PR for 7.0.
Alistair
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
2022-03-31 5:13 ` Alistair Francis
(?)
@ 2022-03-31 19:54 ` Palmer Dabbelt
-1 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2022-03-31 19:54 UTC (permalink / raw)
To: alistair23
Cc: idan.horowitz, linux-riscv, qemu-riscv, phantom,
Alistair Francis, qemu-devel
On Wed, 30 Mar 2022 22:13:39 PDT (-0700), alistair23@gmail.com wrote:
> On Thu, Mar 31, 2022 at 2:36 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>> On Wed, 30 Mar 2022 20:23:21 PDT (-0700), alistair23@gmail.com wrote:
>> > On Thu, Mar 31, 2022 at 3:11 AM Idan Horowitz <idan.horowitz@gmail.com> wrote:
>> >>
>> >> On Wed, 30 Mar 2022 at 19:11, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> >> >
>> >> >
>> >> > Presumably you mean "revert" here? That might be the right way to go,
>> >> > just to avoid breaking users (even if we fix the kernel bug, it'll take
>> >> > a while to get everyone to update). That said, this smells like the
>> >> > sort of thing that's going to crop up at arbitrary times in dynamic
>> >> > systems so while a revert looks like it'd work around the boot issue we
>> >> > might be making more headaches for folks down the road.
>> >> >
>> >>
>> >> The opposite in fact, I did not suggest to revert it, but rather undo
>> >> the revert (as Alistair already removed it from the apply-next tree),
>> >> since my original patch fixes buggy behaviour that is blocking the
>> >> testing of some embedded software on QEMU.
>>
>> Ah, sorry -- the QEMU tree I was looking at still had the patch in
>> there, must have just been an old one.
>>
>> > So, this is a little tricky.
>> >
>> > We want to apply the fix, but that will break current users.
>> >
>> > Once the fix is merged into Linux we can apply it here. That should
>> > hopefully be right at the start of the 7.1 QEMU development window,
>> > which should give time for the fix to propagate into stable kernels
>> > and not break too many people by the time QEMU is released.
>>
>> If you think this is a Linux bug then that makes sense, but I think this
>> is a QEMU bug -- I sent a patch, not sure if it went through as it didn't
>> make it to lore.
>
> Ah whoops. I saw the patch but didn't read it, then I assumed it was a
> Linux bug from your diff earlier.
No problem, that was the first thing I sent in the morning so I doubt it
made any sense.
>
>>
>> I also think the bug will manifest without the TB exit patch, maybe in
>> single-step mode and definately if we happen to exit the TB at that
>> point for other reasons. Assuming my reasoning is correct in that
>> patch, we may also be hitting this as arbitrary corruption anywhere.
>> I'd started to write up a "QEMU errata" Linux patch for this, but then
>> convinced myself that just adding the sfence.vma was insufficient.
>
> Yeah, looking at it now I agree, I'll send a PR for 7.0.
Thanks!
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
@ 2022-03-31 19:54 ` Palmer Dabbelt
0 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2022-03-31 19:54 UTC (permalink / raw)
To: alistair23
Cc: idan.horowitz, linux-riscv, qemu-riscv, phantom,
Alistair Francis, qemu-devel
On Wed, 30 Mar 2022 22:13:39 PDT (-0700), alistair23@gmail.com wrote:
> On Thu, Mar 31, 2022 at 2:36 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>> On Wed, 30 Mar 2022 20:23:21 PDT (-0700), alistair23@gmail.com wrote:
>> > On Thu, Mar 31, 2022 at 3:11 AM Idan Horowitz <idan.horowitz@gmail.com> wrote:
>> >>
>> >> On Wed, 30 Mar 2022 at 19:11, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> >> >
>> >> >
>> >> > Presumably you mean "revert" here? That might be the right way to go,
>> >> > just to avoid breaking users (even if we fix the kernel bug, it'll take
>> >> > a while to get everyone to update). That said, this smells like the
>> >> > sort of thing that's going to crop up at arbitrary times in dynamic
>> >> > systems so while a revert looks like it'd work around the boot issue we
>> >> > might be making more headaches for folks down the road.
>> >> >
>> >>
>> >> The opposite in fact, I did not suggest to revert it, but rather undo
>> >> the revert (as Alistair already removed it from the apply-next tree),
>> >> since my original patch fixes buggy behaviour that is blocking the
>> >> testing of some embedded software on QEMU.
>>
>> Ah, sorry -- the QEMU tree I was looking at still had the patch in
>> there, must have just been an old one.
>>
>> > So, this is a little tricky.
>> >
>> > We want to apply the fix, but that will break current users.
>> >
>> > Once the fix is merged into Linux we can apply it here. That should
>> > hopefully be right at the start of the 7.1 QEMU development window,
>> > which should give time for the fix to propagate into stable kernels
>> > and not break too many people by the time QEMU is released.
>>
>> If you think this is a Linux bug then that makes sense, but I think this
>> is a QEMU bug -- I sent a patch, not sure if it went through as it didn't
>> make it to lore.
>
> Ah whoops. I saw the patch but didn't read it, then I assumed it was a
> Linux bug from your diff earlier.
No problem, that was the first thing I sent in the morning so I doubt it
made any sense.
>
>>
>> I also think the bug will manifest without the TB exit patch, maybe in
>> single-step mode and definately if we happen to exit the TB at that
>> point for other reasons. Assuming my reasoning is correct in that
>> patch, we may also be hitting this as arbitrary corruption anywhere.
>> I'd started to write up a "QEMU errata" Linux patch for this, but then
>> convinced myself that just adding the sfence.vma was insufficient.
>
> Yeah, looking at it now I agree, I'll send a PR for 7.0.
Thanks!
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: Re: [PATCH] target/riscv: Exit current TB after an sfence.vma
@ 2022-03-31 19:54 ` Palmer Dabbelt
0 siblings, 0 replies; 32+ messages in thread
From: Palmer Dabbelt @ 2022-03-31 19:54 UTC (permalink / raw)
To: alistair23
Cc: phantom, qemu-riscv, qemu-devel, Alistair Francis, linux-riscv,
idan.horowitz
On Wed, 30 Mar 2022 22:13:39 PDT (-0700), alistair23@gmail.com wrote:
> On Thu, Mar 31, 2022 at 2:36 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>> On Wed, 30 Mar 2022 20:23:21 PDT (-0700), alistair23@gmail.com wrote:
>> > On Thu, Mar 31, 2022 at 3:11 AM Idan Horowitz <idan.horowitz@gmail.com> wrote:
>> >>
>> >> On Wed, 30 Mar 2022 at 19:11, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>> >> >
>> >> >
>> >> > Presumably you mean "revert" here? That might be the right way to go,
>> >> > just to avoid breaking users (even if we fix the kernel bug, it'll take
>> >> > a while to get everyone to update). That said, this smells like the
>> >> > sort of thing that's going to crop up at arbitrary times in dynamic
>> >> > systems so while a revert looks like it'd work around the boot issue we
>> >> > might be making more headaches for folks down the road.
>> >> >
>> >>
>> >> The opposite in fact, I did not suggest to revert it, but rather undo
>> >> the revert (as Alistair already removed it from the apply-next tree),
>> >> since my original patch fixes buggy behaviour that is blocking the
>> >> testing of some embedded software on QEMU.
>>
>> Ah, sorry -- the QEMU tree I was looking at still had the patch in
>> there, must have just been an old one.
>>
>> > So, this is a little tricky.
>> >
>> > We want to apply the fix, but that will break current users.
>> >
>> > Once the fix is merged into Linux we can apply it here. That should
>> > hopefully be right at the start of the 7.1 QEMU development window,
>> > which should give time for the fix to propagate into stable kernels
>> > and not break too many people by the time QEMU is released.
>>
>> If you think this is a Linux bug then that makes sense, but I think this
>> is a QEMU bug -- I sent a patch, not sure if it went through as it didn't
>> make it to lore.
>
> Ah whoops. I saw the patch but didn't read it, then I assumed it was a
> Linux bug from your diff earlier.
No problem, that was the first thing I sent in the morning so I doubt it
made any sense.
>
>>
>> I also think the bug will manifest without the TB exit patch, maybe in
>> single-step mode and definately if we happen to exit the TB at that
>> point for other reasons. Assuming my reasoning is correct in that
>> patch, we may also be hitting this as arbitrary corruption anywhere.
>> I'd started to write up a "QEMU errata" Linux patch for this, but then
>> convinced myself that just adding the sfence.vma was insufficient.
>
> Yeah, looking at it now I agree, I'll send a PR for 7.0.
Thanks!
^ permalink raw reply [flat|nested] 32+ messages in thread