* [PATCH v5] target/riscv: update checks on writing pmpcfg for Smepmp to version 1.0
@ 2023-11-14 2:22 Alvin Chang via
2023-12-06 3:38 ` Alistair Francis
0 siblings, 1 reply; 5+ messages in thread
From: Alvin Chang via @ 2023-11-14 2:22 UTC (permalink / raw)
To: qemu-riscv, qemu-devel; +Cc: alistair.francis, liweiwei, Alvin Chang
Current checks on writing pmpcfg for Smepmp follows Smepmp version
0.9.1. However, Smepmp specification has already been ratified, and
there are some differences between version 0.9.1 and 1.0. In this
commit we update the checks of writing pmpcfg to follow Smepmp version
1.0.
When mseccfg.MML is set, the constraints to modify PMP rules are:
1. Locked rules cannot be removed or modified until a PMP reset, unless
mseccfg.RLB is set.
2. From Smepmp specification version 1.0, chapter 2 section 4b:
Adding a rule with executable privileges that either is M-mode-only
or a locked Shared-Region is not possible and such pmpcfg writes are
ignored, leaving pmpcfg unchanged.
The commit transfers the value of pmpcfg into the index of the Smepmp
truth table, and checks the rules by aforementioned specification
changes.
Signed-off-by: Alvin Chang <alvinga@andestech.com>
---
Changes from v4: Rebase on master.
Changes from v3: Modify "epmp_operation" to "smepmp_operation".
Changes from v2: Adopt switch case ranges and numerical order.
Changes from v1: Convert ePMP over to Smepmp.
target/riscv/pmp.c | 40 ++++++++++++++++++++++++++++++++--------
1 file changed, 32 insertions(+), 8 deletions(-)
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
index 162e88a90a..4069514069 100644
--- a/target/riscv/pmp.c
+++ b/target/riscv/pmp.c
@@ -102,16 +102,40 @@ static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
locked = false;
}
- /* mseccfg.MML is set */
- if (MSECCFG_MML_ISSET(env)) {
- /* not adding execute bit */
- if ((val & PMP_LOCK) != 0 && (val & PMP_EXEC) != PMP_EXEC) {
+ /*
+ * mseccfg.MML is set. Locked rules cannot be removed or modified
+ * until a PMP reset. Besides, from Smepmp specification version 1.0
+ * , chapter 2 section 4b says:
+ * Adding a rule with executable privileges that either is
+ * M-mode-only or a locked Shared-Region is not possible and such
+ * pmpcfg writes are ignored, leaving pmpcfg unchanged.
+ */
+ if (MSECCFG_MML_ISSET(env) && !pmp_is_locked(env, pmp_index)) {
+ /*
+ * Convert the PMP permissions to match the truth table in the
+ * Smepmp spec.
+ */
+ const uint8_t smepmp_operation =
+ ((val & PMP_LOCK) >> 4) | ((val & PMP_READ) << 2) |
+ (val & PMP_WRITE) | ((val & PMP_EXEC) >> 2);
+
+ switch (smepmp_operation) {
+ case 0 ... 8:
locked = false;
- }
- /* shared region and not adding X bit */
- if ((val & PMP_LOCK) != PMP_LOCK &&
- (val & 0x7) != (PMP_WRITE | PMP_EXEC)) {
+ break;
+ case 9 ... 11:
+ break;
+ case 12:
+ locked = false;
+ break;
+ case 13:
+ break;
+ case 14:
+ case 15:
locked = false;
+ break;
+ default:
+ g_assert_not_reached();
}
}
} else {
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: [PATCH v5] target/riscv: update checks on writing pmpcfg for Smepmp to version 1.0
@ 2023-12-06 1:12 Alvin Chang
0 siblings, 0 replies; 5+ messages in thread
From: Alvin Chang @ 2023-12-06 1:12 UTC (permalink / raw)
To: qemu-riscv, qemu-devel; +Cc: alistair.francis, Andrew Jones, liweiwei, dbarboza
[-- Attachment #1: Type: text/plain, Size: 4280 bytes --]
Ping for review.
> -----Original Message-----
> From: Alvin Che-Chia Chang(張哲嘉) <alvinga@andestech.com>
> Sent: Tuesday, November 14, 2023 10:23 AM
> To: qemu-riscv@nongnu.org; qemu-devel@nongnu.org
> Cc: alistair.francis@wdc.com; liweiwei@iscas.ac.cn; Alvin Che-Chia Chang(張
> 哲嘉) <alvinga@andestech.com>
> Subject: [PATCH v5] target/riscv: update checks on writing pmpcfg for
Smepmp
> to version 1.0
>
> Current checks on writing pmpcfg for Smepmp follows Smepmp version 0.9.1.
> However, Smepmp specification has already been ratified, and there are
some
> differences between version 0.9.1 and 1.0. In this commit we update the
> checks of writing pmpcfg to follow Smepmp version 1.0.
>
> When mseccfg.MML is set, the constraints to modify PMP rules are:
> 1. Locked rules cannot be removed or modified until a PMP reset, unless
> mseccfg.RLB is set.
> 2. From Smepmp specification version 1.0, chapter 2 section 4b:
> Adding a rule with executable privileges that either is M-mode-only
> or a locked Shared-Region is not possible and such pmpcfg writes are
> ignored, leaving pmpcfg unchanged.
>
> The commit transfers the value of pmpcfg into the index of the Smepmp
truth
> table, and checks the rules by aforementioned specification changes.
>
> Signed-off-by: Alvin Chang <alvinga@andestech.com>
> ---
> Changes from v4: Rebase on master.
>
> Changes from v3: Modify "epmp_operation" to "smepmp_operation".
>
> Changes from v2: Adopt switch case ranges and numerical order.
>
> Changes from v1: Convert ePMP over to Smepmp.
>
> target/riscv/pmp.c | 40 ++++++++++++++++++++++++++++++++--------
> 1 file changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c index
> 162e88a90a..4069514069 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -102,16 +102,40 @@ static bool pmp_write_cfg(CPURISCVState *env,
> uint32_t pmp_index, uint8_t val)
> locked = false;
> }
>
> - /* mseccfg.MML is set */
> - if (MSECCFG_MML_ISSET(env)) {
> - /* not adding execute bit */
> - if ((val & PMP_LOCK) != 0 && (val & PMP_EXEC) !=
> PMP_EXEC) {
> + /*
> + * mseccfg.MML is set. Locked rules cannot be removed or
> modified
> + * until a PMP reset. Besides, from Smepmp specification
> version 1.0
> + * , chapter 2 section 4b says:
> + * Adding a rule with executable privileges that either is
> + * M-mode-only or a locked Shared-Region is not possible and
> such
> + * pmpcfg writes are ignored, leaving pmpcfg unchanged.
> + */
> + if (MSECCFG_MML_ISSET(env) && !pmp_is_locked(env,
> pmp_index)) {
> + /*
> + * Convert the PMP permissions to match the truth table
> in the
> + * Smepmp spec.
> + */
> + const uint8_t smepmp_operation =
> + ((val & PMP_LOCK) >> 4) | ((val & PMP_READ) << 2)
> |
> + (val & PMP_WRITE) | ((val & PMP_EXEC) >> 2);
> +
> + switch (smepmp_operation) {
> + case 0 ... 8:
> locked = false;
> - }
> - /* shared region and not adding X bit */
> - if ((val & PMP_LOCK) != PMP_LOCK &&
> - (val & 0x7) != (PMP_WRITE | PMP_EXEC)) {
> + break;
> + case 9 ... 11:
> + break;
> + case 12:
> + locked = false;
> + break;
> + case 13:
> + break;
> + case 14:
> + case 15:
> locked = false;
> + break;
> + default:
> + g_assert_not_reached();
> }
> }
> } else {
> --
> 2.34.1
[-- Attachment #2: Type: text/html, Size: 19218 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5] target/riscv: update checks on writing pmpcfg for Smepmp to version 1.0
2023-11-14 2:22 Alvin Chang via
@ 2023-12-06 3:38 ` Alistair Francis
0 siblings, 0 replies; 5+ messages in thread
From: Alistair Francis @ 2023-12-06 3:38 UTC (permalink / raw)
To: Alvin Chang; +Cc: qemu-riscv, qemu-devel, alistair.francis, liweiwei
On Tue, Nov 14, 2023 at 12:24 PM Alvin Chang via <qemu-devel@nongnu.org> wrote:
>
> Current checks on writing pmpcfg for Smepmp follows Smepmp version
> 0.9.1. However, Smepmp specification has already been ratified, and
> there are some differences between version 0.9.1 and 1.0. In this
> commit we update the checks of writing pmpcfg to follow Smepmp version
> 1.0.
>
> When mseccfg.MML is set, the constraints to modify PMP rules are:
> 1. Locked rules cannot be removed or modified until a PMP reset, unless
> mseccfg.RLB is set.
> 2. From Smepmp specification version 1.0, chapter 2 section 4b:
> Adding a rule with executable privileges that either is M-mode-only
> or a locked Shared-Region is not possible and such pmpcfg writes are
> ignored, leaving pmpcfg unchanged.
>
> The commit transfers the value of pmpcfg into the index of the Smepmp
> truth table, and checks the rules by aforementioned specification
> changes.
>
> Signed-off-by: Alvin Chang <alvinga@andestech.com>
> ---
> Changes from v4: Rebase on master.
>
> Changes from v3: Modify "epmp_operation" to "smepmp_operation".
>
> Changes from v2: Adopt switch case ranges and numerical order.
>
> Changes from v1: Convert ePMP over to Smepmp.
>
> target/riscv/pmp.c | 40 ++++++++++++++++++++++++++++++++--------
> 1 file changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 162e88a90a..4069514069 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -102,16 +102,40 @@ static bool pmp_write_cfg(CPURISCVState *env, uint32_t pmp_index, uint8_t val)
> locked = false;
> }
>
> - /* mseccfg.MML is set */
> - if (MSECCFG_MML_ISSET(env)) {
> - /* not adding execute bit */
> - if ((val & PMP_LOCK) != 0 && (val & PMP_EXEC) != PMP_EXEC) {
> + /*
> + * mseccfg.MML is set. Locked rules cannot be removed or modified
> + * until a PMP reset. Besides, from Smepmp specification version 1.0
> + * , chapter 2 section 4b says:
> + * Adding a rule with executable privileges that either is
> + * M-mode-only or a locked Shared-Region is not possible and such
> + * pmpcfg writes are ignored, leaving pmpcfg unchanged.
> + */
> + if (MSECCFG_MML_ISSET(env) && !pmp_is_locked(env, pmp_index)) {
This is tricky and took me a while to get my head around.
From what I can tell, there is a bug in the spec.
The spec specifically states that:
"""
The meaning of pmpcfg.L changes: Instead of marking a rule as locked
and enforced in all modes, it
now marks a rule as M-mode-only when set and S/U-mode-only when unset.
"""
So the check for !pmp_is_locked() sounds correct.
But then they add:
"""
The formerly reserved encoding of pmpcfg.RW=01, and the encoding
pmpcfg.LRWX=1111, now encode a Shared-Region.
"""
Which contradicts what they just said.
I *think* we want to ignore the locked bit here. We don't actually
care if it's already set, instead we care if the region is an M-mode
only region from the 2.1 table
I think the best bet here is to create a helper function that takes a
pmpcfg value and returns if it is M-mode only. Then we should check if
the current pmp_index is M-mode only OR if we are adding one and then
reject that.
Does that make sense?
Alistair
> + /*
> + * Convert the PMP permissions to match the truth table in the
> + * Smepmp spec.
> + */
> + const uint8_t smepmp_operation =
> + ((val & PMP_LOCK) >> 4) | ((val & PMP_READ) << 2) |
> + (val & PMP_WRITE) | ((val & PMP_EXEC) >> 2);
> +
> + switch (smepmp_operation) {
> + case 0 ... 8:
> locked = false;
> - }
> - /* shared region and not adding X bit */
> - if ((val & PMP_LOCK) != PMP_LOCK &&
> - (val & 0x7) != (PMP_WRITE | PMP_EXEC)) {
> + break;
> + case 9 ... 11:
> + break;
> + case 12:
> + locked = false;
> + break;
> + case 13:
> + break;
> + case 14:
> + case 15:
> locked = false;
> + break;
> + default:
> + g_assert_not_reached();
> }
> }
> } else {
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5] target/riscv: update checks on writing pmpcfg for Smepmp to version 1.0
@ 2023-12-06 5:36 Alvin Chang
2023-12-15 5:25 ` Alistair Francis
0 siblings, 1 reply; 5+ messages in thread
From: Alvin Chang @ 2023-12-06 5:36 UTC (permalink / raw)
To: qemu-riscv, qemu-devel, alistair.francis; +Cc: liweiwei
[-- Attachment #1: Type: text/plain, Size: 6716 bytes --]
> -----Original Message-----
> From: Alistair Francis <alistair23@gmail.com>
> Sent: Wednesday, December 6, 2023 11:39 AM
> To: Alvin Che-Chia Chang(張哲嘉) <alvinga@andestech.com>
> Cc: qemu-riscv@nongnu.org; qemu-devel@nongnu.org;
> alistair.francis@wdc.com; liweiwei@iscas.ac.cn
> Subject: Re: [PATCH v5] target/riscv: update checks on writing pmpcfg for
> Smepmp to version 1.0
>
> On Tue, Nov 14, 2023 at 12:24 PM Alvin Chang via <qemu-devel@nongnu.org>
> wrote:
> >
> > Current checks on writing pmpcfg for Smepmp follows Smepmp version
> > 0.9.1. However, Smepmp specification has already been ratified, and
> > there are some differences between version 0.9.1 and 1.0. In this
> > commit we update the checks of writing pmpcfg to follow Smepmp version
> > 1.0.
> >
> > When mseccfg.MML is set, the constraints to modify PMP rules are:
> > 1. Locked rules cannot be removed or modified until a PMP reset, unless
> > mseccfg.RLB is set.
> > 2. From Smepmp specification version 1.0, chapter 2 section 4b:
> > Adding a rule with executable privileges that either is M-mode-only
> > or a locked Shared-Region is not possible and such pmpcfg writes are
> > ignored, leaving pmpcfg unchanged.
> >
> > The commit transfers the value of pmpcfg into the index of the Smepmp
> > truth table, and checks the rules by aforementioned specification
> > changes.
> >
> > Signed-off-by: Alvin Chang <alvinga@andestech.com>
> > ---
> > Changes from v4: Rebase on master.
> >
> > Changes from v3: Modify "epmp_operation" to "smepmp_operation".
> >
> > Changes from v2: Adopt switch case ranges and numerical order.
> >
> > Changes from v1: Convert ePMP over to Smepmp.
> >
> > target/riscv/pmp.c | 40 ++++++++++++++++++++++++++++++++--------
> > 1 file changed, 32 insertions(+), 8 deletions(-)
> >
> > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c index
> > 162e88a90a..4069514069 100644
> > --- a/target/riscv/pmp.c
> > +++ b/target/riscv/pmp.c
> > @@ -102,16 +102,40 @@ static bool pmp_write_cfg(CPURISCVState *env,
> uint32_t pmp_index, uint8_t val)
> > locked = false;
> > }
> >
> > - /* mseccfg.MML is set */
> > - if (MSECCFG_MML_ISSET(env)) {
> > - /* not adding execute bit */
> > - if ((val & PMP_LOCK) != 0 && (val & PMP_EXEC) !=
> PMP_EXEC) {
> > + /*
> > + * mseccfg.MML is set. Locked rules cannot be removed or
> modified
> > + * until a PMP reset. Besides, from Smepmp specification
> version 1.0
> > + * , chapter 2 section 4b says:
> > + * Adding a rule with executable privileges that either is
> > + * M-mode-only or a locked Shared-Region is not possible
> and such
> > + * pmpcfg writes are ignored, leaving pmpcfg unchanged.
> > + */
> > + if (MSECCFG_MML_ISSET(env) && !pmp_is_locked(env,
> > + pmp_index)) {
>
> This is tricky and took me a while to get my head around.
>
> From what I can tell, there is a bug in the spec.
>
> The spec specifically states that:
>
> """
> The meaning of pmpcfg.L changes: Instead of marking a rule as locked and
> enforced in all modes, it now marks a rule as M-mode-only when set and
> S/U-mode-only when unset.
> """
>
> So the check for !pmp_is_locked() sounds correct.
>
> But then they add:
>
> """
> The formerly reserved encoding of pmpcfg.RW=01, and the encoding
> pmpcfg.LRWX=1111, now encode a Shared-Region.
> """
>
> Which contradicts what they just said.
Yes you are right, it seems there are some misleading words.
>
> I *think* we want to ignore the locked bit here. We don't actually care
if it's
> already set, instead we care if the region is an M-mode only region from
the
> 2.1 table
The check for !pmp_is_locked() is because spec says (below table 2.1):
"*Locked rules cannot be removed or modified until a PMP reset, unless
mseccfg.RLB is set."
It is not related to M-mode-only or S/U-mode-only or Shared-Region.
In other words, a pmpcfg where the pmpcfg.L bit was set can not be
configured anymore. Therefore, I think we should not ignore it here, since
we are trying to write a new value into the pmpcfg. If we ignore it, the
locked pmpcfg will be modified and it would violate the spec.
If the pmpcfg was not locked, we also need to check the new value that the
user wants to write. Because chapter 2 section 4b says: "Adding a rule with
executable privileges that either is M-mode-only or a locked Shared-Region
is not possible and such pmpcfg writes are ignored, leaving pmpcfg
unchanged". This checking is implemented as that switch-case
statement, based on table 2.1 truth table.
Alvin Chang
>
> I think the best bet here is to create a helper function that takes a
pmpcfg
> value and returns if it is M-mode only. Then we should check if the
current
> pmp_index is M-mode only OR if we are adding one and then reject that.
>
> Does that make sense?
>
> Alistair
>
> > + /*
> > + * Convert the PMP permissions to match the truth
> table in the
> > + * Smepmp spec.
> > + */
> > + const uint8_t smepmp_operation =
> > + ((val & PMP_LOCK) >> 4) | ((val & PMP_READ) <<
> 2) |
> > + (val & PMP_WRITE) | ((val & PMP_EXEC) >> 2);
> > +
> > + switch (smepmp_operation) {
> > + case 0 ... 8:
> > locked = false;
> > - }
> > - /* shared region and not adding X bit */
> > - if ((val & PMP_LOCK) != PMP_LOCK &&
> > - (val & 0x7) != (PMP_WRITE | PMP_EXEC)) {
> > + break;
> > + case 9 ... 11:
> > + break;
> > + case 12:
> > + locked = false;
> > + break;
> > + case 13:
> > + break;
> > + case 14:
> > + case 15:
> > locked = false;
> > + break;
> > + default:
> > + g_assert_not_reached();
> > }
> > }
> > } else {
> > --
> > 2.34.1
> >
> >
[-- Attachment #2: Type: text/html, Size: 29003 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v5] target/riscv: update checks on writing pmpcfg for Smepmp to version 1.0
2023-12-06 5:36 Alvin Chang
@ 2023-12-15 5:25 ` Alistair Francis
0 siblings, 0 replies; 5+ messages in thread
From: Alistair Francis @ 2023-12-15 5:25 UTC (permalink / raw)
To: Alvin Chang; +Cc: qemu-riscv, qemu-devel, alistair.francis, liweiwei
On Wed, Dec 6, 2023 at 3:37 PM Alvin Chang <vivahavey@gmail.com> wrote:
>
> > -----Original Message-----
>
> > From: Alistair Francis <alistair23@gmail.com>
>
> > Sent: Wednesday, December 6, 2023 11:39 AM
>
> > To: Alvin Che-Chia Chang(張哲嘉) <alvinga@andestech.com>
>
> > Cc: qemu-riscv@nongnu.org; qemu-devel@nongnu.org;
>
> > alistair.francis@wdc.com; liweiwei@iscas.ac.cn
>
> > Subject: Re: [PATCH v5] target/riscv: update checks on writing pmpcfg for
>
> > Smepmp to version 1.0
>
> >
>
> > On Tue, Nov 14, 2023 at 12:24 PM Alvin Chang via <qemu-devel@nongnu.org>
>
> > wrote:
>
> > >
>
> > > Current checks on writing pmpcfg for Smepmp follows Smepmp version
>
> > > 0.9.1. However, Smepmp specification has already been ratified, and
>
> > > there are some differences between version 0.9.1 and 1.0. In this
>
> > > commit we update the checks of writing pmpcfg to follow Smepmp version
>
> > > 1.0.
>
> > >
>
> > > When mseccfg.MML is set, the constraints to modify PMP rules are:
>
> > > 1. Locked rules cannot be removed or modified until a PMP reset, unless
>
> > > mseccfg.RLB is set.
>
> > > 2. From Smepmp specification version 1.0, chapter 2 section 4b:
>
> > > Adding a rule with executable privileges that either is M-mode-only
>
> > > or a locked Shared-Region is not possible and such pmpcfg writes are
>
> > > ignored, leaving pmpcfg unchanged.
>
> > >
>
> > > The commit transfers the value of pmpcfg into the index of the Smepmp
>
> > > truth table, and checks the rules by aforementioned specification
>
> > > changes.
>
> > >
>
> > > Signed-off-by: Alvin Chang <alvinga@andestech.com>
>
> > > ---
>
> > > Changes from v4: Rebase on master.
>
> > >
>
> > > Changes from v3: Modify "epmp_operation" to "smepmp_operation".
>
> > >
>
> > > Changes from v2: Adopt switch case ranges and numerical order.
>
> > >
>
> > > Changes from v1: Convert ePMP over to Smepmp.
>
> > >
>
> > > target/riscv/pmp.c | 40 ++++++++++++++++++++++++++++++++--------
>
> > > 1 file changed, 32 insertions(+), 8 deletions(-)
>
> > >
>
> > > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c index
>
> > > 162e88a90a..4069514069 100644
>
> > > --- a/target/riscv/pmp.c
>
> > > +++ b/target/riscv/pmp.c
>
> > > @@ -102,16 +102,40 @@ static bool pmp_write_cfg(CPURISCVState *env,
>
> > uint32_t pmp_index, uint8_t val)
>
> > > locked = false;
>
> > > }
>
> > >
>
> > > - /* mseccfg.MML is set */
>
> > > - if (MSECCFG_MML_ISSET(env)) {
>
> > > - /* not adding execute bit */
>
> > > - if ((val & PMP_LOCK) != 0 && (val & PMP_EXEC) !=
>
> > PMP_EXEC) {
>
> > > + /*
>
> > > + * mseccfg.MML is set. Locked rules cannot be removed or
>
> > modified
>
> > > + * until a PMP reset. Besides, from Smepmp specification
>
> > version 1.0
>
> > > + * , chapter 2 section 4b says:
>
> > > + * Adding a rule with executable privileges that either is
>
> > > + * M-mode-only or a locked Shared-Region is not possible
>
> > and such
>
> > > + * pmpcfg writes are ignored, leaving pmpcfg unchanged.
>
> > > + */
>
> > > + if (MSECCFG_MML_ISSET(env) && !pmp_is_locked(env,
>
> > > + pmp_index)) {
>
> >
>
> > This is tricky and took me a while to get my head around.
>
> >
>
> > From what I can tell, there is a bug in the spec.
>
> >
>
> > The spec specifically states that:
>
> >
>
> > """
>
> > The meaning of pmpcfg.L changes: Instead of marking a rule as locked and
>
> > enforced in all modes, it now marks a rule as M-mode-only when set and
>
> > S/U-mode-only when unset.
>
> > """
>
> >
>
> > So the check for !pmp_is_locked() sounds correct.
>
> >
>
> > But then they add:
>
> >
>
> > """
>
> > The formerly reserved encoding of pmpcfg.RW=01, and the encoding
>
> > pmpcfg.LRWX=1111, now encode a Shared-Region.
>
> > """
>
> >
>
> > Which contradicts what they just said.
In future can you please reply in plain text? Otherwise the formatting
gets a little funky
>
>
> Yes you are right, it seems there are some misleading words.
>
>
> >
>
> > I *think* we want to ignore the locked bit here. We don't actually care if it's
>
> > already set, instead we care if the region is an M-mode only region from the
>
> > 2.1 table
>
>
> The check for !pmp_is_locked() is because spec says (below table 2.1):
>
> "*Locked rules cannot be removed or modified until a PMP reset, unless mseccfg.RLB is set."
>
> It is not related to M-mode-only or S/U-mode-only or Shared-Region.
Yes, but when mseccfg.MML is set
"The meaning of pmpcfg.L changes: Instead of marking a rule as locked
and enforced in all modes, it
now marks a rule as M-mode-only when set and S/U-mode-only when unset."
So the comment below table 2.1 no longer applies
>
>
> In other words, a pmpcfg where the pmpcfg.L bit was set can not be configured anymore. Therefore, I think we should not ignore it here, since we are trying to write a new value into the pmpcfg. If we ignore it, the locked pmpcfg will be modified and it would violate the spec.
>
>
> If the pmpcfg was not locked, we also need to check the new value that the user wants to write. Because chapter 2 section 4b says: "Adding a rule with executable privileges that either is M-mode-only or a locked Shared-Region is not possible and such pmpcfg writes are ignored, leaving pmpcfg unchanged". This checking is implemented as that switch-case statement, based on table 2.1 truth table.
Yeah, that sounds right.
Alistair
>
>
> Alvin Chang
>
>
> >
>
> > I think the best bet here is to create a helper function that takes a pmpcfg
>
> > value and returns if it is M-mode only. Then we should check if the current
>
> > pmp_index is M-mode only OR if we are adding one and then reject that.
>
> >
>
> > Does that make sense?
>
> >
>
> > Alistair
>
> >
>
> > > + /*
>
> > > + * Convert the PMP permissions to match the truth
>
> > table in the
>
> > > + * Smepmp spec.
>
> > > + */
>
> > > + const uint8_t smepmp_operation =
>
> > > + ((val & PMP_LOCK) >> 4) | ((val & PMP_READ) <<
>
> > 2) |
>
> > > + (val & PMP_WRITE) | ((val & PMP_EXEC) >> 2);
>
> > > +
>
> > > + switch (smepmp_operation) {
>
> > > + case 0 ... 8:
>
> > > locked = false;
>
> > > - }
>
> > > - /* shared region and not adding X bit */
>
> > > - if ((val & PMP_LOCK) != PMP_LOCK &&
>
> > > - (val & 0x7) != (PMP_WRITE | PMP_EXEC)) {
>
> > > + break;
>
> > > + case 9 ... 11:
>
> > > + break;
>
> > > + case 12:
>
> > > + locked = false;
>
> > > + break;
>
> > > + case 13:
>
> > > + break;
>
> > > + case 14:
>
> > > + case 15:
>
> > > locked = false;
>
> > > + break;
>
> > > + default:
>
> > > + g_assert_not_reached();
>
> > > }
>
> > > }
>
> > > } else {
>
> > > --
>
> > > 2.34.1
>
> > >
>
> > >
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-12-15 5:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-06 1:12 [PATCH v5] target/riscv: update checks on writing pmpcfg for Smepmp to version 1.0 Alvin Chang
-- strict thread matches above, loose matches on Subject: below --
2023-12-06 5:36 Alvin Chang
2023-12-15 5:25 ` Alistair Francis
2023-11-14 2:22 Alvin Chang via
2023-12-06 3:38 ` Alistair Francis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).