Re: [PATCH 4/5] target/riscv: Check memory access to meet svuket rule
Thank you for your advice.
I will take them after the spec is more finalized.
Sincerely,
Fea
On Wed, Sep 4, 2024 at 6:18 AM Daniel Henrique Barboza <
dbarb...@ventanamicro.com> wrote:
>
>
> On 9/3/24 3:17 AM, Fea.Wang wrote:
> > Follow the Svukte spec, do the memory access address checking
> >
> > 1. Include instruction fetches or explicit memory accesses
> > 2. System run in effective privilege U or VU
> > 3. Check senvcfg[UKTE] being set, or hstatus[HUKTE] being set if
> > instruction is HLV, HLVX, HSV and excute from U mode to VU mode
> > 4. Depend on Sv39 and check virtual addresses bit[SXLEN-1]
> > 5. Raises a page-fault exception corresponding to the original access
> > type.
> >
> > Ref: https://github.com/riscv/riscv-isa-manual/pull/1564/files
> >
> > Signed-off-by: Frank Chang
> > Signed-off-by: Fea.Wang
> > Reviewed-by: Jim Shu
> > ---
> > target/riscv/cpu_helper.c | 55 +++
> > 1 file changed, 55 insertions(+)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 395a1d9140..db65ed14b9 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -777,6 +777,54 @@ static int get_physical_address_pmp(CPURISCVState
> *env, int *prot, hwaddr addr,
> > return TRANSLATE_SUCCESS;
> > }
> >
> > +/*
> > + * Return 'true' means no need to do svukte check, or need to do svukte
> and the
> > + * address is valid. Return 'false' means need to do svukte check but
> address
> > + * is invalid.
> > + */
> > +static bool check_svukte_valid(CPURISCVState *env, vaddr addr,
> > + int mode, bool virt)
> > +{
> > +if (VM_1_10_SV39 != get_field(env->satp, SATP64_MODE)) {
> > +/* Svukte extension depends on Sv39. */
> > +return true;
> > +}
> > +
> > +/*
> > + * Svukte extension is qualified only in U or VU-mode.
> > + *
> > + * Effective mode can be switched to U or VU-mode by:
> > + * - M-mode + mstatus.MPRV=1 + mstatus.MPP=U-mode.
> > + * - Execute HLV/HLVX/HSV from HS-mode + hstatus.SPVP=0.
> > + * - U-mode.
> > + * - VU-mode.
> > + * - Execute HLV/HLVX/HSV from U-mode + hstatus.HU=1.
> > + */
> > +if (mode != PRV_U) {
> > +return true;
> > +}
> > +
> > +/*
> > + * Check hstatus.HUKTE if the effective mode is switched to VU-mode
> by
> > + * executing HLV/HLVX/HSV in U-mode.
> > + * For other cases, check senvcfg.UKTE.
> > + */
> > +bool ukte = (env->priv == PRV_U && !env->virt_enabled && virt) ?
> > + !!(env->hstatus &
> HSTATUS_HUKTE) :
> > + !!(env->senvcfg &
> SENVCFG_UKTE);
>
> I would move the 'bool ukte' to the start of the function, and would avoid
> the
> ternary to make the code a bit more readable:
>
>if (env->priv == PRV_U && !env->virt_enabled && virt) {
>ukte = !!(env->hstatus & HSTATUS_HUKTE);
>} else {
>ukte = !!(env->senvcfg & SENVCFG_UKTE);
>}
>
>
> > +
> > +if (!ukte) {
> > +return true;
> > +}
> > +
> > +uint32_t sxl = riscv_cpu_sxl(env);
> > +sxl = (sxl == 0) ? MXL_RV32 : sxl;
> > +uint32_t sxlen = 32 * sxl;
> > +uint64_t high_bit = addr & (1UL << (sxlen - 1));
> > +
> > +return !high_bit;
> > +}
> > +
> > /*
> >* get_physical_address - get the physical address for this virtual
> address
> >*
> > @@ -814,11 +862,18 @@ static int get_physical_address(CPURISCVState
> *env, hwaddr *physical,
> > MemTxResult res;
> > MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
> > int mode = mmuidx_priv(mmu_idx);
> > +bool virt = mmuidx_2stage(mmu_idx);
> > bool use_background = false;
> > hwaddr ppn;
> > int napot_bits = 0;
> > target_ulong napot_mask;
> >
> > +if (first_stage) {
> > +if (!check_svukte_valid(env, addr, mode, virt)) {
> > +return TRANSLATE_FAIL;
> > +}
> > +}
> > +
>
> We can avoid the nested 'if':
>
> > +if (first_stage && !check_svukte_valid(env, addr, mode, virt)) {
> > +return TRANSLATE_FAIL;
> > +}
>
>
> I would also add a check for ext_svukte before doing any checks. If we
> don't have
> the ext enabled we can skip everything:
>
>
> > +if (env_archcpu(env)->cfg.ext_svukte && first_stage &&
> > +!check_svukte_valid(env, addr, mode, virt)) {
> > +return TRANSLATE_FAIL;
> > +}
>
>
>
> Thanks,
>
> Daniel
>
>
> > /*
> >* Check if we should use the background registers for the two
> >* stage translation. We don't need to check if we actually need
>
Re: [PATCH v2 2/3] hw/dma: Add a trace log for a description loading failure
Sure, I will fix it in the next patch series. Thank you Sincerely, Fea On Mon, Jun 10, 2024 at 7:49 PM Philippe Mathieu-Daudé wrote: > Hi Fea, > > On 4/6/24 09:15, Fea.Wang wrote: > > Due to a description loading failure, adding a trace log makes observing > > the DMA behavior easy. > > > > Signed-off-by: Fea.Wang > > Reviewed-by: Edgar E. Iglesias > > Reviewed-by: Frank Chang > > --- > > hw/dma/trace-events| 3 +++ > > hw/dma/xilinx_axidma.c | 3 +++ > > 2 files changed, 6 insertions(+) > > > > +# xilinx_axidma.c > > +xilinx_axidma_loading_desc_fail(uint32_t res) "error:%d" > > Unsigned format is "%u". > > Regards, > > Phil. >
Re: [PATCH v3 3/6] target/riscv: Support the version for ss1p13
Sure, I will reorder the commits in the next patch series.
Thank you
Sincerely,
Fea
On Thu, Jun 6, 2024 at 7:58 AM Alistair Francis
wrote:
> On Tue, Jun 4, 2024 at 4:23 PM Fea.Wang wrote:
> >
> > Add RISC-V privilege 1.13 support.
> >
> > Signed-off-by: Fea.Wang
> > Signed-off-by: Fea.Wang
> > Reviewed-by: Frank Chang
> > Reviewed-by: Weiwei Li
> > Reviewed-by: LIU Zhiwei
>
> This should be the last patch in the series. The idea is that we add
> support and then let users enable it.
>
> Alistair
>
> > ---
> > target/riscv/cpu.c | 6 +-
> > target/riscv/tcg/tcg-cpu.c | 4
> > 2 files changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index e9e69b9863..02c1e12a03 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -1775,7 +1775,9 @@ static int priv_spec_from_str(const char
> *priv_spec_str)
> > {
> > int priv_version = -1;
> >
> > -if (!g_strcmp0(priv_spec_str, PRIV_VER_1_12_0_STR)) {
> > +if (!g_strcmp0(priv_spec_str, PRIV_VER_1_13_0_STR)) {
> > +priv_version = PRIV_VERSION_1_13_0;
> > +} else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_12_0_STR)) {
> > priv_version = PRIV_VERSION_1_12_0;
> > } else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_11_0_STR)) {
> > priv_version = PRIV_VERSION_1_11_0;
> > @@ -1795,6 +1797,8 @@ const char *priv_spec_to_str(int priv_version)
> > return PRIV_VER_1_11_0_STR;
> > case PRIV_VERSION_1_12_0:
> > return PRIV_VER_1_12_0_STR;
> > +case PRIV_VERSION_1_13_0:
> > +return PRIV_VER_1_13_0_STR;
> > default:
> > return NULL;
> > }
> > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> > index 60fe0fd060..595d3b5b8f 100644
> > --- a/target/riscv/tcg/tcg-cpu.c
> > +++ b/target/riscv/tcg/tcg-cpu.c
> > @@ -318,6 +318,10 @@ static void
> riscv_cpu_update_named_features(RISCVCPU *cpu)
> > cpu->cfg.has_priv_1_12 = true;
> > }
> >
> > +if (cpu->env.priv_ver >= PRIV_VERSION_1_13_0) {
> > +cpu->cfg.has_priv_1_13 = true;
> > +}
> > +
> > /* zic64b is 1.12 or later */
> > cpu->cfg.ext_zic64b = cpu->cfg.cbom_blocksize == 64 &&
> >cpu->cfg.cbop_blocksize == 64 &&
> > --
> > 2.34.1
> >
> >
>
Re: [PATCH 4/4] hw/net: Fix the transmission return size
I just encountered this issue when running Linux, and the trouble will be fixed after the patches. So I think they work. Sincerely, Fea On Mon, Jun 3, 2024 at 6:31 PM Edgar E. Iglesias wrote: > On Mon, Jun 3, 2024 at 7:48 AM Fea.Wang wrote: > >> Fix the transmission return size because not all bytes could be >> transmitted successfully. So, return a successful length instead of a >> constant value. >> >> > How did you test this patch, on Linux or something else? I have some > memory that we had some trouble with similar patches before. > > Anyway, the change looks good to me: > Reviewed-by: Edgar E. Iglesias > > > >> Signed-off-by: Fea.Wang >> --- >> hw/net/xilinx_axienet.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c >> index 7d1fd37b4a..05d41bd548 100644 >> --- a/hw/net/xilinx_axienet.c >> +++ b/hw/net/xilinx_axienet.c >> @@ -847,7 +847,7 @@ static ssize_t eth_rx(NetClientState *nc, const >> uint8_t *buf, size_t size) >> axienet_eth_rx_notify(s); >> >> enet_update_irq(s); >> -return size; >> +return s->rxpos; >> } >> >> static size_t >> -- >> 2.34.1 >> >>
Re: [PATCH 2/4] hw/dma: Break the loop when loading descriptions fails
Hi Edgar,
thank you for the advice, I will squash them in the next version of the
patch series.
Sincerely,
Fea
On Mon, Jun 3, 2024 at 6:21 PM Edgar E. Iglesias
wrote:
> On Mon, Jun 3, 2024 at 7:48 AM Fea.Wang wrote:
>
>> When calling the loading a description function, it should be noticed
>> that the function may return a failure value. Breaking the loop is one
>> of the possible ways to handle it.
>>
>> Signed-off-by: Fea.Wang
>>
>
>
> Looks good, a nitpick comment, I would either squash this with patch #1
> or move the change to return of error code in stream_desc_load() to this
> patch.
>
>
>
>
>> ---
>> hw/dma/xilinx_axidma.c | 8 ++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
>> index 4b475e5484..b8ab5a423d 100644
>> --- a/hw/dma/xilinx_axidma.c
>> +++ b/hw/dma/xilinx_axidma.c
>> @@ -291,7 +291,9 @@ static void stream_process_mem2s(struct Stream *s,
>> StreamSink *tx_data_dev,
>> }
>>
>> while (1) {
>> -stream_desc_load(s, s->regs[R_CURDESC]);
>> +if (MEMTX_OK != stream_desc_load(s, s->regs[R_CURDESC])) {
>> +break;
>> +}
>>
>> if (s->desc.status & SDESC_STATUS_COMPLETE) {
>> s->regs[R_DMASR] |= DMASR_HALTED;
>> @@ -348,7 +350,9 @@ static size_t stream_process_s2mem(struct Stream *s,
>> unsigned char *buf,
>> }
>>
>> while (len) {
>> -stream_desc_load(s, s->regs[R_CURDESC]);
>> +if (MEMTX_OK != stream_desc_load(s, s->regs[R_CURDESC])) {
>> +break;
>> +}
>>
>> if (s->desc.status & SDESC_STATUS_COMPLETE) {
>> s->regs[R_DMASR] |= DMASR_HALTED;
>> --
>> 2.34.1
>>
>>
Re: [PATCH 1/4] hw/dma: Enhance error handling in loading description
Hi Edgar,
Thank you for recommending to me. I will make the change in the next
version of the patch series.
Sincerely,
Fea
On Mon, Jun 3, 2024 at 6:19 PM Edgar E. Iglesias
wrote:
> On Mon, Jun 3, 2024 at 7:47 AM Fea.Wang wrote:
>
>> Loading a description from memory may cause a bus-error. In this
>> case, the DMA should stop working, set the error flag, and return
>> the error value.
>>
>> Signed-off-by: Fea.Wang
>>
>
>
> Hi Fea,
>
> I've got a couple of small comments:
>
>
> ---
>> hw/dma/xilinx_axidma.c | 16 ++--
>> 1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
>> index 0ae056ed06..4b475e5484 100644
>> --- a/hw/dma/xilinx_axidma.c
>> +++ b/hw/dma/xilinx_axidma.c
>> @@ -71,8 +71,10 @@ enum {
>> enum {
>> DMASR_HALTED = 1,
>> DMASR_IDLE = 2,
>> +DMASR_SLVERR = 1 << 5,
>>
>
> We should also add DMASR_DECERR = 1 << 6
>
>
>> DMASR_IOC_IRQ = 1 << 12,
>> DMASR_DLY_IRQ = 1 << 13,
>> +DMASR_ERR_IRQ = 1 << 14,
>>
>> DMASR_IRQ_MASK = 7 << 12
>> };
>> @@ -190,17 +192,27 @@ static inline int streamid_from_addr(hwaddr addr)
>> return sid;
>> }
>>
>> -static void stream_desc_load(struct Stream *s, hwaddr addr)
>> +static MemTxResult stream_desc_load(struct Stream *s, hwaddr addr)
>> {
>> struct SDesc *d = &s->desc;
>>
>> -address_space_read(&s->dma->as, addr, MEMTXATTRS_UNSPECIFIED, d,
>> sizeof *d);
>> +MemTxResult result = address_space_read(&s->dma->as,
>> +addr, MEMTXATTRS_UNSPECIFIED,
>> +d, sizeof *d);
>> +if (result != MEMTX_OK) {
>> +s->regs[R_DMACR] &= ~DMACR_RUNSTOP;
>> +s->regs[R_DMASR] |= DMASR_HALTED;
>> +s->regs[R_DMASR] |= DMASR_SLVERR;
>>
>
> ... and map MEMTX_DECODE_ERROR to DMASR_DECERR and everything else to
> SLVERR, for example:
> if (result == MEMTX_DECODE_ERROR) {
> s->regs[R_DMASR] |= DMASR_DECERR;
> } else {
> s->regs[R_DMASR] |= DMASR_SLVERR;
> }
>
>
>> +s->regs[R_DMASR] |= DMASR_ERR_IRQ;
>> +return result;
>> +}
>>
>> /* Convert from LE into host endianness. */
>> d->buffer_address = le64_to_cpu(d->buffer_address);
>> d->nxtdesc = le64_to_cpu(d->nxtdesc);
>> d->control = le32_to_cpu(d->control);
>> d->status = le32_to_cpu(d->status);
>> +return result;
>> }
>>
>> static void stream_desc_store(struct Stream *s, hwaddr addr)
>> --
>> 2.34.1
>>
>>
Re: [RESEND PATCH v2 3/5] target/riscv: Add 'P1P13' bit in SMSTATEEN0
Thank you for correcting me.
I will fix it in the next version of the patch series.
Sincerely,
Fea
On Tue, Jun 4, 2024 at 8:54 AM Alistair Francis
wrote:
> On Wed, May 15, 2024 at 6:01 PM Fea.Wang wrote:
> >
> > Based on privilege 1.13 spec, there should be a bit56 for 'P1P13' in
> > SMSTATEEN0 that controls access to the hedeleg.
>
> I don't see this in the spec. I only see P1P13 in mstateen0
>
> Alistair
>
> >
> > Signed-off-by: Fea.Wang
> > Reviewed-by: Frank Chang
> > Reviewed-by: Weiwei Li
> > ---
> > target/riscv/cpu_bits.h | 1 +
> > target/riscv/csr.c | 8
> > 2 files changed, 9 insertions(+)
> >
> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > index 74318a925c..28bd3fb0b4 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -315,6 +315,7 @@
> > #define SMSTATEEN0_CS (1ULL << 0)
> > #define SMSTATEEN0_FCSR (1ULL << 1)
> > #define SMSTATEEN0_JVT (1ULL << 2)
> > +#define SMSTATEEN0_P1P13(1ULL << 56)
> > #define SMSTATEEN0_HSCONTXT (1ULL << 57)
> > #define SMSTATEEN0_IMSIC(1ULL << 58)
> > #define SMSTATEEN0_AIA (1ULL << 59)
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 6b460ee0e8..bdbc8de0e2 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -2248,6 +2248,10 @@ static RISCVException
> write_mstateen0(CPURISCVState *env, int csrno,
> > wr_mask |= SMSTATEEN0_FCSR;
> > }
> >
> > +if (env->priv_ver >= PRIV_VERSION_1_13_0) {
> > +wr_mask |= SMSTATEEN0_P1P13;
> > +}
> > +
> > return write_mstateen(env, csrno, wr_mask, new_val);
> > }
> >
> > @@ -2283,6 +2287,10 @@ static RISCVException
> write_mstateen0h(CPURISCVState *env, int csrno,
> > {
> > uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> >
> > +if (env->priv_ver >= PRIV_VERSION_1_13_0) {
> > +wr_mask |= SMSTATEEN0_P1P13;
> > +}
> > +
> > return write_mstateenh(env, csrno, wr_mask, new_val);
> > }
> >
> > --
> > 2.34.1
> >
> >
>
Re: [RESEND PATCH v2 2/5] target/riscv: Support the version for ss1p13
Got it.
I will fix it in the next version of the patch series.
Sincerely,
Fea
On Tue, Jun 4, 2024 at 8:56 AM Alistair Francis
wrote:
> On Tue, Jun 4, 2024 at 10:46 AM Alistair Francis
> wrote:
> >
> > On Wed, May 15, 2024 at 6:02 PM Fea.Wang wrote:
> > >
> > > Add RISC-V privilege 1.13 support.
> > >
> > > Signed-off-by: Fea.Wang
> > > Reviewed-by: Frank Chang
> > > Reviewed-by: Weiwei Li
> > > Reviewed-by: LIU Zhiwei
> >
> > Reviewed-by: Alistair Francis
>
> Whoops, ignore this.
>
> >
> > Alistair
> >
> > > ---
> > > target/riscv/cpu.c | 6 +-
> > > target/riscv/cpu.h | 4 +++-
> > > target/riscv/cpu_cfg.h | 1 +
> > > target/riscv/tcg/tcg-cpu.c | 4
> > > 4 files changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index 6dd3d7f4a3..ee2ec4c4e5 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -1775,7 +1775,9 @@ static int priv_spec_from_str(const char
> *priv_spec_str)
> > > {
> > > int priv_version = -1;
> > >
> > > -if (!g_strcmp0(priv_spec_str, PRIV_VER_1_12_0_STR)) {
> > > +if (!g_strcmp0(priv_spec_str, PRIV_VER_1_13_0_STR)) {
> > > +priv_version = PRIV_VERSION_1_13_0;
> > > +} else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_12_0_STR)) {
>
> This patch should be split in half. We only want to expose this to
> user after it has been implemented.
>
> Alistair
>
> > > priv_version = PRIV_VERSION_1_12_0;
> > > } else if (!g_strcmp0(priv_spec_str, PRIV_VER_1_11_0_STR)) {
> > > priv_version = PRIV_VERSION_1_11_0;
> > > @@ -1795,6 +1797,8 @@ const char *priv_spec_to_str(int priv_version)
> > > return PRIV_VER_1_11_0_STR;
> > > case PRIV_VERSION_1_12_0:
> > > return PRIV_VER_1_12_0_STR;
> > > +case PRIV_VERSION_1_13_0:
> > > +return PRIV_VER_1_13_0_STR;
> > > default:
> > > return NULL;
> > > }
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index 140eb43fcb..f691c7d828 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -96,12 +96,14 @@ extern RISCVCPUProfile *riscv_profiles[];
> > > #define PRIV_VER_1_10_0_STR "v1.10.0"
> > > #define PRIV_VER_1_11_0_STR "v1.11.0"
> > > #define PRIV_VER_1_12_0_STR "v1.12.0"
> > > +#define PRIV_VER_1_13_0_STR "v1.13.0"
> > > enum {
> > > PRIV_VERSION_1_10_0 = 0,
> > > PRIV_VERSION_1_11_0,
> > > PRIV_VERSION_1_12_0,
> > > +PRIV_VERSION_1_13_0,
> > >
> > > -PRIV_VERSION_LATEST = PRIV_VERSION_1_12_0,
> > > +PRIV_VERSION_LATEST = PRIV_VERSION_1_13_0,
> > > };
> > >
> > > #define VEXT_VERSION_1_00_0 0x0001
> > > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> > > index e1e4f32698..fb7eebde52 100644
> > > --- a/target/riscv/cpu_cfg.h
> > > +++ b/target/riscv/cpu_cfg.h
> > > @@ -136,6 +136,7 @@ struct RISCVCPUConfig {
> > > * TCG always implement/can't be user disabled,
> > > * based on spec version.
> > > */
> > > +bool has_priv_1_13;
> > > bool has_priv_1_12;
> > > bool has_priv_1_11;
> > >
> > > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> > > index fa186093fb..f53422d605 100644
> > > --- a/target/riscv/tcg/tcg-cpu.c
> > > +++ b/target/riscv/tcg/tcg-cpu.c
> > > @@ -318,6 +318,10 @@ static void
> riscv_cpu_update_named_features(RISCVCPU *cpu)
> > > cpu->cfg.has_priv_1_12 = true;
> > > }
> > >
> > > +if (cpu->env.priv_ver >= PRIV_VERSION_1_13_0) {
> > > +cpu->cfg.has_priv_1_13 = true;
> > > +}
> > > +
> > > /* zic64b is 1.12 or later */
> > > cpu->cfg.ext_zic64b = cpu->cfg.cbom_blocksize == 64 &&
> > >cpu->cfg.cbop_blocksize == 64 &&
> > > --
> > > 2.34.1
> > >
> > >
>
Re: [RESEND PATCH v2 0/5] target/riscv: Support RISC-V privilege 1.13 spec
Hi Daniel, Andrew: Thank you for giving me the feedback. I found a new command set to cc the related maintainer's email. It also includes these emails for the cover letter. git send-email --dry-run --to='qemu-devel@nongnu.org,qemu-ri...@nongnu.org' --cc="$(scripts/get_maintainer.pl XXXPATCH/* |sed -e 's/$/,/')" /XXXPATCH/* Sincerely, Fea On Thu, May 30, 2024 at 6:37 PM Andrew Jones wrote: > On Thu, May 30, 2024 at 11:30:28AM GMT, Fea Wang wrote: > > Hi Daniel, > > thank you for your help. > > > > I found that only the cover is without many maintainers. I used to send > > patches by git send-email --dry-run --to 'qemu-devel@nongnu.org, > > qemu-ri...@nongnu.org' --cc-cmd='scripts/get_maintainer.pl -i' > patches/*. > > Do you have a better script for me? > > Thank you. > > Some maintainers frown on that because they prefer the whole series, > even if only a few patches touch code they maintain. And, the cover > letter is quite important to get the context. Also, depending on > mail client and filter configurations, CC's that are per patch can > mess up the threading, making a mess of the mailbox. I tend to > use scripts/get_maintainer.pl to pull together a set of people/lists > to CC as an independent step, dump the contents into a file and then > do 'git send-email $(cat my-series.to-list) patches/my-series' > > git-publish can help with that too by capturing the "to list" and > maintaining it for you. > > Thanks, > drew > > > > > Sincerely, > > Fea > > > > On Mon, May 27, 2024 at 5:21 PM Daniel Henrique Barboza < > > dbarb...@ventanamicro.com> wrote: > > > > > Fea, > > > > > > Please try to also add all RISC-V QEMU maintainers and reviewers when > > > sending > > > patches. It will get your patches reviewed and queued faster. > Otherwise the > > > maintainers can miss you your series due to high ML traffic. > > > > > > You can fetch who you want to CC using the get_maintainer.pl script > with > > > the > > > patch files or any source file in particular, e.g.: > > > > > > $ ./scripts/get_maintainer.pl -f target/riscv/cpu.c > > > Palmer Dabbelt (supporter:RISC-V TCG CPUs) > > > Alistair Francis (supporter:RISC-V TCG > CPUs) > > > Bin Meng (supporter:RISC-V TCG CPUs) > > > Weiwei Li (reviewer:RISC-V TCG CPUs) > > > Daniel Henrique Barboza (reviewer:RISC-V > TCG > > > CPUs) > > > Liu Zhiwei (reviewer:RISC-V TCG CPUs) > > > qemu-ri...@nongnu.org (open list:RISC-V TCG CPUs) > > > qemu-devel@nongnu.org (open list:All patches CC here) > > > > > > > > > I added the extra folk in the CC for this reply so don't worry about > it. > > > > > > > > > Alistair, please queue this series. It's already fully acked and I > would > > > like to add > > > some bits on top of the priv_spec 1.13 support. > > > > > > > > > Thanks, > > > > > > > > > Daniel > > > > > > On 5/15/24 05:05, Fea.Wang wrote: > > > > Based on the change log for the RISC-V privilege 1.13 spec, add the > > > > support for ss1p13. > > > > > > > > Ref: > > > > https://github.com/riscv/riscv-isa-manual/blob/a7d93c9/src/priv-preface.adoc?plain=1#L40-L72 > > > > > > > > Lists what to do without clarification or document format. > > > > * Redefined misa.MXL to be read-only, making MXLEN a constant.(Skip, > > > implementation ignored) > > > > * Added the constraint that SXLEN≥UXLEN.(Skip, implementation > ignored) > > > > * Defined the misa.V field to reflect that the V extension has been > > > implemented.(Skip, existed) > > > > * Defined the RV32-only medelegh and hedelegh CSRs.(Done in these > > > patches) > > > > * Defined the misaligned atomicity granule PMA, superseding the > proposed > > > Zam extension..(Skip, implementation ignored) > > > > * Allocated interrupt 13 for Sscofpmf LCOFI interrupt.(Skip, existed) > > > > * Defined hardware error and software check exception codes.(Done in > > > these patches) > > > > * Specified synchronization requirements when changing the PBMTE > fields > > > in menvcfg and henvcfg.(Skip, implementation ignored) > > > > * Incorporated Svade and Svadu extension specifications.(Skip, > existed) > > > > > > > > > > > > Fea.Wang (4): > > > >target/riscv: Support the version for ss1p13 > > > >target/riscv: Add 'P1P13' bit in SMSTATEEN0 > > > >target/riscv: Add MEDELEGH, HEDELEGH csrs for RV32 > > > >target/riscv: Reserve exception codes for sw-check and hw-err > > > > > > > > Jim Shu (1): > > > >target/riscv: Reuse the conversion function of priv_spec > > > > > > > > target/riscv/cpu.c | 8 ++-- > > > > target/riscv/cpu.h | 5 - > > > > target/riscv/cpu_bits.h| 5 + > > > > target/riscv/cpu_cfg.h | 1 + > > > > target/riscv/csr.c | 39 > ++ > > > > target/riscv/tcg/tcg-cpu.c | 17 - > > > > 6 files changed, 63 insertions(+), 12 deletions(-) > > > > > > > >
Re: [RESEND PATCH v2 0/5] target/riscv: Support RISC-V privilege 1.13 spec
Hi Daniel, thank you for your help. I found that only the cover is without many maintainers. I used to send patches by git send-email --dry-run --to 'qemu-devel@nongnu.org, qemu-ri...@nongnu.org' --cc-cmd='scripts/get_maintainer.pl -i' patches/*. Do you have a better script for me? Thank you. Sincerely, Fea On Mon, May 27, 2024 at 5:21 PM Daniel Henrique Barboza < dbarb...@ventanamicro.com> wrote: > Fea, > > Please try to also add all RISC-V QEMU maintainers and reviewers when > sending > patches. It will get your patches reviewed and queued faster. Otherwise the > maintainers can miss you your series due to high ML traffic. > > You can fetch who you want to CC using the get_maintainer.pl script with > the > patch files or any source file in particular, e.g.: > > $ ./scripts/get_maintainer.pl -f target/riscv/cpu.c > Palmer Dabbelt (supporter:RISC-V TCG CPUs) > Alistair Francis (supporter:RISC-V TCG CPUs) > Bin Meng (supporter:RISC-V TCG CPUs) > Weiwei Li (reviewer:RISC-V TCG CPUs) > Daniel Henrique Barboza (reviewer:RISC-V TCG > CPUs) > Liu Zhiwei (reviewer:RISC-V TCG CPUs) > qemu-ri...@nongnu.org (open list:RISC-V TCG CPUs) > qemu-devel@nongnu.org (open list:All patches CC here) > > > I added the extra folk in the CC for this reply so don't worry about it. > > > Alistair, please queue this series. It's already fully acked and I would > like to add > some bits on top of the priv_spec 1.13 support. > > > Thanks, > > > Daniel > > On 5/15/24 05:05, Fea.Wang wrote: > > Based on the change log for the RISC-V privilege 1.13 spec, add the > > support for ss1p13. > > > > Ref: > https://github.com/riscv/riscv-isa-manual/blob/a7d93c9/src/priv-preface.adoc?plain=1#L40-L72 > > > > Lists what to do without clarification or document format. > > * Redefined misa.MXL to be read-only, making MXLEN a constant.(Skip, > implementation ignored) > > * Added the constraint that SXLEN≥UXLEN.(Skip, implementation ignored) > > * Defined the misa.V field to reflect that the V extension has been > implemented.(Skip, existed) > > * Defined the RV32-only medelegh and hedelegh CSRs.(Done in these > patches) > > * Defined the misaligned atomicity granule PMA, superseding the proposed > Zam extension..(Skip, implementation ignored) > > * Allocated interrupt 13 for Sscofpmf LCOFI interrupt.(Skip, existed) > > * Defined hardware error and software check exception codes.(Done in > these patches) > > * Specified synchronization requirements when changing the PBMTE fields > in menvcfg and henvcfg.(Skip, implementation ignored) > > * Incorporated Svade and Svadu extension specifications.(Skip, existed) > > > > > > Fea.Wang (4): > >target/riscv: Support the version for ss1p13 > >target/riscv: Add 'P1P13' bit in SMSTATEEN0 > >target/riscv: Add MEDELEGH, HEDELEGH csrs for RV32 > >target/riscv: Reserve exception codes for sw-check and hw-err > > > > Jim Shu (1): > >target/riscv: Reuse the conversion function of priv_spec > > > > target/riscv/cpu.c | 8 ++-- > > target/riscv/cpu.h | 5 - > > target/riscv/cpu_bits.h| 5 + > > target/riscv/cpu_cfg.h | 1 + > > target/riscv/csr.c | 39 ++ > > target/riscv/tcg/tcg-cpu.c | 17 - > > 6 files changed, 63 insertions(+), 12 deletions(-) > > >
Re: [PATCH v2 0/5] target/riscv: Support RISC-V privilege 1.13 spec
Sorry that I only put the patch version on the cover letter. I will resend the patches. Sincerely, Fea Fea.Wang 於 2024年5月15日 週三 下午3:48寫道： > Based on the change log for the RISC-V privilege 1.13 spec, add the > support for ss1p13. > > Ref: > https://github.com/riscv/riscv-isa-manual/blob/a7d93c9/src/priv-preface.adoc?plain=1#L40-L72 > > Lists what to do without clarification or document format. > * Redefined misa.MXL to be read-only, making MXLEN a constant.(Skip, > implementation ignored) > * Added the constraint that SXLEN≥UXLEN.(Skip, implementation ignored) > * Defined the misa.V field to reflect that the V extension has been > implemented.(Skip, existed) > * Defined the RV32-only medelegh and hedelegh CSRs.(Done in these patches) > * Defined the misaligned atomicity granule PMA, superseding the proposed > Zam extension..(Skip, implementation ignored) > * Allocated interrupt 13 for Sscofpmf LCOFI interrupt.(Skip, existed) > * Defined hardware error and software check exception codes.(Done in these > patches) > * Specified synchronization requirements when changing the PBMTE fields in > menvcfg and henvcfg.(Skip, implementation ignored) > * Incorporated Svade and Svadu extension specifications.(Skip, existed) > > > Fea.Wang (4): > target/riscv: Support the version for ss1p13 > target/riscv: Add 'P1P13' bit in SMSTATEEN0 > target/riscv: Add MEDELEGH, HEDELEGH csrs for RV32 > target/riscv: Reserve exception codes for sw-check and hw-err > > Jim Shu (1): > target/riscv: Reuse the conversion function of priv_spec > > target/riscv/cpu.c | 8 ++-- > target/riscv/cpu.h | 5 - > target/riscv/cpu_bits.h| 5 + > target/riscv/cpu_cfg.h | 1 + > target/riscv/csr.c | 39 ++ > target/riscv/tcg/tcg-cpu.c | 17 - > 6 files changed, 63 insertions(+), 12 deletions(-) > > -- > 2.34.1 > >
Re: [PATCH 4/5] target/riscv: Add MEDELEGH, HEDELEGH csrs for RV32
OK, I will correct it in the patch v2. Thank you.
Sincerely,
Fea
LIU Zhiwei 於 2024年5月13日 週一 上午10:49寫道：
>
> On 2024/5/10 14:58, Fea.Wang wrote:
> > Based on privileged spec 1.13, the RV32 needs to implement MEDELEGH
> > and HEDELEGH for exception codes 32-47 for reserving and exception codes
> > 48-63 for custom use. Add the CSR number though the implementation is
> > just reading zero and writing ignore. Besides, for accessing HEDELEGH, it
> > should be controlled by mstateen0 'P1P13' bit.
> >
> > Signed-off-by: Fea.Wang
> > Reviewed-by: Frank Chang
> > ---
> > target/riscv/cpu_bits.h | 2 ++
> > target/riscv/csr.c | 31 +++
> > 2 files changed, 33 insertions(+)
> >
> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > index 28bd3fb0b4..f888025c59 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -156,6 +156,8 @@
> >
> > /* 32-bit only */
> > #define CSR_MSTATUSH0x310
> > +#define CSR_MEDELEGH0x312
> > +#define CSR_HEDELEGH0x612
> >
> > /* Machine Trap Handling */
> > #define CSR_MSCRATCH0x340
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index d844ce770e..4d7313f456 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -3227,6 +3227,33 @@ static RISCVException write_hedeleg(CPURISCVState
> *env, int csrno,
> > return RISCV_EXCP_NONE;
> > }
> >
> > +static RISCVException read_hedelegh(CPURISCVState *env, int csrno,
> > + target_ulong *val)
> > +{
> > +RISCVException ret;
> > +ret = smstateen_acc_ok(env, 0, SMSTATEEN0_P1P13);
> > +if (ret != RISCV_EXCP_NONE) {
> > +return ret;
> > +}
> > +
> > +/* Reserved, now read zero */
> > +*val = 0;
> > +return RISCV_EXCP_NONE;
> > +}
> > +
> > +static RISCVException write_hedelegh(CPURISCVState *env, int csrno,
> > +target_ulong val)
> > +{
> > +RISCVException ret;
> > +ret = smstateen_acc_ok(env, 0, SMSTATEEN0_P1P13);
> > +if (ret != RISCV_EXCP_NONE) {
> > +return ret;
> > +}
> > +
> > +/* Reserved, now write ignore */
> > +return RISCV_EXCP_NONE;
> > +}
> > +
> > static RISCVException rmw_hvien64(CPURISCVState *env, int csrno,
> > uint64_t *ret_val,
> > uint64_t new_val, uint64_t wr_mask)
> > @@ -4674,6 +4701,10 @@ riscv_csr_operations csr_ops[CSR_TABLE_SIZE] = {
> >
> > [CSR_MSTATUSH]= { "mstatush", any32, read_mstatush,
> > write_mstatush
> },
> > +[CSR_MEDELEGH]= { "medelegh", any32, read_zero, write_ignore,
> > + .min_priv_ver = PRIV_VERSION_1_13_0
> },
> > +[CSR_HEDELEGH]= { "hedelegh", any32, read_hedelegh,
> write_hedelegh,
>
> Using hmode32 instead of any32.
>
> Otherwise,
>
> Reviewed-by: LIU Zhiwei
>
> > + .min_priv_ver = PRIV_VERSION_1_13_0
> },
> >
> > /* Machine Trap Handling */
> > [CSR_MSCRATCH] = { "mscratch", any, read_mscratch, write_mscratch,
>
Re: [PATCH 3/5] target/riscv: Add 'P1P13' bit in SMSTATEEN0
Thank you, I will correct it in the patch v2.
Sincerely,
Fea
LIU Zhiwei 於 2024年5月13日 週一 上午10:51寫道：
>
> On 2024/5/10 14:58, Fea.Wang wrote:
> > Based on privilege 1.13 spec, there should be a bit56 for 'P1P13' in
> > SMSTATEEN0 that controls access to the hedeleg.
> >
> > Signed-off-by: Fea.Wang
> > Reviewed-by: Frank Chang
> > ---
> > target/riscv/cpu_bits.h | 1 +
> > target/riscv/csr.c | 10 ++
> > 2 files changed, 11 insertions(+)
> >
> > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > index 74318a925c..28bd3fb0b4 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -315,6 +315,7 @@
> > #define SMSTATEEN0_CS (1ULL << 0)
> > #define SMSTATEEN0_FCSR (1ULL << 1)
> > #define SMSTATEEN0_JVT (1ULL << 2)
> > +#define SMSTATEEN0_P1P13(1ULL << 56)
> > #define SMSTATEEN0_HSCONTXT (1ULL << 57)
> > #define SMSTATEEN0_IMSIC(1ULL << 58)
> > #define SMSTATEEN0_AIA (1ULL << 59)
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 6b460ee0e8..d844ce770e 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -2248,6 +2248,11 @@ static RISCVException
> write_mstateen0(CPURISCVState *env, int csrno,
> > wr_mask |= SMSTATEEN0_FCSR;
> > }
> >
> > +RISCVCPU *cpu = env_archcpu(env);
> > +if (cpu->env.priv_ver >= PRIV_VERSION_1_13_0) {
> Why not use env directly?
> > +wr_mask |= SMSTATEEN0_P1P13;
> > +}
> > +
> > return write_mstateen(env, csrno, wr_mask, new_val);
> > }
> >
> > @@ -2283,6 +2288,11 @@ static RISCVException
> write_mstateen0h(CPURISCVState *env, int csrno,
> > {
> > uint64_t wr_mask = SMSTATEEN_STATEEN | SMSTATEEN0_HSENVCFG;
> >
> > +RISCVCPU *cpu = env_archcpu(env);
> > +if (cpu->env.priv_ver >= PRIV_VERSION_1_13_0) {
> Same here.
> > +wr_mask |= SMSTATEEN0_P1P13;
>
> Indent.
>
> Zhiwei
>
> > +}
> > +
> > return write_mstateenh(env, csrno, wr_mask, new_val);
> > }
> >
>
Re: [PATCH 1/5] target/riscv: Reuse the conversion function of priv_spec and string
Thank you, I will correct it in the patch v2.
Sincerely,
Fea
LIU Zhiwei 於 2024年5月13日 週一 上午10:55寫道：
>
> On 2024/5/10 14:58, Fea.Wang wrote:
> > From: Jim Shu
> >
> > Public the conversion function of priv_spec and string in cpu.h, so that
> > tcg-cpu.c could also use it.
> >
> > Signed-off-by: Jim Shu
> > Signed-off-by: Fea.Wang
> > Reviewed-by: Frank Chang
> > ---
> > target/riscv/cpu.c | 4 ++--
> > target/riscv/cpu.h | 3 +++
> > target/riscv/tcg/tcg-cpu.c | 13 +
> > 3 files changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index a74f0eb29c..b6b48e5620 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -1769,7 +1769,7 @@ static const PropertyInfo prop_pmp = {
> > .set = prop_pmp_set,
> > };
> >
> > -static int priv_spec_from_str(const char *priv_spec_str)
> > +int priv_spec_from_str(const char *priv_spec_str)
> > {
> > int priv_version = -1;
> >
> > @@ -1784,7 +1784,7 @@ static int priv_spec_from_str(const char
> *priv_spec_str)
> > return priv_version;
> > }
> >
> > -static const char *priv_spec_to_str(int priv_version)
> > +const char *priv_spec_to_str(int priv_version)
> > {
> > switch (priv_version) {
> > case PRIV_VERSION_1_10_0:
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index e0dd1828b5..7696102697 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -829,4 +829,7 @@ target_ulong riscv_new_csr_seed(target_ulong
> new_value,
> > uint8_t satp_mode_max_from_map(uint32_t map);
> > const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit);
> >
> > +const char *priv_spec_to_str(int priv_version);
> > +int priv_spec_from_str(const char *priv_spec_str);
> > +
> > #endif /* RISCV_CPU_H */
> > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> > index 4ebebebe09..faa8de9b83 100644
> > --- a/target/riscv/tcg/tcg-cpu.c
> > +++ b/target/riscv/tcg/tcg-cpu.c
> > @@ -76,16 +76,13 @@ static void riscv_cpu_write_misa_bit(RISCVCPU *cpu,
> uint32_t bit,
> >
> > static const char *cpu_priv_ver_to_str(int priv_ver)
> > {
> > -switch (priv_ver) {
> > -case PRIV_VERSION_1_10_0:
> > -return "v1.10.0";
> > -case PRIV_VERSION_1_11_0:
> > -return "v1.11.0";
> > -case PRIV_VERSION_1_12_0:
> > -return "v1.12.0";
> > +const char *priv_spec_str = priv_spec_to_str(priv_ver);
> > +
> > +if (priv_spec_str == NULL) {
> > +g_assert_not_reached();
> > }
>
> g_assert(priv_spec_str != NULL) or g_assert(priv_spec_str)
>
> Otherwise,
>
> Reviewed-by: LIU Zhiwei
>
> Zhiwei
>
> >
> > -g_assert_not_reached();
> > +return priv_spec_str;
> > }
> >
> > static void riscv_cpu_synchronize_from_tb(CPUState *cs,
>
