Re: [PATCH v2 14/18] target/riscv: Compile pmp.c once

[Anton Johansson via qemu development <qemu-devel@nongnu.org>]

On 07/10/26, Philippe Mathieu-Daudé wrote:
> Phil.
> 
> Regards,
> 
> I missed it, but again Anton's comment were in this patch
> description I wouldn't had barked.
> 
> How does this will work in heterogeneous binary with one
> RV32 and another RV64 cores running together?
> 
> [we tend to avoid top-posting on technical lists]

Ah my bad I should have mentioned it here also.

riscv_cpu_mxl_bits() vs target_long_bits() is, from my understanding,
one of the main discrepancies between rv32 on qemu-riscv32 vs qemu-riscv64,
and they are not equivalent since 32-bit code on qemu-riscv64 will
still have target_long_bits() == 64, but riscv_cpu_mxl_bits() == 32.

For the time being using target_long_bits() preserves the original
behaviour.  In a heterogeneous setting qemu-riscv64 can already run
RV32 and RV64 code despite target_long_bits() == 64, but I'll take a
pass over the code to check for incorrect uses of target_long_bits()
and submit that separately since that would change behaviour.

//Anton

> 
> On 7/10/26 15:39, Yonggang Luo wrote:
> > It's here https://lore.kernel.org/qemu-devel/ar9WZip2Q2G0qvQI@hive.localdomain/
> > 
> > From: Anton Johansson via qemu development <qemu-devel@nongnu.org>
> > 
> > 
> > I wouldn't be surprised if that's the case, although I'm personally a
> > bit scared of riscv_cpu_mxl_bits() since it returns 128 for MXL_128, so
> > if it's not used carefully or on clear MXL_[32|64] paths you could easily
> > get UB.
> > 
> > 
> > On Wed, Oct 7, 2026 at 9:26 PM Philippe Mathieu-Daudé
> > <philmd@oss.qualcomm.com> wrote:
> > > 
> > > On 7/10/26 13:51, Yonggang Luo wrote:
> > > > On Wed, Oct 7, 2026 at 6:25 PM Philippe Mathieu-Daudé
> > > > <philmd@oss.qualcomm.com> wrote:
> > > > > 
> > > > > On 7/10/26 10:43, Anton Johansson wrote:
> > > > > > Reviewed-by: Pierrick Bouvier <pierrick.bouvier@oss.qualcomm.com>
> > > > > > Signed-off-by: Anton Johansson <anjo@rev.ng>
> > > > > > ---
> > > > > >     target/riscv/tcg/pmp.c       | 5 +++--
> > > > > >     target/riscv/tcg/meson.build | 2 +-
> > > > > >     2 files changed, 4 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/target/riscv/tcg/pmp.c b/target/riscv/tcg/pmp.c
> > > > > > index 99345d63dc..2d7194de10 100644
> > > > > > --- a/target/riscv/tcg/pmp.c
> > > > > > +++ b/target/riscv/tcg/pmp.c
> > > > > > @@ -638,19 +638,20 @@ uint64_t pmpaddr_csr_read(CPURISCVState *env, uint32_t addr_index)
> > > > > >         if (addr_index < pmp_regions) {
> > > > > >             val = env->pmp_state.pmp[addr_index].addr_reg;
> > > > > >             int g = pmp_get_granularity_g(env);
> > > > > > +        const unsigned long_bits = target_long_bits();
> > > > > 
> > > > > We need to fetch this information from @env. Maybe riscv_cpu_mxl_bits()?
> > > > 
> > > > discussed before, there is possible be 128bits
> > > 
> > > Please point me to the discussion (directly, saving us a ping / pong),
> > > as:
> > > 
> > >     '''
> > >     This was already discussed on v42:
> > > 
> > >     https://lore.kernel.org/qemu-devel/some-message-id-v42-20128438/
> > >     '''
> > > 
> > > Having the discussion decision resumed in the patch description will
> > > prevent further reviewers to ask again the same.
> > > 
> > > What do you mean by "possible be 128bits"? If we want to cap the
> > > value we can use MIN(64, riscv_cpu_mxl_bits(env)).
> > > 
> > > > 
> > > > > >             switch (pmp_get_a_field(env->pmp_state.pmp[addr_index].cfg_reg)) {
> > > > > >             case PMP_AMATCH_OFF:
> > > > > >                 /* fallthrough */
> > > > > >             case PMP_AMATCH_TOR:
> > > > > >                 /* Bit [g-1:0] read all zero */
> > > > > > -            if (g >= 1 && g < TARGET_LONG_BITS) {
> > > > > > +            if (g >= 1 && g < long_bits) {
> > > > > >                     uint64_t granule = 1ULL << g;
> > > > > >                     val = ROUND_DOWN(val, granule);
> > > > > >                 }
> > > > > >                 break;
> > > > > >             case PMP_AMATCH_NAPOT:
> > > > > >                 /* Bit [g-2:0] read all one */
> > > > > > -            if (g >= 2 && g < TARGET_LONG_BITS) {
> > > > > > +            if (g >= 2 && g < long_bits) {
> > > > > >                     val = deposit64(val, 0, g - 1, -1ULL);
> > > > > >                 }
> > > > > >                 break;
> > > > > 
> > > > 
> > > > 
> > > 
> > 
> > 
>