[PATCH] target/riscv: PMP violation due to wrong size parameter
riscv_cpu_tlb_fill() uses the `size` parameter to check PMP violation
using pmp_hart_has_privs().
However, if the size is unknown (=0), the ending address will be
`addr - 1` as it is `addr + size - 1` in `pmp_hart_has_privs()`.
This always causes a false PMP violation on the starting address of the
range, as `addr - 1` is not in the range.
In order to fix, we just assume that all bytes from addr to the end of
the page will be accessed if the size is unknown.
Signed-off-by: Dayeol Lee
Reviewed-by: Richard Henderson
---
target/riscv/pmp.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 958c7502a0..7a9fd415ba 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -232,6 +232,7 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong
addr,
{
int i = 0;
int ret = -1;
+int pmp_size = 0;
target_ulong s = 0;
target_ulong e = 0;
pmp_priv_t allowed_privs = 0;
@@ -241,11 +242,21 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong
addr,
return true;
}
+/*
+ * if size is unknown (0), assume that all bytes
+ * from addr to the end of the page will be accessed.
+ */
+if (size == 0) {
+pmp_size = -(addr | TARGET_PAGE_MASK);
+} else {
+pmp_size = size;
+}
+
/* 1.10 draft priv spec states there is an implicit order
from low to high */
for (i = 0; i < MAX_RISCV_PMPS; i++) {
s = pmp_is_in_range(env, i, addr);
-e = pmp_is_in_range(env, i, addr + size - 1);
+e = pmp_is_in_range(env, i, addr + pmp_size - 1);
/* partially inside */
if ((s + e) == 1) {
--
2.23.0
Re: [PATCH] target/riscv: PMP violation due to wrong size parameter
I'll move the entire check into pmp_hart_has_privs as it makes more sense.
Thanks!
On Fri, Oct 18, 2019, 3:01 PM Palmer Dabbelt wrote:
> On Tue, 15 Oct 2019 10:04:32 PDT (-0700), day...@berkeley.edu wrote:
> > Hi,
> >
> > Could this patch go through?
> > If not please let me know so that I can fix.
> > Thank you!
>
> Sorry, I dropped this one. It's in the patch queue now. We should also
> check
> for size==0 in pmp_hart_has_privs(), as that won't work. LMK if you want
> to
> send a patch for that.
>
> >
> > Dayeol
> >
> >
> > On Sat, Oct 12, 2019, 11:30 AM Dayeol Lee wrote:
> >
> >> No it doesn't mean that.
> >> But the following code will make the size TARGET_PAGE_SIZE - (page
> offset)
> >> if the address is not aligned.
> >>
> >> pmp_size = -(address | TARGET_PAGE_MASK)
> >>
> >>
> >> On Fri, Oct 11, 2019, 7:37 PM Jonathan Behrens
> wrote:
> >>
> >>> How do you know that the access won't straddle a page boundary? Is
> there
> >>> a guarantee somewhere that size=0 means that the access is naturally
> >>> aligned?
> >>>
> >>> Jonathan
> >>>
> >>>
> >>> On Fri, Oct 11, 2019 at 7:14 PM Dayeol Lee
> wrote:
> >>>
> >>>> riscv_cpu_tlb_fill() uses the `size` parameter to check PMP violation
> >>>> using pmp_hart_has_privs().
> >>>> However, if the size is unknown (=0), the ending address will be
> >>>> `addr - 1` as it is `addr + size - 1` in `pmp_hart_has_privs()`.
> >>>> This always causes a false PMP violation on the starting address of
> the
> >>>> range, as `addr - 1` is not in the range.
> >>>>
> >>>> In order to fix, we just assume that all bytes from addr to the end of
> >>>> the page will be accessed if the size is unknown.
> >>>>
> >>>> Signed-off-by: Dayeol Lee
> >>>> Reviewed-by: Richard Henderson
> >>>> ---
> >>>> target/riscv/cpu_helper.c | 13 -
> >>>> 1 file changed, 12 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> >>>> index e32b6126af..7d9a22b601 100644
> >>>> --- a/target/riscv/cpu_helper.c
> >>>> +++ b/target/riscv/cpu_helper.c
> >>>> @@ -441,6 +441,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr
> address,
> >>>> int size,
> >>>> CPURISCVState *env = >env;
> >>>> hwaddr pa = 0;
> >>>> int prot;
> >>>> +int pmp_size = 0;
> >>>> bool pmp_violation = false;
> >>>> int ret = TRANSLATE_FAIL;
> >>>> int mode = mmu_idx;
> >>>> @@ -460,9 +461,19 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr
> >>>> address, int size,
> >>>>"%s address=%" VADDR_PRIx " ret %d physical "
> >>>> TARGET_FMT_plx
> >>>>" prot %d\n", __func__, address, ret, pa, prot);
> >>>>
> >>>> +/*
> >>>> + * if size is unknown (0), assume that all bytes
> >>>> + * from addr to the end of the page will be accessed.
> >>>> + */
> >>>> +if (size == 0) {
> >>>> +pmp_size = -(address | TARGET_PAGE_MASK);
> >>>> +} else {
> >>>> +pmp_size = size;
> >>>> +}
> >>>> +
> >>>> if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> >>>> (ret == TRANSLATE_SUCCESS) &&
> >>>> -!pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
> >>>> +!pmp_hart_has_privs(env, pa, pmp_size, 1 << access_type,
> mode))
> >>>> {
> >>>> ret = TRANSLATE_PMP_FAIL;
> >>>> }
> >>>> if (ret == TRANSLATE_PMP_FAIL) {
> >>>> --
> >>>> 2.20.1
> >>>>
> >>>>
> >>>>
>
Re: [PATCH] target/riscv: PMP violation due to wrong size parameter
Hi,
Could this patch go through?
If not please let me know so that I can fix.
Thank you!
Dayeol
On Sat, Oct 12, 2019, 11:30 AM Dayeol Lee wrote:
> No it doesn't mean that.
> But the following code will make the size TARGET_PAGE_SIZE - (page offset)
> if the address is not aligned.
>
> pmp_size = -(address | TARGET_PAGE_MASK)
>
>
> On Fri, Oct 11, 2019, 7:37 PM Jonathan Behrens wrote:
>
>> How do you know that the access won't straddle a page boundary? Is there
>> a guarantee somewhere that size=0 means that the access is naturally
>> aligned?
>>
>> Jonathan
>>
>>
>> On Fri, Oct 11, 2019 at 7:14 PM Dayeol Lee wrote:
>>
>>> riscv_cpu_tlb_fill() uses the `size` parameter to check PMP violation
>>> using pmp_hart_has_privs().
>>> However, if the size is unknown (=0), the ending address will be
>>> `addr - 1` as it is `addr + size - 1` in `pmp_hart_has_privs()`.
>>> This always causes a false PMP violation on the starting address of the
>>> range, as `addr - 1` is not in the range.
>>>
>>> In order to fix, we just assume that all bytes from addr to the end of
>>> the page will be accessed if the size is unknown.
>>>
>>> Signed-off-by: Dayeol Lee
>>> Reviewed-by: Richard Henderson
>>> ---
>>> target/riscv/cpu_helper.c | 13 -
>>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>> index e32b6126af..7d9a22b601 100644
>>> --- a/target/riscv/cpu_helper.c
>>> +++ b/target/riscv/cpu_helper.c
>>> @@ -441,6 +441,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address,
>>> int size,
>>> CPURISCVState *env = >env;
>>> hwaddr pa = 0;
>>> int prot;
>>> +int pmp_size = 0;
>>> bool pmp_violation = false;
>>> int ret = TRANSLATE_FAIL;
>>> int mode = mmu_idx;
>>> @@ -460,9 +461,19 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr
>>> address, int size,
>>>"%s address=%" VADDR_PRIx " ret %d physical "
>>> TARGET_FMT_plx
>>>" prot %d\n", __func__, address, ret, pa, prot);
>>>
>>> +/*
>>> + * if size is unknown (0), assume that all bytes
>>> + * from addr to the end of the page will be accessed.
>>> + */
>>> +if (size == 0) {
>>> +pmp_size = -(address | TARGET_PAGE_MASK);
>>> +} else {
>>> +pmp_size = size;
>>> +}
>>> +
>>> if (riscv_feature(env, RISCV_FEATURE_PMP) &&
>>> (ret == TRANSLATE_SUCCESS) &&
>>> -!pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
>>> +!pmp_hart_has_privs(env, pa, pmp_size, 1 << access_type, mode))
>>> {
>>> ret = TRANSLATE_PMP_FAIL;
>>> }
>>> if (ret == TRANSLATE_PMP_FAIL) {
>>> --
>>> 2.20.1
>>>
>>>
>>>
Re: [PATCH] target/riscv: PMP violation due to wrong size parameter
No it doesn't mean that.
But the following code will make the size TARGET_PAGE_SIZE - (page offset)
if the address is not aligned.
pmp_size = -(address | TARGET_PAGE_MASK)
On Fri, Oct 11, 2019, 7:37 PM Jonathan Behrens wrote:
> How do you know that the access won't straddle a page boundary? Is there a
> guarantee somewhere that size=0 means that the access is naturally aligned?
>
> Jonathan
>
>
> On Fri, Oct 11, 2019 at 7:14 PM Dayeol Lee wrote:
>
>> riscv_cpu_tlb_fill() uses the `size` parameter to check PMP violation
>> using pmp_hart_has_privs().
>> However, if the size is unknown (=0), the ending address will be
>> `addr - 1` as it is `addr + size - 1` in `pmp_hart_has_privs()`.
>> This always causes a false PMP violation on the starting address of the
>> range, as `addr - 1` is not in the range.
>>
>> In order to fix, we just assume that all bytes from addr to the end of
>> the page will be accessed if the size is unknown.
>>
>> Signed-off-by: Dayeol Lee
>> Reviewed-by: Richard Henderson
>> ---
>> target/riscv/cpu_helper.c | 13 -
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index e32b6126af..7d9a22b601 100644
>> --- a/target/riscv/cpu_helper.c
>> +++ b/target/riscv/cpu_helper.c
>> @@ -441,6 +441,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address,
>> int size,
>> CPURISCVState *env = >env;
>> hwaddr pa = 0;
>> int prot;
>> +int pmp_size = 0;
>> bool pmp_violation = false;
>> int ret = TRANSLATE_FAIL;
>> int mode = mmu_idx;
>> @@ -460,9 +461,19 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address,
>> int size,
>>"%s address=%" VADDR_PRIx " ret %d physical "
>> TARGET_FMT_plx
>>" prot %d\n", __func__, address, ret, pa, prot);
>>
>> +/*
>> + * if size is unknown (0), assume that all bytes
>> + * from addr to the end of the page will be accessed.
>> + */
>> +if (size == 0) {
>> +pmp_size = -(address | TARGET_PAGE_MASK);
>> +} else {
>> +pmp_size = size;
>> +}
>> +
>> if (riscv_feature(env, RISCV_FEATURE_PMP) &&
>> (ret == TRANSLATE_SUCCESS) &&
>> -!pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
>> +!pmp_hart_has_privs(env, pa, pmp_size, 1 << access_type, mode)) {
>> ret = TRANSLATE_PMP_FAIL;
>> }
>> if (ret == TRANSLATE_PMP_FAIL) {
>> --
>> 2.20.1
>>
>>
>>
Re: [PATCH v2 1/1] target/riscv/pmp: Fix bug preventing
Hi, Alistair,
Thank you for reminding me.
I already had the local patch, so I re-submitted the patch.
Please let me know if that's fair enough (or you have any other comments)
Thanks,
Dayeol
On Fri, Oct 11, 2019 at 3:24 PM Alistair Francis
wrote:
> On Sun, Oct 6, 2019 at 1:32 AM Chris Williams wrote:
> >
> > Hello. I hope you don't mind me resubmitting this patch. Please let me
> know if
>
> Not at all!
>
> Thanks for the patch.
>
> In future if people don't respond you can just keep pinging your patch.
>
> > I've formatted it incorrectly or if it needs more explanation. My
> previous
> > attempt probably wasn't formatted quite right. This is my first time
> > contributing to Qemu, so I'm keen to get it right - thanks.
>
> This whole paragraph should not be in the commit. It can go below the
> line though.
>
> Also please use `git format-patch` to format the patch and then `git
> send-email` to send the patch. There is a whole heap of detail here:
> https://wiki.qemu.org/Contribute/SubmitAPatch
>
> >
> > This fixes an issue that prevents a RISC-V CPU from executing
> instructions
> > immediately from the base address of a PMP TOR region.
> >
> > When jumping to an instruction in a PMP TOR region, pmp_hart_has_privs()
> is
> > called to validate the access. If this instruction is the very first
> word of a
> > PMP TOR region, at address 0 relative to the start address of the
> region, then
> > the access will fail. This is because pmp_hart_has_privs() is called
> with size
> > 0 to perform this validation, causing this check...
> >
> > e = pmp_is_in_range(env, i, addr + size - 1);
> >
> > ... to fail, as (addr + size - 1) falls below the base address of the PMP
> > region. Really, the access should succeed. For example, if I have a
> region
> > spanning 0x80d96000 to 0x88d95fff and the CPU jumps to 0x80d96000, then:
> >
> > s = 0x80d96000
> > e = 0x80d95fff
> >
> > And the validation fails. The size check proposed below catches these
> zero-size
> > instruction fetch access probes. The word alignment in pmpaddr{0-15} and
> > earlier instruction alignment checks should prevent the execution of
> > instructions over the upper boundary of the PMP region, though I'm happy
> to give
> > this more attention if this is a concern.
>
> This seems like a similar issue to this patch as well:
>
> https://lore.kernel.org/qemu-devel/20191007052813.25814-1-day...@berkeley.edu/
>
> From that discussion:
>
> "In general, size 0 means "unknown size". In this case, the one tlb
> lookup is
> going to be used by lots of instructions -- everything that fits on the
> page."
>
> Richard's last comment seems like a better fix:
>
> "You certainly could do
>
> if (size == 0) {
> size = -(addr | TARGET_PAGE_MASK);
> }
>
> to assume that all bytes from addr to the end of the page are accessed.
> That
> would avoid changing too much of the rest of the logic.
>
> That said, this code will continue to not work for mis-aligned boundaries."
>
> So I don't think this is the correct solution. I'm not sure if Dayeol
> is planning on sending a follow up version. If not feel free to send
> it.
>
> >
> > Signed-off-by: Chris Williams diodes...@tuta.io>>
>
> It looks like this is a HTML patch, also ensure all patches are just
> plain text, `git send-email` will do this.
>
> Thanks for the patch though! Please send any more fixes that you find :)
>
> Alistair
>
> >
> > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> > index d4f1007109..9308672e20 100644
> > --- a/target/riscv/pmp.c
> > +++ b/target/riscv/pmp.c
> > @@ -235,8 +235,9 @@ bool pmp_hart_has_privs(CPURISCVState *env,
> target_ulong
> > addr,
> > /* 1.10 draft priv spec states there is an implicit order
> > from low to high */
> > for (i = 0; i < MAX_RISCV_PMPS; i++) {
> > +/* catch zero-size instruction checks */
> > s = pmp_is_in_range(env, i, addr);
> > -e = pmp_is_in_range(env, i, addr + size - 1);
> > +e = pmp_is_in_range(env, i, (size == 0) ? addr : addr + size -
> 1);
> >
> > /* partially inside */
> > if ((s + e) == 1) {
> >
> >
>
[PATCH] target/riscv: PMP violation due to wrong size parameter
riscv_cpu_tlb_fill() uses the `size` parameter to check PMP violation
using pmp_hart_has_privs().
However, if the size is unknown (=0), the ending address will be
`addr - 1` as it is `addr + size - 1` in `pmp_hart_has_privs()`.
This always causes a false PMP violation on the starting address of the
range, as `addr - 1` is not in the range.
In order to fix, we just assume that all bytes from addr to the end of
the page will be accessed if the size is unknown.
Signed-off-by: Dayeol Lee
Reviewed-by: Richard Henderson
---
target/riscv/cpu_helper.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index e32b6126af..7d9a22b601 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -441,6 +441,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
CPURISCVState *env = >env;
hwaddr pa = 0;
int prot;
+int pmp_size = 0;
bool pmp_violation = false;
int ret = TRANSLATE_FAIL;
int mode = mmu_idx;
@@ -460,9 +461,19 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
"%s address=%" VADDR_PRIx " ret %d physical " TARGET_FMT_plx
" prot %d\n", __func__, address, ret, pa, prot);
+/*
+ * if size is unknown (0), assume that all bytes
+ * from addr to the end of the page will be accessed.
+ */
+if (size == 0) {
+pmp_size = -(address | TARGET_PAGE_MASK);
+} else {
+pmp_size = size;
+}
+
if (riscv_feature(env, RISCV_FEATURE_PMP) &&
(ret == TRANSLATE_SUCCESS) &&
-!pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
+!pmp_hart_has_privs(env, pa, pmp_size, 1 << access_type, mode)) {
ret = TRANSLATE_PMP_FAIL;
}
if (ret == TRANSLATE_PMP_FAIL) {
--
2.20.1
Re: [PATCH] target/riscv: PMP violation due to wrong size parameter
On Mon, Oct 7, 2019 at 11:25 AM Richard Henderson < richard.hender...@linaro.org> wrote: > On 10/7/19 10:19 AM, Dayeol Lee wrote: > > Thank you very much for the clarification! > > > > I found tlb_set_page with size != TARGET_PAGE_SIZE makes the translation > way > > too slow; the Linux doesn't seem to boot. > > To clarify, PMP specifies a range. That range has only two end points. > Therefore, a maximum of 2 pages may be affected by a mis-aligned PMP > boundary. > > It sounds like you're getting size != TARGET_PAGE_SIZE for all pages. > > The cause of the problem is not a mis-aligned PMP boundary. Let's say a PMP range is 0x1000 - 0x2000 if pmp_hart_has_privs() gets addr=0x2000 and size=0, pmp_hart_has_privs() will ALWAYS return false because the code assumes size > 0. It checks if (addr) and (addr + size - 1) are within the PMP range for each PMP entry. (addr + size - 1) is supposed to be the last byte address of the memory access, but it ends up with (addr - 1) if size = 0. Thus, pmp_hart_has_privs() returns false as (addr - 1) = 0x1fff is within the range, and addr = 0x2000 is out of the range (partial match violation).
Re: [PATCH] target/riscv: PMP violation due to wrong size parameter
Thank you very much for the clarification! I found tlb_set_page with size != TARGET_PAGE_SIZE makes the translation way too slow; the Linux doesn't seem to boot. If that's the only way to reduce PMP granularity to less than TARGET_PAGE_SIZE, Can we just set the PMP default granularity to TARGET_PAGE_SIZE as it did before? OR Can we bypass the partial match violation when size is unknown? (check the starting address only) I think both of the options does not exactly match with the ISA specification, but given that size=0 always causes the problem, I want it to be fixed as soon as possible. Any thoughts would be appreciated! On Mon, Oct 7, 2019, 6:00 AM Richard Henderson wrote: > On 10/6/19 10:28 PM, Dayeol Lee wrote: > > riscv_cpu_tlb_fill() uses the `size` parameter to check PMP violation > > using pmp_hart_has_privs(). > > However, the size passed from tlb_fill(), which is called by > > get_page_addr_code(), is always a hard-coded value 0. > > This causes a false PMP violation if the instruction presents on a > > PMP boundary. > > > > In order to fix, simply correct the size to 4 if the access_type is > > MMU_INST_FETCH. > > That's not correct. > > In general, size 0 means "unknown size". In this case, the one tlb lookup > is > going to be used by lots of instructions -- everything that fits on the > page. > > If you want to support PMP on things that are not page boundaries, then you > will also have to call tlb_set_page with size != TARGET_PAGE_SIZE. > > Fixing that will cause instructions within that page to be executed one at > a > time, which also means they will be tlb_fill'd one at a time, which means > that > you'll get the correct size value. > > Which will be 2 or 4, depending on whether the configuration supports the > Compressed extension, and not just 4. > > > r~ > >
[PATCH] target/riscv: PMP violation due to wrong size parameter
riscv_cpu_tlb_fill() uses the `size` parameter to check PMP violation
using pmp_hart_has_privs().
However, the size passed from tlb_fill(), which is called by
get_page_addr_code(), is always a hard-coded value 0.
This causes a false PMP violation if the instruction presents on a
PMP boundary.
In order to fix, simply correct the size to 4 if the access_type is
MMU_INST_FETCH.
Signed-off-by: Dayeol Lee
---
target/riscv/cpu.h| 1 +
target/riscv/cpu_helper.c | 9 -
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 0adb307f32..386c80e764 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -88,6 +88,7 @@ enum {
#define MMU_USER_IDX 3
#define MAX_RISCV_PMPS (16)
+#define RISCV_INSN_LENGTH 4
typedef struct CPURISCVState CPURISCVState;
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index e32b6126af..877e89dbf2 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -441,6 +441,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
CPURISCVState *env = >env;
hwaddr pa = 0;
int prot;
+int pmp_size = 0;
bool pmp_violation = false;
int ret = TRANSLATE_FAIL;
int mode = mmu_idx;
@@ -460,9 +461,15 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int
size,
"%s address=%" VADDR_PRIx " ret %d physical " TARGET_FMT_plx
" prot %d\n", __func__, address, ret, pa, prot);
+if (access_type == MMU_INST_FETCH) {
+ pmp_size = RISCV_INSN_LENGTH;
+} else {
+ pmp_size = size;
+}
+
if (riscv_feature(env, RISCV_FEATURE_PMP) &&
(ret == TRANSLATE_SUCCESS) &&
-!pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
+!pmp_hart_has_privs(env, pa, pmp_size, 1 << access_type, mode)) {
ret = TRANSLATE_PMP_FAIL;
}
if (ret == TRANSLATE_PMP_FAIL) {
--
2.20.1
Re: [Qemu-devel] [PATCH] target/riscv/pmp.c: pmpcfg_csr_read returns bogus value on RV64
Hi,
I submitted the patch, but just found this has been already fixed by
Michael Clark
and pushed to riscv/riscv-qemu https://github.com/riscv/riscv-qemu/pull/166
but not in the upstream.
Do we still need this patch?
Thanks,
Dayeol
On Fri, Oct 26, 2018 at 11:04 AM Dayeol Lee wrote:
> pmp_read_cfg() returns 8-bit value, which is combined together to form a
> single pmpcfg CSR.
> The default promotion rules will result in an integer here ("i*8" is
> integer, which
> flows through) resulting in a 32-bit signed value on most hosts.
> That's bogus on RV64I, with the high bits of the CSR being wrong.
>
> Signed-off-by: Dayeol Lee
> Reviewed-by: Palmer Dabbelt
> ---
> target/riscv/pmp.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index c828950..3d3906a 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -330,7 +330,7 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env,
> uint32_t reg_index)
> {
> int i;
> target_ulong cfg_val = 0;
> -uint8_t val = 0;
> +target_ulong val = 0;
>
> if(sizeof(target_ulong) == 8)
> reg_index /= 2;
> --
> 2.7.4
>
>
[Qemu-devel] [PATCH] target/riscv/pmp.c: pmpcfg_csr_read returns bogus value on RV64
pmp_read_cfg() returns 8-bit value, which is combined together to form a single
pmpcfg CSR.
The default promotion rules will result in an integer here ("i*8" is integer,
which
flows through) resulting in a 32-bit signed value on most hosts.
That's bogus on RV64I, with the high bits of the CSR being wrong.
Signed-off-by: Dayeol Lee
Reviewed-by: Palmer Dabbelt
---
target/riscv/pmp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index c828950..3d3906a 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -330,7 +330,7 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t
reg_index)
{
int i;
target_ulong cfg_val = 0;
-uint8_t val = 0;
+target_ulong val = 0;
if(sizeof(target_ulong) == 8)
reg_index /= 2;
--
2.7.4
[Qemu-devel] [PATCH] [PATCH] target/riscv/pmp.c: pmpcfg_csr_read return type demotion
There is a data type demotion bug in target/riscv/pmp.c
When the target_ulong is 8 bytes, pmpcfg_csr_read returns only lower 4
bytes.
---
target/riscv/pmp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index c828950..4b6c20e 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -337,7 +337,7 @@ target_ulong pmpcfg_csr_read(CPURISCVState *env, uint32_t
reg_index)
for (i = 0; i < sizeof(target_ulong); i++) {
val = pmp_read_cfg(env, (reg_index * sizeof(target_ulong)) + i);
-cfg_val |= (val << (i * 8));
+cfg_val |= ((target_ulong)val << ((target_ulong)i * 8));
}
PMP_DEBUG("hart " TARGET_FMT_ld ": reg%d, val: 0x" TARGET_FMT_lx,
--
2.7.4
[Qemu-devel] [PATCH] target/riscv/pmp.c: Fix PMP NAPOT decoding bug
According to the RISC-V priv. v1.10 ISA document,
pmpaddr register stores (base_addr | (size/2 - 1)) >> 2 for a
NAPOT-encoded address.
However, the current code decodes (base_addr | (size - 1)) >> 3 which
leads to a wrong base address and size.
Signed-off-by: Dayeol Lee
Reviewed-by: Alistair Francis
---
target/riscv/pmp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index f432f3b..c4c6b09 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -138,7 +138,7 @@ static void pmp_decode_napot(target_ulong a, target_ulong
*sa, target_ulong *ea)
return;
} else {
target_ulong t1 = ctz64(~a);
-target_ulong base = (a & ~(((target_ulong)1 << t1) - 1)) << 3;
+target_ulong base = (a & ~(((target_ulong)1 << t1) - 1)) << 2;
target_ulong range = ((target_ulong)1 << (t1 + 3)) - 1;
*sa = base;
*ea = base + range;
--
2.7.4
[Qemu-devel] [PATCH] target/riscv/pmp.c: Fix PMP range boundary address bug
A wrong address is passed to `pmp_is_in_range` while checking if a
memory access is within a PMP range.
Since the ending address of the pmp range (i.e., pmp_state.addr[i].ea)
is set to the last address in the range (i.e., pmp base + pmp size - 1),
memory accesses containg the last address in the range will always fail.
For example, assume that a PMP range is 4KB from 0x87654000 such that
the last address within the range is 0x87654fff.
1-byte access to 0x87654fff should be considered to be fully inside the
PMP range.
However the access now fails and complains partial inclusion because
pmp_is_in_range(env, i, addr + size) returns 0 whereas
pmp_is_in_range(env, i, addr) returns 1.
Signed-off-by: Dayeol Lee
Reviewed-by: Alistair Francis
---
target/riscv/pmp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index c4c6b09..459e556 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -245,7 +245,7 @@ bool pmp_hart_has_privs(CPURISCVState *env, target_ulong
addr,
from low to high */
for (i = 0; i < MAX_RISCV_PMPS; i++) {
s = pmp_is_in_range(env, i, addr);
-e = pmp_is_in_range(env, i, addr + size);
+e = pmp_is_in_range(env, i, addr + size - 1);
/* partially inside */
if ((s + e) == 1) {
--
2.7.4
