[PATCH] disas/riscv.c: rvv: Add disas support for vector instructions
Tested with https://github.com/ksco/rvv-decoder-tests
Expected checkpatch errors for consistency and brevity reasons:
ERROR: line over 90 characters
ERROR: trailing statements should be on next line
ERROR: braces {} are necessary for all arms of this statement
Signed-off-by: Yang Liu
---
disas/riscv.c | 1430 -
1 file changed, 1428 insertions(+), 2 deletions(-)
diff --git a/disas/riscv.c b/disas/riscv.c
index 7af6afc8fa..e313e877f0 100644
--- a/disas/riscv.c
+++ b/disas/riscv.c
@@ -158,6 +158,11 @@ typedef enum {
rv_codec_css_sqsp,
rv_codec_k_bs,
rv_codec_k_rnum,
+rv_codec_v_r,
+rv_codec_v_ldst,
+rv_codec_v_i,
+rv_codec_vsetvli,
+rv_codec_vsetivli,
} rv_codec;
typedef enum {
@@ -560,6 +565,376 @@ typedef enum {
rv_op_zip = 396,
rv_op_xperm4 = 397,
rv_op_xperm8 = 398,
+rv_op_vle8_v = 399,
+rv_op_vle16_v = 400,
+rv_op_vle32_v = 401,
+rv_op_vle64_v = 402,
+rv_op_vse8_v = 403,
+rv_op_vse16_v = 404,
+rv_op_vse32_v = 405,
+rv_op_vse64_v = 406,
+rv_op_vlm_v = 407,
+rv_op_vsm_v = 408,
+rv_op_vlse8_v = 409,
+rv_op_vlse16_v = 410,
+rv_op_vlse32_v = 411,
+rv_op_vlse64_v = 412,
+rv_op_vsse8_v = 413,
+rv_op_vsse16_v = 414,
+rv_op_vsse32_v = 415,
+rv_op_vsse64_v = 416,
+rv_op_vluxei8_v = 417,
+rv_op_vluxei16_v = 418,
+rv_op_vluxei32_v = 419,
+rv_op_vluxei64_v = 420,
+rv_op_vloxei8_v = 421,
+rv_op_vloxei16_v = 422,
+rv_op_vloxei32_v = 423,
+rv_op_vloxei64_v = 424,
+rv_op_vsuxei8_v = 425,
+rv_op_vsuxei16_v = 426,
+rv_op_vsuxei32_v = 427,
+rv_op_vsuxei64_v = 428,
+rv_op_vsoxei8_v = 429,
+rv_op_vsoxei16_v = 430,
+rv_op_vsoxei32_v = 431,
+rv_op_vsoxei64_v = 432,
+rv_op_vle8ff_v = 433,
+rv_op_vle16ff_v = 434,
+rv_op_vle32ff_v = 435,
+rv_op_vle64ff_v = 436,
+rv_op_vl1re8_v = 437,
+rv_op_vl1re16_v = 438,
+rv_op_vl1re32_v = 439,
+rv_op_vl1re64_v = 440,
+rv_op_vl2re8_v = 441,
+rv_op_vl2re16_v = 442,
+rv_op_vl2re32_v = 443,
+rv_op_vl2re64_v = 444,
+rv_op_vl4re8_v = 445,
+rv_op_vl4re16_v = 446,
+rv_op_vl4re32_v = 447,
+rv_op_vl4re64_v = 448,
+rv_op_vl8re8_v = 449,
+rv_op_vl8re16_v = 450,
+rv_op_vl8re32_v = 451,
+rv_op_vl8re64_v = 452,
+rv_op_vs1r_v = 453,
+rv_op_vs2r_v = 454,
+rv_op_vs4r_v = 455,
+rv_op_vs8r_v = 456,
+rv_op_vadd_vv = 457,
+rv_op_vadd_vx = 458,
+rv_op_vadd_vi = 459,
+rv_op_vsub_vv = 460,
+rv_op_vsub_vx = 461,
+rv_op_vrsub_vx = 462,
+rv_op_vrsub_vi = 463,
+rv_op_vwaddu_vv = 464,
+rv_op_vwaddu_vx = 465,
+rv_op_vwadd_vv = 466,
+rv_op_vwadd_vx = 467,
+rv_op_vwsubu_vv = 468,
+rv_op_vwsubu_vx = 469,
+rv_op_vwsub_vv = 470,
+rv_op_vwsub_vx = 471,
+rv_op_vwaddu_wv = 472,
+rv_op_vwaddu_wx = 473,
+rv_op_vwadd_wv = 474,
+rv_op_vwadd_wx = 475,
+rv_op_vwsubu_wv = 476,
+rv_op_vwsubu_wx = 477,
+rv_op_vwsub_wv = 478,
+rv_op_vwsub_wx = 479,
+rv_op_vadc_vvm = 480,
+rv_op_vadc_vxm = 481,
+rv_op_vadc_vim = 482,
+rv_op_vmadc_vvm = 483,
+rv_op_vmadc_vxm = 484,
+rv_op_vmadc_vim = 485,
+rv_op_vsbc_vvm = 486,
+rv_op_vsbc_vxm = 487,
+rv_op_vmsbc_vvm = 488,
+rv_op_vmsbc_vxm = 489,
+rv_op_vand_vv = 490,
+rv_op_vand_vx = 491,
+rv_op_vand_vi = 492,
+rv_op_vor_vv = 493,
+rv_op_vor_vx = 494,
+rv_op_vor_vi = 495,
+rv_op_vxor_vv = 496,
+rv_op_vxor_vx = 497,
+rv_op_vxor_vi = 498,
+rv_op_vsll_vv = 499,
+rv_op_vsll_vx = 500,
+rv_op_vsll_vi = 501,
+rv_op_vsrl_vv = 502,
+rv_op_vsrl_vx = 503,
+rv_op_vsrl_vi = 504,
+rv_op_vsra_vv = 505,
+rv_op_vsra_vx = 506,
+rv_op_vsra_vi = 507,
+rv_op_vnsrl_wv = 508,
+rv_op_vnsrl_wx = 509,
+rv_op_vnsrl_wi = 510,
+rv_op_vnsra_wv = 511,
+rv_op_vnsra_wx = 512,
+rv_op_vnsra_wi = 513,
+rv_op_vmseq_vv = 514,
+rv_op_vmseq_vx = 515,
+rv_op_vmseq_vi = 516,
+rv_op_vmsne_vv = 517,
+rv_op_vmsne_vx = 518,
+rv_op_vmsne_vi = 519,
+rv_op_vmsltu_vv = 520,
+rv_op_vmsltu_vx = 521,
+rv_op_vmslt_vv = 522,
+rv_op_vmslt_vx = 523,
+rv_op_vmsleu_vv = 524,
+rv_op_vmsleu_vx = 525,
+rv_op_vmsleu_vi = 526,
+rv_op_vmsle_vv = 527,
+rv_op_vmsle_vx = 528,
+rv_op_vmsle_vi = 529,
+rv_op_vmsgtu_vx = 530,
+rv_op_vmsgtu_vi = 531,
+rv_op_vmsgt_vx = 532,
+rv_op_vmsgt_vi = 533,
+rv_op_vminu_vv = 534,
+rv_op_vminu_vx = 535,
+rv_op_vmin_vv = 536,
+rv_op_vmin_vx = 537,
+rv_op_vmaxu_vv = 538,
+rv_op_vmaxu_vx = 539,
+rv_op_vmax_vv = 540,
+rv_op_vmax_vx = 541,
+rv_op_vmul_vv = 542,
+rv_op_vmul_vx = 543,
+rv_op_vmulh_vv = 544,
+rv_op_vmulh_vx = 545,
+rv_op_vmulhu_vv = 546,
+rv_op_vmulhu_vx = 547,
+rv_op_vmulhsu_vv = 548,
+rv_op_vmulhsu_vx = 549,
+rv_op_vdivu_vv = 550
Re: [PATCH v7 00/14] KVM: mm: fd-based approach for supporting KVM guest private memory
On Thu, Aug 18, 2022 at 08:00:41PM -0700, Hugh Dickins wrote: > tmpfs and hugetlbfs and page cache are designed around sharing memory: > TDX is designed around absolutely not sharing memory; and the further > uses which Sean foresees appear not to need it as page cache either. > > Except perhaps for page migration reasons. It's somewhat incidental, > but of course page migration knows how to migrate page cache, so > masquerading as page cache will give a short cut to page migration, > when page migration becomes at all possible. I haven't read the patch series, and I'm not taking a position one way or the other on whether this is better implemented as a shmem addition or a shim that asks shmem for memory. Page migration can be done for driver memory by using PageMovable. I just rewrote how it works, so the details are top of my mind at the moment if anyone wants something explained. Commit 68f2736a8583 is the key one to look at.
Re: [PATCH 1/2] scsi: Add buf_len parameter to scsi_req_new()
Thank you for the suggestions for CDB sizes! Especially the tricky ones
in spapr_vscsi.c and dev-uas.c.
v2: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02997.html
On Fri, Aug 19, 2022 at 06:06:13PM +0200, Paolo Bonzini wrote:
> On 8/17/22 07:34, John Millikin wrote:
> > The sigil SCSI_CMD_BUF_LEN_TODO() is used to indicate that the buffer
> > length calculation is TODO it should be replaced by a better value,
> > such as the length of a successful DMA read.
>
> Let's just do it right:
>
> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> index e8a4a705e7..05a43ec807 100644
> --- a/hw/scsi/lsi53c895a.c
> +++ b/hw/scsi/lsi53c895a.c
> @@ -864,7 +864,7 @@ static void lsi_do_command(LSIState *s)
> s->current = g_new0(lsi_request, 1);
> s->current->tag = s->select_tag;
> s->current->req = scsi_req_new(dev, s->current->tag, s->current_lun, buf,
> - SCSI_CMD_BUF_LEN_TODO(s->dbc),
> s->current);
> + s->dbc, s->current);
> n = scsi_req_enqueue(s->current->req);
> if (n) {
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index e887ae8adb..842edc3f9d 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -1053,7 +1053,6 @@ static int megasas_pd_get_info_submit(SCSIDevice *sdev,
> int lun,
> uint64_t pd_size;
> uint16_t pd_id = ((sdev->id & 0xFF) << 8) | (lun & 0xFF);
> uint8_t cmdbuf[6];
> -size_t cmdbuf_len = SCSI_CMD_BUF_LEN_TODO(sizeof(cmdbuf));
> size_t len;
> dma_addr_t residual;
> @@ -1063,7 +1062,7 @@ static int megasas_pd_get_info_submit(SCSIDevice *sdev,
> int lun,
> info->inquiry_data[0] = 0x7f; /* Force PQual 0x3, PType 0x1f */
> info->vpd_page83[0] = 0x7f;
> megasas_setup_inquiry(cmdbuf, 0, sizeof(info->inquiry_data));
> -cmd->req = scsi_req_new(sdev, cmd->index, lun, cmdbuf, cmdbuf_len,
> cmd);
> +cmd->req = scsi_req_new(sdev, cmd->index, lun, cmdbuf,
> sizeof(cmdbuf), cmd);
> if (!cmd->req) {
> trace_megasas_dcmd_req_alloc_failed(cmd->index,
> "PD get info std inquiry");
> @@ -1081,7 +1080,7 @@ static int megasas_pd_get_info_submit(SCSIDevice *sdev,
> int lun,
> return MFI_STAT_INVALID_STATUS;
> } else if (info->inquiry_data[0] != 0x7f && info->vpd_page83[0] == 0x7f)
> {
> megasas_setup_inquiry(cmdbuf, 0x83, sizeof(info->vpd_page83));
> -cmd->req = scsi_req_new(sdev, cmd->index, lun, cmdbuf, cmdbuf_len,
> cmd);
> +cmd->req = scsi_req_new(sdev, cmd->index, lun, cmdbuf,
> sizeof(cmdbuf), cmd);
> if (!cmd->req) {
> trace_megasas_dcmd_req_alloc_failed(cmd->index,
> "PD get info vpd inquiry");
> diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
> index 711613b5b1..a90c2546f1 100644
> --- a/hw/scsi/mptsas.c
> +++ b/hw/scsi/mptsas.c
> @@ -324,8 +324,8 @@ static int mptsas_process_scsi_io_request(MPTSASState *s,
> }
> req->sreq = scsi_req_new(sdev, scsi_io->MsgContext,
> -scsi_io->LUN[1], scsi_io->CDB,
> -SCSI_CMD_BUF_LEN_TODO(SIZE_MAX), req);
> + scsi_io->LUN[1], scsi_io->CDB,
> + scsi_io->CDBLength, req);
> if (req->sreq->cmd.xfer > scsi_io->DataLength) {
> goto overrun;
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 288ea12969..cc71524024 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -1707,7 +1707,6 @@ static int get_scsi_requests(QEMUFile *f, void *pv,
> size_t size,
> while ((sbyte = qemu_get_sbyte(f)) > 0) {
> uint8_t buf[SCSI_CMD_BUF_SIZE];
> -size_t buf_len;
> uint32_t tag;
> uint32_t lun;
> SCSIRequest *req;
> @@ -1715,8 +1714,11 @@ static int get_scsi_requests(QEMUFile *f, void *pv,
> size_t size,
> qemu_get_buffer(f, buf, sizeof(buf));
> qemu_get_be32s(f, &tag);
> qemu_get_be32s(f, &lun);
> -buf_len = SCSI_CMD_BUF_LEN_TODO(sizeof(buf));
> -req = scsi_req_new(s, tag, lun, buf, buf_len, NULL);
> +/*
> + * A too-short CDB would have been rejected by scsi_req_new, so just
> use
> + * SCSI_CMD_BUF_SIZE as the CDB length.
> + */
> +req = scsi_req_new(s, tag, lun, buf, sizeof(buf), NULL);
> req->retry = (sbyte == 1);
> if (bus->info->load_request) {
> req->hba_private = bus->info->load_request(f, req);
> diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> index c17bb47f7b..0a8cbf5a4b 100644
> --- a/hw/scsi/spapr_vscsi.c
> +++ b/hw/scsi/spapr_vscsi.c
> @@ -783,7 +783,7 @@ static int vscsi_queue_cmd(VSCSIState *s, vscsi_req *req)
> union srp_iu *srp = &req_iu(req)->srp;
> SCSIDevice *sdev;
> int n, lun;
> -size_t cdb_len = SCSI_CMD_BUF_LEN_TODO(SIZE_MAX);
> +s
[PATCH] net: tulip: Restrict DMA engine to memories
The DMA engine is started by I/O access and then itself accesses the
I/O registers, triggering a reentrancy bug.
The following log can reveal it:
==5637==ERROR: AddressSanitizer: stack-overflow
#0 0x5595435f6078 in tulip_xmit_list_update qemu/hw/net/tulip.c:673
#1 0x5595435f204a in tulip_write qemu/hw/net/tulip.c:805:13
#2 0x559544637f86 in memory_region_write_accessor
qemu/softmmu/memory.c:492:5
#3 0x5595446379fa in access_with_adjusted_size qemu/softmmu/memory.c:554:18
#4 0x5595446372fa in memory_region_dispatch_write qemu/softmmu/memory.c
#5 0x55954468b74c in flatview_write_continue qemu/softmmu/physmem.c:2825:23
#6 0x559544683662 in flatview_write qemu/softmmu/physmem.c:2867:12
#7 0x5595446833f3 in address_space_write qemu/softmmu/physmem.c:2963:18
#8 0x5595435fb082 in dma_memory_rw_relaxed
/home/mzy/truman/third_party/qemu/include/sysemu/dma.h:87:12
#9 0x5595435fb082 in dma_memory_rw
/home/mzy/truman/third_party/qemu/include/sysemu/dma.h:130:12
#10 0x5595435fb082 in dma_memory_write
/home/mzy/truman/third_party/qemu/include/sysemu/dma.h:171:12
#11 0x5595435fb082 in stl_le_dma
/home/mzy/truman/third_party/qemu/include/sysemu/dma.h:272:1
#12 0x5595435fb082 in stl_le_pci_dma
/home/mzy/truman/third_party/qemu/include/hw/pci/pci.h:910:1
#13 0x5595435fb082 in tulip_desc_write qemu/hw/net/tulip.c:101:9
#14 0x5595435f7e3d in tulip_xmit_list_update qemu/hw/net/tulip.c:706:9
#15 0x5595435f204a in tulip_write qemu/hw/net/tulip.c:805:13
Fix this bug by restricting the DMA engine to memories regions.
Signed-off-by: Zheyu Ma
---
hw/net/tulip.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/net/tulip.c b/hw/net/tulip.c
index 097e905bec..b9e42c322a 100644
--- a/hw/net/tulip.c
+++ b/hw/net/tulip.c
@@ -70,7 +70,7 @@ static const VMStateDescription vmstate_pci_tulip = {
static void tulip_desc_read(TULIPState *s, hwaddr p,
struct tulip_descriptor *desc)
{
-const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
+const MemTxAttrs attrs = { .memory = true };
if (s->csr[0] & CSR0_DBO) {
ldl_be_pci_dma(&s->dev, p, &desc->status, attrs);
@@ -88,7 +88,7 @@ static void tulip_desc_read(TULIPState *s, hwaddr p,
static void tulip_desc_write(TULIPState *s, hwaddr p,
struct tulip_descriptor *desc)
{
-const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
+const MemTxAttrs attrs = { .memory = true };
if (s->csr[0] & CSR0_DBO) {
stl_be_pci_dma(&s->dev, p, desc->status, attrs);
--
2.25.1
[PATCH v2] net: tulip: Restrict DMA engine to memories
The DMA engine is started by I/O access and then itself accesses the
I/O registers, triggering a reentrancy bug.
The following log can reveal it:
==5637==ERROR: AddressSanitizer: stack-overflow
#0 0x5595435f6078 in tulip_xmit_list_update qemu/hw/net/tulip.c:673
#1 0x5595435f204a in tulip_write qemu/hw/net/tulip.c:805:13
#2 0x559544637f86 in memory_region_write_accessor
qemu/softmmu/memory.c:492:5
#3 0x5595446379fa in access_with_adjusted_size qemu/softmmu/memory.c:554:18
#4 0x5595446372fa in memory_region_dispatch_write qemu/softmmu/memory.c
#5 0x55954468b74c in flatview_write_continue qemu/softmmu/physmem.c:2825:23
#6 0x559544683662 in flatview_write qemu/softmmu/physmem.c:2867:12
#7 0x5595446833f3 in address_space_write qemu/softmmu/physmem.c:2963:18
#8 0x5595435fb082 in dma_memory_rw_relaxed qemu/include/sysemu/dma.h:87:12
#9 0x5595435fb082 in dma_memory_rw qemu/include/sysemu/dma.h:130:12
#10 0x5595435fb082 in dma_memory_write qemu/include/sysemu/dma.h:171:12
#11 0x5595435fb082 in stl_le_dma qemu/include/sysemu/dma.h:272:1
#12 0x5595435fb082 in stl_le_pci_dma qemu/include/hw/pci/pci.h:910:1
#13 0x5595435fb082 in tulip_desc_write qemu/hw/net/tulip.c:101:9
#14 0x5595435f7e3d in tulip_xmit_list_update qemu/hw/net/tulip.c:706:9
#15 0x5595435f204a in tulip_write qemu/hw/net/tulip.c:805:13
Fix this bug by restricting the DMA engine to memories regions.
Signed-off-by: Zheyu Ma
---
Changes in v2:
- Remove irrelevant relative paths in the asan log
---
hw/net/tulip.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/net/tulip.c b/hw/net/tulip.c
index 097e905bec..b9e42c322a 100644
--- a/hw/net/tulip.c
+++ b/hw/net/tulip.c
@@ -70,7 +70,7 @@ static const VMStateDescription vmstate_pci_tulip = {
static void tulip_desc_read(TULIPState *s, hwaddr p,
struct tulip_descriptor *desc)
{
-const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
+const MemTxAttrs attrs = { .memory = true };
if (s->csr[0] & CSR0_DBO) {
ldl_be_pci_dma(&s->dev, p, &desc->status, attrs);
@@ -88,7 +88,7 @@ static void tulip_desc_read(TULIPState *s, hwaddr p,
static void tulip_desc_write(TULIPState *s, hwaddr p,
struct tulip_descriptor *desc)
{
-const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
+const MemTxAttrs attrs = { .memory = true };
if (s->csr[0] & CSR0_DBO) {
stl_be_pci_dma(&s->dev, p, desc->status, attrs);
--
2.25.1
Re: [PATCH v6 07/21] accel/tcg: Introduce is_same_page()
On Fri, Aug 19, 2022 at 1:26 PM Richard Henderson
wrote:
>
> From: Ilya Leoshkevich
>
> Introduce a function that checks whether a given address is on the same
> page as where disassembly started. Having it improves readability of
> the following patches.
>
> Signed-off-by: Ilya Leoshkevich
> Message-Id: <20220811095534.241224-3-...@linux.ibm.com>
> Reviewed-by: Richard Henderson
> [rth: Make the DisasContextBase parameter const.]
> Signed-off-by: Richard Henderson
Reviewed-by: Alistair Francis
Alistair
> ---
> include/exec/translator.h | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/include/exec/translator.h b/include/exec/translator.h
> index 7db6845535..0d0bf3a31e 100644
> --- a/include/exec/translator.h
> +++ b/include/exec/translator.h
> @@ -187,4 +187,14 @@ FOR_EACH_TRANSLATOR_LD(GEN_TRANSLATOR_LD)
>
> #undef GEN_TRANSLATOR_LD
>
> +/*
> + * Return whether addr is on the same page as where disassembly started.
> + * Translators can use this to enforce the rule that only single-insn
> + * translation blocks are allowed to cross page boundaries.
> + */
> +static inline bool is_same_page(const DisasContextBase *db, target_ulong
> addr)
> +{
> +return ((addr ^ db->pc_first) & TARGET_PAGE_MASK) == 0;
> +}
> +
> #endif /* EXEC__TRANSLATOR_H */
> --
> 2.34.1
>
>
Re: [PATCH v6 09/21] accel/tcg: Unlock mmap_lock after longjmp
On Fri, Aug 19, 2022 at 1:29 PM Richard Henderson
wrote:
>
> The mmap_lock is held around tb_gen_code. While the comment
> is correct that the lock is dropped when tb_gen_code runs out
> of memory, the lock is *not* dropped when an exception is
> raised reading code for translation.
>
> Signed-off-by: Richard Henderson
Acked-by: Alistair Francis
Alistair
> ---
> accel/tcg/cpu-exec.c | 12 ++--
> accel/tcg/user-exec.c | 3 ---
> 2 files changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index a565a3f8ec..d18081ca6f 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -462,13 +462,11 @@ void cpu_exec_step_atomic(CPUState *cpu)
> cpu_tb_exec(cpu, tb, &tb_exit);
> cpu_exec_exit(cpu);
> } else {
> -/*
> - * The mmap_lock is dropped by tb_gen_code if it runs out of
> - * memory.
> - */
> #ifndef CONFIG_SOFTMMU
> clear_helper_retaddr();
> -tcg_debug_assert(!have_mmap_lock());
> +if (have_mmap_lock()) {
> +mmap_unlock();
> +}
> #endif
> if (qemu_mutex_iothread_locked()) {
> qemu_mutex_unlock_iothread();
> @@ -936,7 +934,9 @@ int cpu_exec(CPUState *cpu)
>
> #ifndef CONFIG_SOFTMMU
> clear_helper_retaddr();
> -tcg_debug_assert(!have_mmap_lock());
> +if (have_mmap_lock()) {
> +mmap_unlock();
> +}
> #endif
> if (qemu_mutex_iothread_locked()) {
> qemu_mutex_unlock_iothread();
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index a20234fb02..58edd33896 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -80,10 +80,7 @@ MMUAccessType adjust_signal_pc(uintptr_t *pc, bool
> is_write)
> * (and if the translator doesn't handle page boundaries correctly
> * there's little we can do about that here). Therefore, do not
> * trigger the unwinder.
> - *
> - * Like tb_gen_code, release the memory lock before cpu_loop_exit.
> */
> -mmap_unlock();
> *pc = 0;
> return MMU_INST_FETCH;
> }
> --
> 2.34.1
>
>
Re: [PATCH v6 10/21] accel/tcg: Make tb_htable_lookup static
On Fri, Aug 19, 2022 at 1:33 PM Richard Henderson
wrote:
>
> The function is not used outside of cpu-exec.c. Move it and
> its subroutines up in the file, before the first use.
>
> Signed-off-by: Richard Henderson
Reviewed-by: Alistair Francis
Alistair
> ---
> include/exec/exec-all.h | 3 -
> accel/tcg/cpu-exec.c| 122
> 2 files changed, 61 insertions(+), 64 deletions(-)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 0475ec6007..9f35e3b7a9 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -552,9 +552,6 @@ void tb_invalidate_phys_addr(AddressSpace *as, hwaddr
> addr, MemTxAttrs attrs);
> #endif
> void tb_flush(CPUState *cpu);
> void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr);
> -TranslationBlock *tb_htable_lookup(CPUState *cpu, target_ulong pc,
> - target_ulong cs_base, uint32_t flags,
> - uint32_t cflags);
> void tb_set_jmp_target(TranslationBlock *tb, int n, uintptr_t addr);
>
> /* GETPC is the true target of the return instruction that we'll execute. */
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index d18081ca6f..7887af6f45 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -170,6 +170,67 @@ uint32_t curr_cflags(CPUState *cpu)
> return cflags;
> }
>
> +struct tb_desc {
> +target_ulong pc;
> +target_ulong cs_base;
> +CPUArchState *env;
> +tb_page_addr_t phys_page1;
> +uint32_t flags;
> +uint32_t cflags;
> +uint32_t trace_vcpu_dstate;
> +};
> +
> +static bool tb_lookup_cmp(const void *p, const void *d)
> +{
> +const TranslationBlock *tb = p;
> +const struct tb_desc *desc = d;
> +
> +if (tb->pc == desc->pc &&
> +tb->page_addr[0] == desc->phys_page1 &&
> +tb->cs_base == desc->cs_base &&
> +tb->flags == desc->flags &&
> +tb->trace_vcpu_dstate == desc->trace_vcpu_dstate &&
> +tb_cflags(tb) == desc->cflags) {
> +/* check next page if needed */
> +if (tb->page_addr[1] == -1) {
> +return true;
> +} else {
> +tb_page_addr_t phys_page2;
> +target_ulong virt_page2;
> +
> +virt_page2 = (desc->pc & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE;
> +phys_page2 = get_page_addr_code(desc->env, virt_page2);
> +if (tb->page_addr[1] == phys_page2) {
> +return true;
> +}
> +}
> +}
> +return false;
> +}
> +
> +static TranslationBlock *tb_htable_lookup(CPUState *cpu, target_ulong pc,
> + target_ulong cs_base, uint32_t
> flags,
> + uint32_t cflags)
> +{
> +tb_page_addr_t phys_pc;
> +struct tb_desc desc;
> +uint32_t h;
> +
> +desc.env = cpu->env_ptr;
> +desc.cs_base = cs_base;
> +desc.flags = flags;
> +desc.cflags = cflags;
> +desc.trace_vcpu_dstate = *cpu->trace_dstate;
> +desc.pc = pc;
> +phys_pc = get_page_addr_code(desc.env, pc);
> +if (phys_pc == -1) {
> +return NULL;
> +}
> +desc.phys_page1 = phys_pc & TARGET_PAGE_MASK;
> +h = tb_hash_func(phys_pc, pc, flags, cflags, *cpu->trace_dstate);
> +return qht_lookup_custom(&tb_ctx.htable, &desc, h, tb_lookup_cmp);
> +}
> +
> /* Might cause an exception, so have a longjmp destination ready */
> static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc,
>target_ulong cs_base,
> @@ -485,67 +546,6 @@ void cpu_exec_step_atomic(CPUState *cpu)
> end_exclusive();
> }
>
> -struct tb_desc {
> -target_ulong pc;
> -target_ulong cs_base;
> -CPUArchState *env;
> -tb_page_addr_t phys_page1;
> -uint32_t flags;
> -uint32_t cflags;
> -uint32_t trace_vcpu_dstate;
> -};
> -
> -static bool tb_lookup_cmp(const void *p, const void *d)
> -{
> -const TranslationBlock *tb = p;
> -const struct tb_desc *desc = d;
> -
> -if (tb->pc == desc->pc &&
> -tb->page_addr[0] == desc->phys_page1 &&
> -tb->cs_base == desc->cs_base &&
> -tb->flags == desc->flags &&
> -tb->trace_vcpu_dstate == desc->trace_vcpu_dstate &&
> -tb_cflags(tb) == desc->cflags) {
> -/* check next page if needed */
> -if (tb->page_addr[1] == -1) {
> -return true;
> -} else {
> -tb_page_addr_t phys_page2;
> -target_ulong virt_page2;
> -
> -virt_page2 = (desc->pc & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE;
> -phys_page2 = get_page_addr_code(desc->env, virt_page2);
> -if (tb->page_addr[1] == phys_page2) {
> -return true;
> -}
> -}
> -}
> -return false;
> -}
> -
> -TranslationBlock *tb_htable_lookup(CPUState *cpu, target_ulong pc,
> - targe
Re: [PATCH v6 11/21] accel/tcg: Move qemu_ram_addr_from_host_nofail to physmem.c
On Fri, Aug 19, 2022 at 1:34 PM Richard Henderson
wrote:
>
> The base qemu_ram_addr_from_host function is already in
> softmmu/physmem.c; move the nofail version to be adjacent.
>
> Signed-off-by: Richard Henderson
Reviewed-by: Alistair Francis
Alistair
> ---
> include/exec/cpu-common.h | 1 +
> accel/tcg/cputlb.c| 12
> softmmu/physmem.c | 12
> 3 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index 2281be4e10..d909429427 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -72,6 +72,7 @@ typedef uintptr_t ram_addr_t;
> void qemu_ram_remap(ram_addr_t addr, ram_addr_t length);
> /* This should not be used by devices. */
> ram_addr_t qemu_ram_addr_from_host(void *ptr);
> +ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr);
> RAMBlock *qemu_ram_block_by_name(const char *name);
> RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset,
> ram_addr_t *offset);
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 43bd65c973..80a3eb4f1c 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1283,18 +1283,6 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
> prot, mmu_idx, size);
> }
>
> -static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
> -{
> -ram_addr_t ram_addr;
> -
> -ram_addr = qemu_ram_addr_from_host(ptr);
> -if (ram_addr == RAM_ADDR_INVALID) {
> -error_report("Bad ram pointer %p", ptr);
> -abort();
> -}
> -return ram_addr;
> -}
> -
> /*
> * Note: tlb_fill() can trigger a resize of the TLB. This means that all of
> the
> * caller's prior references to the TLB table (e.g. CPUTLBEntry pointers)
> must
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index dc3c3e5f2e..d4c30e99ea 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -2460,6 +2460,18 @@ ram_addr_t qemu_ram_addr_from_host(void *ptr)
> return block->offset + offset;
> }
>
> +ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
> +{
> +ram_addr_t ram_addr;
> +
> +ram_addr = qemu_ram_addr_from_host(ptr);
> +if (ram_addr == RAM_ADDR_INVALID) {
> +error_report("Bad ram pointer %p", ptr);
> +abort();
> +}
> +return ram_addr;
> +}
> +
> static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
> MemTxAttrs attrs, void *buf, hwaddr len);
> static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs
> attrs,
> --
> 2.34.1
>
>
Re: [PATCH v6 13/21] accel/tcg: Add nofault parameter to get_page_addr_code_hostp
On Fri, Aug 19, 2022 at 1:36 PM Richard Henderson
wrote:
>
> Signed-off-by: Richard Henderson
Reviewed-by: Alistair Francis
Alistair
> ---
> include/exec/exec-all.h | 10 +-
> accel/tcg/cputlb.c | 8
> accel/tcg/plugin-gen.c | 4 ++--
> accel/tcg/user-exec.c | 4 ++--
> 4 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 9f35e3b7a9..7a6dc44d86 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -599,6 +599,8 @@ struct MemoryRegionSection *iotlb_to_section(CPUState
> *cpu,
> * get_page_addr_code_hostp()
> * @env: CPUArchState
> * @addr: guest virtual address of guest code
> + * @nofault: do not raise an exception
> + * @hostp: output for host pointer
> *
> * See get_page_addr_code() (full-system version) for documentation on the
> * return value.
> @@ -607,10 +609,10 @@ struct MemoryRegionSection *iotlb_to_section(CPUState
> *cpu,
> * If the return value is -1, sets *@hostp to NULL. Otherwise, sets *@hostp
> * to the host address where @addr's content is kept.
> *
> - * Note: this function can trigger an exception.
> + * Note: Unless @nofault, this function can trigger an exception.
> */
> tb_page_addr_t get_page_addr_code_hostp(CPUArchState *env, target_ulong addr,
> -void **hostp);
> +bool nofault, void **hostp);
>
> /**
> * get_page_addr_code()
> @@ -620,13 +622,11 @@ tb_page_addr_t get_page_addr_code_hostp(CPUArchState
> *env, target_ulong addr,
> * If we cannot translate and execute from the entire RAM page, or if
> * the region is not backed by RAM, returns -1. Otherwise, returns the
> * ram_addr_t corresponding to the guest code at @addr.
> - *
> - * Note: this function can trigger an exception.
> */
> static inline tb_page_addr_t get_page_addr_code(CPUArchState *env,
> target_ulong addr)
> {
> -return get_page_addr_code_hostp(env, addr, NULL);
> +return get_page_addr_code_hostp(env, addr, true, NULL);
> }
>
> #if defined(CONFIG_USER_ONLY)
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 2dc2affa12..ae7b40dd51 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1644,16 +1644,16 @@ void *tlb_vaddr_to_host(CPUArchState *env, abi_ptr
> addr,
> * of RAM. This will force us to execute by loading and translating
> * one insn at a time, without caching.
> *
> - * NOTE: This function will trigger an exception if the page is
> - * not executable.
> + * NOTE: Unless @nofault, this function will trigger an exception
> + * if the page is not executable.
> */
> tb_page_addr_t get_page_addr_code_hostp(CPUArchState *env, target_ulong addr,
> -void **hostp)
> +bool nofault, void **hostp)
> {
> void *p;
>
> (void)probe_access_internal(env, addr, 1, MMU_INST_FETCH,
> -cpu_mmu_index(env, true), true, &p, 0);
> +cpu_mmu_index(env, true), nofault, &p, 0);
> if (p == NULL) {
> return -1;
> }
> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
> index 3d0b101e34..8377c15383 100644
> --- a/accel/tcg/plugin-gen.c
> +++ b/accel/tcg/plugin-gen.c
> @@ -872,7 +872,7 @@ bool plugin_gen_tb_start(CPUState *cpu, const
> TranslationBlock *tb, bool mem_onl
>
> ptb->vaddr = tb->pc;
> ptb->vaddr2 = -1;
> -get_page_addr_code_hostp(cpu->env_ptr, tb->pc, &ptb->haddr1);
> +get_page_addr_code_hostp(cpu->env_ptr, tb->pc, true, &ptb->haddr1);
> ptb->haddr2 = NULL;
> ptb->mem_only = mem_only;
>
> @@ -902,7 +902,7 @@ void plugin_gen_insn_start(CPUState *cpu, const
> DisasContextBase *db)
> unlikely((db->pc_next & TARGET_PAGE_MASK) !=
> (db->pc_first & TARGET_PAGE_MASK))) {
> get_page_addr_code_hostp(cpu->env_ptr, db->pc_next,
> - &ptb->haddr2);
> + true, &ptb->haddr2);
> ptb->vaddr2 = db->pc_next;
> }
> if (likely(ptb->vaddr2 == -1)) {
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index 58edd33896..e7fec960c2 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -197,11 +197,11 @@ void *probe_access(CPUArchState *env, target_ulong
> addr, int size,
> }
>
> tb_page_addr_t get_page_addr_code_hostp(CPUArchState *env, target_ulong addr,
> -void **hostp)
> +bool nofault, void **hostp)
> {
> int flags;
>
> -flags = probe_access_internal(env, addr, 1, MMU_INST_FETCH, true, 0);
> +flags = probe_access_internal(env, addr, 1, MMU_INST_FETCH, nofault, 0);
> if (unlikely(flags)) {
> return -1;
> }
> --
> 2.34.1
>
>
Re: [PATCH v6 08/21] accel/tcg: Properly implement get_page_addr_code for user-only
On Fri, Aug 19, 2022 at 1:40 PM Richard Henderson
wrote:
>
> The current implementation is a no-op, simply returning addr.
> This is incorrect, because we ought to be checking the page
> permissions for execution.
>
> Make get_page_addr_code inline for both implementations.
>
> Signed-off-by: Richard Henderson
Acked-by: Alistair Francis
Alistair
> ---
> include/exec/exec-all.h | 85 ++---
> accel/tcg/cputlb.c | 5 ---
> accel/tcg/user-exec.c | 15
> 3 files changed, 43 insertions(+), 62 deletions(-)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 311e5fb422..0475ec6007 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -598,43 +598,44 @@ struct MemoryRegionSection *iotlb_to_section(CPUState
> *cpu,
> hwaddr index, MemTxAttrs attrs);
> #endif
>
> -#if defined(CONFIG_USER_ONLY)
> -void mmap_lock(void);
> -void mmap_unlock(void);
> -bool have_mmap_lock(void);
> -
> /**
> - * get_page_addr_code() - user-mode version
> + * get_page_addr_code_hostp()
> * @env: CPUArchState
> * @addr: guest virtual address of guest code
> *
> - * Returns @addr.
> + * See get_page_addr_code() (full-system version) for documentation on the
> + * return value.
> + *
> + * Sets *@hostp (when @hostp is non-NULL) as follows.
> + * If the return value is -1, sets *@hostp to NULL. Otherwise, sets *@hostp
> + * to the host address where @addr's content is kept.
> + *
> + * Note: this function can trigger an exception.
> + */
> +tb_page_addr_t get_page_addr_code_hostp(CPUArchState *env, target_ulong addr,
> +void **hostp);
> +
> +/**
> + * get_page_addr_code()
> + * @env: CPUArchState
> + * @addr: guest virtual address of guest code
> + *
> + * If we cannot translate and execute from the entire RAM page, or if
> + * the region is not backed by RAM, returns -1. Otherwise, returns the
> + * ram_addr_t corresponding to the guest code at @addr.
> + *
> + * Note: this function can trigger an exception.
> */
> static inline tb_page_addr_t get_page_addr_code(CPUArchState *env,
> target_ulong addr)
> {
> -return addr;
> +return get_page_addr_code_hostp(env, addr, NULL);
> }
>
> -/**
> - * get_page_addr_code_hostp() - user-mode version
> - * @env: CPUArchState
> - * @addr: guest virtual address of guest code
> - *
> - * Returns @addr.
> - *
> - * If @hostp is non-NULL, sets *@hostp to the host address where @addr's
> content
> - * is kept.
> - */
> -static inline tb_page_addr_t get_page_addr_code_hostp(CPUArchState *env,
> - target_ulong addr,
> - void **hostp)
> -{
> -if (hostp) {
> -*hostp = g2h_untagged(addr);
> -}
> -return addr;
> -}
> +#if defined(CONFIG_USER_ONLY)
> +void mmap_lock(void);
> +void mmap_unlock(void);
> +bool have_mmap_lock(void);
>
> /**
> * adjust_signal_pc:
> @@ -691,36 +692,6 @@ G_NORETURN void cpu_loop_exit_sigbus(CPUState *cpu,
> target_ulong addr,
> static inline void mmap_lock(void) {}
> static inline void mmap_unlock(void) {}
>
> -/**
> - * get_page_addr_code() - full-system version
> - * @env: CPUArchState
> - * @addr: guest virtual address of guest code
> - *
> - * If we cannot translate and execute from the entire RAM page, or if
> - * the region is not backed by RAM, returns -1. Otherwise, returns the
> - * ram_addr_t corresponding to the guest code at @addr.
> - *
> - * Note: this function can trigger an exception.
> - */
> -tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr);
> -
> -/**
> - * get_page_addr_code_hostp() - full-system version
> - * @env: CPUArchState
> - * @addr: guest virtual address of guest code
> - *
> - * See get_page_addr_code() (full-system version) for documentation on the
> - * return value.
> - *
> - * Sets *@hostp (when @hostp is non-NULL) as follows.
> - * If the return value is -1, sets *@hostp to NULL. Otherwise, sets *@hostp
> - * to the host address where @addr's content is kept.
> - *
> - * Note: this function can trigger an exception.
> - */
> -tb_page_addr_t get_page_addr_code_hostp(CPUArchState *env, target_ulong addr,
> -void **hostp);
> -
> void tlb_reset_dirty(CPUState *cpu, ram_addr_t start1, ram_addr_t length);
> void tlb_set_dirty(CPUState *cpu, target_ulong vaddr);
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index a46f3a654d..43bd65c973 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1544,11 +1544,6 @@ tb_page_addr_t get_page_addr_code_hostp(CPUArchState
> *env, target_ulong addr,
> return qemu_ram_addr_from_host_nofail(p);
> }
>
> -tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
> -{
> -return get_page_addr_code_hostp(env, addr, NULL);
> -}
> -
>
Re: [PATCH v6 14/21] accel/tcg: Raise PROT_EXEC exception early
On Fri, Aug 19, 2022 at 1:40 PM Richard Henderson
wrote:
>
> We currently ignore PROT_EXEC on the initial lookup, and
> defer raising the exception until cpu_ld*_code().
> It makes more sense to raise the exception early.
>
> Signed-off-by: Richard Henderson
Acked-by: Alistair Francis
Alistair
> ---
> accel/tcg/cpu-exec.c | 2 +-
> accel/tcg/translate-all.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 7887af6f45..7b8977a0a4 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -222,7 +222,7 @@ static TranslationBlock *tb_htable_lookup(CPUState *cpu,
> target_ulong pc,
> desc.cflags = cflags;
> desc.trace_vcpu_dstate = *cpu->trace_dstate;
> desc.pc = pc;
> -phys_pc = get_page_addr_code(desc.env, pc);
> +phys_pc = get_page_addr_code_hostp(desc.env, pc, false, NULL);
> if (phys_pc == -1) {
> return NULL;
> }
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index b83161a081..069ed67bac 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -1396,7 +1396,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
> assert_memory_lock();
> qemu_thread_jit_write();
>
> -phys_pc = get_page_addr_code(env, pc);
> +phys_pc = get_page_addr_code_hostp(env, pc, false, NULL);
>
> if (phys_pc == -1) {
> /* Generate a one-shot TB with 1 insn in it */
> --
> 2.34.1
>
>
Re: [PATCH v6 15/21] accel/tcg: Remove translator_ldsw
On Fri, Aug 19, 2022 at 1:36 PM Richard Henderson
wrote:
>
> The only user can easily use translator_lduw and
> adjust the type to signed during the return.
>
> Signed-off-by: Richard Henderson
Reviewed-by: Alistair Francis
Alistair
> ---
> include/exec/translator.h | 1 -
> target/i386/tcg/translate.c | 2 +-
> 2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/include/exec/translator.h b/include/exec/translator.h
> index 0d0bf3a31e..45b9268ca4 100644
> --- a/include/exec/translator.h
> +++ b/include/exec/translator.h
> @@ -178,7 +178,6 @@ bool translator_use_goto_tb(DisasContextBase *db,
> target_ulong dest);
>
> #define FOR_EACH_TRANSLATOR_LD(F) \
> F(translator_ldub, uint8_t, cpu_ldub_code, /* no swap */) \
> -F(translator_ldsw, int16_t, cpu_ldsw_code, bswap16) \
> F(translator_lduw, uint16_t, cpu_lduw_code, bswap16)\
> F(translator_ldl, uint32_t, cpu_ldl_code, bswap32) \
> F(translator_ldq, uint64_t, cpu_ldq_code, bswap64)
> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> index b7972f0ff5..a23417d058 100644
> --- a/target/i386/tcg/translate.c
> +++ b/target/i386/tcg/translate.c
> @@ -2033,7 +2033,7 @@ static inline uint8_t x86_ldub_code(CPUX86State *env,
> DisasContext *s)
>
> static inline int16_t x86_ldsw_code(CPUX86State *env, DisasContext *s)
> {
> -return translator_ldsw(env, &s->base, advance_pc(env, s, 2));
> +return translator_lduw(env, &s->base, advance_pc(env, s, 2));
> }
>
> static inline uint16_t x86_lduw_code(CPUX86State *env, DisasContext *s)
> --
> 2.34.1
>
>
[PATCH v3 1/4] hw/ssi: ibex_spi: fixup typos in ibex_spi_host
From: Wilfred Mallawa
This patch fixes up minor typos in ibex_spi_host
Signed-off-by: Wilfred Mallawa
Reviewed-by: Alistair Francis
Reviewed-by: Andrew Jones
---
hw/ssi/ibex_spi_host.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c
index d14580b409..601041d719 100644
--- a/hw/ssi/ibex_spi_host.c
+++ b/hw/ssi/ibex_spi_host.c
@@ -172,7 +172,7 @@ static void ibex_spi_host_irq(IbexSPIHostState *s)
& R_INTR_STATE_SPI_EVENT_MASK;
int err_irq = 0, event_irq = 0;
-/* Error IRQ enabled and Error IRQ Cleared*/
+/* Error IRQ enabled and Error IRQ Cleared */
if (error_en && !err_pending) {
/* Event enabled, Interrupt Test Error */
if (s->regs[IBEX_SPI_HOST_INTR_TEST] & R_INTR_TEST_ERROR_MASK) {
@@ -434,7 +434,7 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr,
case IBEX_SPI_HOST_TXDATA:
/*
* This is a hardware `feature` where
- * the first word written TXDATA after init is omitted entirely
+ * the first word written to TXDATA after init is omitted entirely
*/
if (s->init_status) {
s->init_status = false;
@@ -487,7 +487,7 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr,
break;
case IBEX_SPI_HOST_ERROR_STATUS:
/*
- * Indicates that any errors that have occurred.
+ * Indicates any errors that have occurred.
* When an error occurs, the corresponding bit must be cleared
* here before issuing any further commands
*/
--
2.37.2
[PATCH v3 3/4] hw/ssi: ibex_spi: fixup/add rw1c functionality
From: Wilfred Mallawa
This patch adds the `rw1c` functionality to the respective
registers. The status fields are cleared when the respective
field is set.
Signed-off-by: Wilfred Mallawa
Reviewed-by: Alistair Francis
---
hw/ssi/ibex_spi_host.c | 34 --
include/hw/ssi/ibex_spi_host.h | 4 ++--
2 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c
index 1298664d2b..3809febb0c 100644
--- a/hw/ssi/ibex_spi_host.c
+++ b/hw/ssi/ibex_spi_host.c
@@ -350,7 +350,17 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr,
switch (addr) {
/* Skipping any R/O registers */
-case IBEX_SPI_HOST_INTR_STATE...IBEX_SPI_HOST_INTR_ENABLE:
+case IBEX_SPI_HOST_INTR_STATE:
+/* rw1c status register */
+if (FIELD_EX32(val32, INTR_STATE, ERROR)) {
+data = FIELD_DP32(data, INTR_STATE, ERROR, 0);
+}
+if (FIELD_EX32(val32, INTR_STATE, SPI_EVENT)) {
+data = FIELD_DP32(data, INTR_STATE, SPI_EVENT, 0);
+}
+s->regs[addr] = data;
+break;
+case IBEX_SPI_HOST_INTR_ENABLE:
s->regs[addr] = val32;
break;
case IBEX_SPI_HOST_INTR_TEST:
@@ -493,7 +503,27 @@ static void ibex_spi_host_write(void *opaque, hwaddr addr,
* When an error occurs, the corresponding bit must be cleared
* here before issuing any further commands
*/
-s->regs[addr] = val32;
+status = s->regs[addr];
+/* rw1c status register */
+if (FIELD_EX32(val32, ERROR_STATUS, CMDBUSY)) {
+status = FIELD_DP32(status, ERROR_STATUS, CMDBUSY, 0);
+}
+if (FIELD_EX32(val32, ERROR_STATUS, OVERFLOW)) {
+status = FIELD_DP32(status, ERROR_STATUS, OVERFLOW, 0);
+}
+if (FIELD_EX32(val32, ERROR_STATUS, UNDERFLOW)) {
+status = FIELD_DP32(status, ERROR_STATUS, UNDERFLOW, 0);
+}
+if (FIELD_EX32(val32, ERROR_STATUS, CMDINVAL)) {
+status = FIELD_DP32(status, ERROR_STATUS, CMDINVAL, 0);
+}
+if (FIELD_EX32(val32, ERROR_STATUS, CSIDINVAL)) {
+status = FIELD_DP32(status, ERROR_STATUS, CSIDINVAL, 0);
+}
+if (FIELD_EX32(val32, ERROR_STATUS, ACCESSINVAL)) {
+status = FIELD_DP32(status, ERROR_STATUS, ACCESSINVAL, 0);
+}
+s->regs[addr] = status;
break;
case IBEX_SPI_HOST_EVENT_ENABLE:
/* Controls which classes of SPI events raise an interrupt. */
diff --git a/include/hw/ssi/ibex_spi_host.h b/include/hw/ssi/ibex_spi_host.h
index 3fedcb6805..1f6d077766 100644
--- a/include/hw/ssi/ibex_spi_host.h
+++ b/include/hw/ssi/ibex_spi_host.h
@@ -40,7 +40,7 @@
OBJECT_CHECK(IbexSPIHostState, (obj), TYPE_IBEX_SPI_HOST)
/* SPI Registers */
-#define IBEX_SPI_HOST_INTR_STATE (0x00 / 4) /* rw */
+#define IBEX_SPI_HOST_INTR_STATE (0x00 / 4) /* rw1c */
#define IBEX_SPI_HOST_INTR_ENABLE(0x04 / 4) /* rw */
#define IBEX_SPI_HOST_INTR_TEST (0x08 / 4) /* wo */
#define IBEX_SPI_HOST_ALERT_TEST (0x0c / 4) /* wo */
@@ -54,7 +54,7 @@
#define IBEX_SPI_HOST_TXDATA (0x28 / 4)
#define IBEX_SPI_HOST_ERROR_ENABLE (0x2c / 4) /* rw */
-#define IBEX_SPI_HOST_ERROR_STATUS (0x30 / 4) /* rw */
+#define IBEX_SPI_HOST_ERROR_STATUS (0x30 / 4) /* rw1c */
#define IBEX_SPI_HOST_EVENT_ENABLE (0x34 / 4) /* rw */
/* FIFO Len in Bytes */
--
2.37.2
[PATCH v3 0/4] hw/ssi: ibex_spi: cleanup and fixup bugs
From: Wilfred Mallawa This patch series cleans up the ibex_spi driver, fixes the specified coverity issue, implements register rw1c functionality and updates an incorrect register offset. Patch V3 fixes up: - Style errors (excess indentation on multi-line) - Remove patch note from commit message in [2/4] Testing: - Tested with Opentitan unit tests for TockOS...[OK] Wilfred Mallawa (4): hw/ssi: ibex_spi: fixup typos in ibex_spi_host hw/ssi: ibex_spi: fixup coverity issue hw/ssi: ibex_spi: fixup/add rw1c functionality hw/ssi: ibex_spi: update reg addr hw/ssi/ibex_spi_host.c | 172 +++-- include/hw/ssi/ibex_spi_host.h | 4 +- 2 files changed, 104 insertions(+), 72 deletions(-) -- 2.37.2
[PATCH v3 4/4] hw/ssi: ibex_spi: update reg addr
From: Wilfred Mallawa Updates the `EVENT_ENABLE` register to offset `0x34` as per OpenTitan spec [1]. [1] https://docs.opentitan.org/hw/ip/spi_host/doc/#Reg_event_enable Signed-off-by: Wilfred Mallawa Reviewed-by: Alistair Francis --- hw/ssi/ibex_spi_host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c index 3809febb0c..bafff8ca1f 100644 --- a/hw/ssi/ibex_spi_host.c +++ b/hw/ssi/ibex_spi_host.c @@ -93,7 +93,7 @@ REG32(ERROR_STATUS, 0x30) FIELD(ERROR_STATUS, CMDINVAL, 3, 1) FIELD(ERROR_STATUS, CSIDINVAL, 4, 1) FIELD(ERROR_STATUS, ACCESSINVAL, 5, 1) -REG32(EVENT_ENABLE, 0x30) +REG32(EVENT_ENABLE, 0x34) FIELD(EVENT_ENABLE, RXFULL, 0, 1) FIELD(EVENT_ENABLE, TXEMPTY, 1, 1) FIELD(EVENT_ENABLE, RXWM, 2, 1) -- 2.37.2
Re: [PATCH v6 16/21] accel/tcg: Add pc and host_pc params to gen_intermediate_code
On Fri, Aug 19, 2022 at 1:42 PM Richard Henderson
wrote:
>
> Pass these along to translator_loop -- pc may be used instead
> of tb->pc, and host_pc is currently unused. Adjust all targets
> at one time.
>
> Signed-off-by: Richard Henderson
Acked-by: Alistair Francis
Alistair
> ---
> include/exec/exec-all.h | 1 -
> include/exec/translator.h | 24
> accel/tcg/translate-all.c | 3 ++-
> accel/tcg/translator.c| 9 +
> target/alpha/translate.c | 5 +++--
> target/arm/translate.c| 5 +++--
> target/avr/translate.c| 5 +++--
> target/cris/translate.c | 5 +++--
> target/hexagon/translate.c| 6 --
> target/hppa/translate.c | 5 +++--
> target/i386/tcg/translate.c | 5 +++--
> target/loongarch/translate.c | 6 --
> target/m68k/translate.c | 5 +++--
> target/microblaze/translate.c | 5 +++--
> target/mips/tcg/translate.c | 5 +++--
> target/nios2/translate.c | 5 +++--
> target/openrisc/translate.c | 6 --
> target/ppc/translate.c| 5 +++--
> target/riscv/translate.c | 5 +++--
> target/rx/translate.c | 5 +++--
> target/s390x/tcg/translate.c | 5 +++--
> target/sh4/translate.c| 5 +++--
> target/sparc/translate.c | 5 +++--
> target/tricore/translate.c| 6 --
> target/xtensa/translate.c | 6 --
> 25 files changed, 95 insertions(+), 52 deletions(-)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 7a6dc44d86..4ad166966b 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -39,7 +39,6 @@ typedef ram_addr_t tb_page_addr_t;
> #define TB_PAGE_ADDR_FMT RAM_ADDR_FMT
> #endif
>
> -void gen_intermediate_code(CPUState *cpu, TranslationBlock *tb, int
> max_insns);
> void restore_state_to_opc(CPUArchState *env, TranslationBlock *tb,
>target_ulong *data);
>
> diff --git a/include/exec/translator.h b/include/exec/translator.h
> index 45b9268ca4..69db0f5c21 100644
> --- a/include/exec/translator.h
> +++ b/include/exec/translator.h
> @@ -26,6 +26,19 @@
> #include "exec/translate-all.h"
> #include "tcg/tcg.h"
>
> +/**
> + * gen_intermediate_code
> + * @cpu: cpu context
> + * @tb: translation block
> + * @max_insns: max number of instructions to translate
> + * @pc: guest virtual program counter address
> + * @host_pc: host physical program counter address
> + *
> + * This function must be provided by the target, which should create
> + * the target-specific DisasContext, and then invoke translator_loop.
> + */
> +void gen_intermediate_code(CPUState *cpu, TranslationBlock *tb, int
> max_insns,
> + target_ulong pc, void *host_pc);
>
> /**
> * DisasJumpType:
> @@ -123,11 +136,13 @@ typedef struct TranslatorOps {
>
> /**
> * translator_loop:
> - * @ops: Target-specific operations.
> - * @db: Disassembly context.
> * @cpu: Target vCPU.
> * @tb: Translation block.
> * @max_insns: Maximum number of insns to translate.
> + * @pc: guest virtual program counter address
> + * @host_pc: host physical program counter address
> + * @ops: Target-specific operations.
> + * @db: Disassembly context.
> *
> * Generic translator loop.
> *
> @@ -141,8 +156,9 @@ typedef struct TranslatorOps {
> * - When single-stepping is enabled (system-wide or on the current vCPU).
> * - When too many instructions have been translated.
> */
> -void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
> - CPUState *cpu, TranslationBlock *tb, int max_insns);
> +void translator_loop(CPUState *cpu, TranslationBlock *tb, int max_insns,
> + target_ulong pc, void *host_pc,
> + const TranslatorOps *ops, DisasContextBase *db);
>
> void translator_loop_temp_check(DisasContextBase *db);
>
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 069ed67bac..b224f856d0 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -46,6 +46,7 @@
>
> #include "exec/cputlb.h"
> #include "exec/translate-all.h"
> +#include "exec/translator.h"
> #include "qemu/bitmap.h"
> #include "qemu/qemu-print.h"
> #include "qemu/timer.h"
> @@ -1444,7 +1445,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
> tcg_func_start(tcg_ctx);
>
> tcg_ctx->cpu = env_cpu(env);
> -gen_intermediate_code(cpu, tb, max_insns);
> +gen_intermediate_code(cpu, tb, max_insns, pc, host_pc);
> assert(tb->size != 0);
> tcg_ctx->cpu = NULL;
> max_insns = tb->icount;
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index fe7af9b943..3eef30d93a 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -51,16 +51,17 @@ static inline void
> translator_page_protect(DisasContextBase *dcbase,
> #endif
> }
>
> -void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
> - CPUState *cpu, Tr
Re: [PATCH v6 20/21] target/riscv: Add MAX_INSN_LEN and insn_len
On Fri, Aug 19, 2022 at 1:42 PM Richard Henderson
wrote:
>
> These will be useful in properly ending the TB.
>
> Signed-off-by: Richard Henderson
Reviewed-by: Alistair Francis
Alistair
> ---
> target/riscv/translate.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 38666ddc91..a719aa6e63 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -1022,6 +1022,14 @@ static uint32_t opcode_at(DisasContextBase *dcbase,
> target_ulong pc)
> /* Include decoders for factored-out extensions */
> #include "decode-XVentanaCondOps.c.inc"
>
> +/* The specification allows for longer insns, but not supported by qemu. */
> +#define MAX_INSN_LEN 4
> +
> +static inline int insn_len(uint16_t first_word)
> +{
> +return (first_word & 3) == 3 ? 4 : 2;
> +}
> +
> static void decode_opc(CPURISCVState *env, DisasContext *ctx, uint16_t
> opcode)
> {
> /*
> @@ -1037,7 +1045,7 @@ static void decode_opc(CPURISCVState *env, DisasContext
> *ctx, uint16_t opcode)
> };
>
> /* Check for compressed insn */
> -if (extract16(opcode, 0, 2) != 3) {
> +if (insn_len(opcode) == 2) {
> if (!has_ext(ctx, RVC)) {
> gen_exception_illegal(ctx);
> } else {
> --
> 2.34.1
>
>
[PATCH v3 2/4] hw/ssi: ibex_spi: fixup coverity issue
From: Wilfred Mallawa
This patch addresses the coverity issues specified in [1],
as suggested, `FIELD_DP32()`/`FIELD_EX32()` macros have been
implemented to clean up the code.
[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg887713.html
Fixes: Coverity CID 1488107
Signed-off-by: Wilfred Mallawa
Reviewed-by: Andrew Jones
---
hw/ssi/ibex_spi_host.c | 130 +
1 file changed, 66 insertions(+), 64 deletions(-)
diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c
index 601041d719..1298664d2b 100644
--- a/hw/ssi/ibex_spi_host.c
+++ b/hw/ssi/ibex_spi_host.c
@@ -108,18 +108,20 @@ static inline uint8_t div4_round_up(uint8_t dividend)
static void ibex_spi_rxfifo_reset(IbexSPIHostState *s)
{
+uint32_t data = s->regs[IBEX_SPI_HOST_STATUS];
/* Empty the RX FIFO and assert RXEMPTY */
fifo8_reset(&s->rx_fifo);
-s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXFULL_MASK;
-s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_RXEMPTY_MASK;
+data = FIELD_DP32(data, STATUS, RXEMPTY, 1);
+s->regs[IBEX_SPI_HOST_STATUS] = data;
}
static void ibex_spi_txfifo_reset(IbexSPIHostState *s)
{
+uint32_t data = s->regs[IBEX_SPI_HOST_STATUS];
/* Empty the TX FIFO and assert TXEMPTY */
fifo8_reset(&s->tx_fifo);
-s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXFULL_MASK;
-s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_TXEMPTY_MASK;
+data = FIELD_DP32(data, STATUS, TXEMPTY, 1);
+s->regs[IBEX_SPI_HOST_STATUS] = data;
}
static void ibex_spi_host_reset(DeviceState *dev)
@@ -162,37 +164,38 @@ static void ibex_spi_host_reset(DeviceState *dev)
*/
static void ibex_spi_host_irq(IbexSPIHostState *s)
{
-bool error_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE]
-& R_INTR_ENABLE_ERROR_MASK;
-bool event_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE]
-& R_INTR_ENABLE_SPI_EVENT_MASK;
-bool err_pending = s->regs[IBEX_SPI_HOST_INTR_STATE]
-& R_INTR_STATE_ERROR_MASK;
-bool status_pending = s->regs[IBEX_SPI_HOST_INTR_STATE]
-& R_INTR_STATE_SPI_EVENT_MASK;
+uint32_t intr_test_reg = s->regs[IBEX_SPI_HOST_INTR_TEST];
+uint32_t intr_en_reg = s->regs[IBEX_SPI_HOST_INTR_ENABLE];
+uint32_t intr_state_reg = s->regs[IBEX_SPI_HOST_INTR_STATE];
+
+uint32_t err_en_reg = s->regs[IBEX_SPI_HOST_ERROR_ENABLE];
+uint32_t event_en_reg = s->regs[IBEX_SPI_HOST_EVENT_ENABLE];
+uint32_t err_status_reg = s->regs[IBEX_SPI_HOST_ERROR_STATUS];
+uint32_t status_reg = s->regs[IBEX_SPI_HOST_STATUS];
+
+
+bool error_en = FIELD_EX32(intr_en_reg, INTR_ENABLE, ERROR);
+bool event_en = FIELD_EX32(intr_en_reg, INTR_ENABLE, SPI_EVENT);
+bool err_pending = FIELD_EX32(intr_state_reg, INTR_STATE, ERROR);
+bool status_pending = FIELD_EX32(intr_state_reg, INTR_STATE, SPI_EVENT);
+
int err_irq = 0, event_irq = 0;
/* Error IRQ enabled and Error IRQ Cleared */
if (error_en && !err_pending) {
/* Event enabled, Interrupt Test Error */
-if (s->regs[IBEX_SPI_HOST_INTR_TEST] & R_INTR_TEST_ERROR_MASK) {
+if (FIELD_EX32(intr_test_reg, INTR_TEST, ERROR)) {
err_irq = 1;
-} else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE]
-& R_ERROR_ENABLE_CMDBUSY_MASK) &&
-s->regs[IBEX_SPI_HOST_ERROR_STATUS]
-& R_ERROR_STATUS_CMDBUSY_MASK) {
+} else if (FIELD_EX32(err_en_reg, ERROR_ENABLE, CMDBUSY) &&
+ FIELD_EX32(err_status_reg, ERROR_STATUS, CMDBUSY)) {
/* Wrote to COMMAND when not READY */
err_irq = 1;
-} else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE]
-& R_ERROR_ENABLE_CMDINVAL_MASK) &&
-s->regs[IBEX_SPI_HOST_ERROR_STATUS]
-& R_ERROR_STATUS_CMDINVAL_MASK) {
+} else if (FIELD_EX32(err_en_reg, ERROR_ENABLE, CMDINVAL) &&
+ FIELD_EX32(err_status_reg, ERROR_STATUS, CMDINVAL)) {
/* Invalid command segment */
err_irq = 1;
-} else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE]
-& R_ERROR_ENABLE_CSIDINVAL_MASK) &&
-s->regs[IBEX_SPI_HOST_ERROR_STATUS]
-& R_ERROR_STATUS_CSIDINVAL_MASK) {
+} else if (FIELD_EX32(err_en_reg, ERROR_ENABLE, CSIDINVAL) &&
+ FIELD_EX32(err_status_reg, ERROR_STATUS, CSIDINVAL)) {
/* Invalid value for CSID */
err_irq = 1;
}
@@ -204,22 +207,19 @@ static void ibex_spi_host_irq(IbexSPIHostState *s)
/* Event IRQ Enabled and Event IRQ Cleared */
if (event_en && !status_pending) {
-if (s->regs[IBEX_SPI_HOST_INTR_TEST] & R_INTR_TEST_SPI_EVENT_MASK) {
+if (FIELD_EX32(intr_test_reg, INTR_STATE, SPI_EVENT)) {
/* Event enabled, Interrupt Test Event */
event_irq = 1;
-} else if ((s->regs[IBEX_SPI_HO
Re: [PATCH v6 21/21] target/riscv: Make translator stop before the end of a page
On Fri, Aug 19, 2022 at 1:39 PM Richard Henderson
wrote:
>
> Right now the translator stops right *after* the end of a page, which
> breaks reporting of fault locations when the last instruction of a
> multi-insn translation block crosses a page boundary.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1155
> Signed-off-by: Richard Henderson
Reviewed-by: Alistair Francis
Alistair
> ---
> target/riscv/translate.c | 17 +--
> tests/tcg/riscv64/noexec.c| 79 +++
> tests/tcg/riscv64/Makefile.target | 1 +
> 3 files changed, 93 insertions(+), 4 deletions(-)
> create mode 100644 tests/tcg/riscv64/noexec.c
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index a719aa6e63..f8af6daa70 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -1154,12 +1154,21 @@ static void riscv_tr_translate_insn(DisasContextBase
> *dcbase, CPUState *cpu)
> }
> ctx->nftemp = 0;
>
> +/* Only the first insn within a TB is allowed to cross a page boundary.
> */
> if (ctx->base.is_jmp == DISAS_NEXT) {
> -target_ulong page_start;
> -
> -page_start = ctx->base.pc_first & TARGET_PAGE_MASK;
> -if (ctx->base.pc_next - page_start >= TARGET_PAGE_SIZE) {
> +if (!is_same_page(&ctx->base, ctx->base.pc_next)) {
> ctx->base.is_jmp = DISAS_TOO_MANY;
> +} else {
> +unsigned page_ofs = ctx->base.pc_next & ~TARGET_PAGE_MASK;
> +
> +if (page_ofs > TARGET_PAGE_SIZE - MAX_INSN_LEN) {
> +uint16_t next_insn = cpu_lduw_code(env, ctx->base.pc_next);
> +int len = insn_len(next_insn);
> +
> +if (!is_same_page(&ctx->base, ctx->base.pc_next + len)) {
> +ctx->base.is_jmp = DISAS_TOO_MANY;
> +}
> +}
> }
> }
> }
> diff --git a/tests/tcg/riscv64/noexec.c b/tests/tcg/riscv64/noexec.c
> new file mode 100644
> index 00..86f64b28db
> --- /dev/null
> +++ b/tests/tcg/riscv64/noexec.c
> @@ -0,0 +1,79 @@
> +#include "../multiarch/noexec.c.inc"
> +
> +static void *arch_mcontext_pc(const mcontext_t *ctx)
> +{
> +return (void *)ctx->__gregs[REG_PC];
> +}
> +
> +static int arch_mcontext_arg(const mcontext_t *ctx)
> +{
> +return ctx->__gregs[REG_A0];
> +}
> +
> +static void arch_flush(void *p, int len)
> +{
> +__builtin___clear_cache(p, p + len);
> +}
> +
> +extern char noexec_1[];
> +extern char noexec_2[];
> +extern char noexec_end[];
> +
> +asm(".option push\n"
> +".option norvc\n"
> +"noexec_1:\n"
> +" li a0,1\n" /* a0 is 0 on entry, set 1. */
> +"noexec_2:\n"
> +" li a0,2\n" /* a0 is 0/1; set 2. */
> +" ret\n"
> +"noexec_end:\n"
> +".option pop");
> +
> +int main(void)
> +{
> +struct noexec_test noexec_tests[] = {
> +{
> +.name = "fallthrough",
> +.test_code = noexec_1,
> +.test_len = noexec_end - noexec_1,
> +.page_ofs = noexec_1 - noexec_2,
> +.entry_ofs = noexec_1 - noexec_2,
> +.expected_si_ofs = 0,
> +.expected_pc_ofs = 0,
> +.expected_arg = 1,
> +},
> +{
> +.name = "jump",
> +.test_code = noexec_1,
> +.test_len = noexec_end - noexec_1,
> +.page_ofs = noexec_1 - noexec_2,
> +.entry_ofs = 0,
> +.expected_si_ofs = 0,
> +.expected_pc_ofs = 0,
> +.expected_arg = 0,
> +},
> +{
> +.name = "fallthrough [cross]",
> +.test_code = noexec_1,
> +.test_len = noexec_end - noexec_1,
> +.page_ofs = noexec_1 - noexec_2 - 2,
> +.entry_ofs = noexec_1 - noexec_2 - 2,
> +.expected_si_ofs = 0,
> +.expected_pc_ofs = -2,
> +.expected_arg = 1,
> +},
> +{
> +.name = "jump [cross]",
> +.test_code = noexec_1,
> +.test_len = noexec_end - noexec_1,
> +.page_ofs = noexec_1 - noexec_2 - 2,
> +.entry_ofs = -2,
> +.expected_si_ofs = 0,
> +.expected_pc_ofs = -2,
> +.expected_arg = 0,
> +},
> +};
> +
> +return test_noexec(noexec_tests,
> + sizeof(noexec_tests) / sizeof(noexec_tests[0]));
> +}
> diff --git a/tests/tcg/riscv64/Makefile.target
> b/tests/tcg/riscv64/Makefile.target
> index d41bf6d60d..b5b89dfb0e 100644
> --- a/tests/tcg/riscv64/Makefile.target
> +++ b/tests/tcg/riscv64/Makefile.target
> @@ -3,3 +3,4 @@
>
> VPATH += $(SRC_PATH)/tests/tcg/riscv64
> TESTS += test-div
> +TESTS += noexec
> --
> 2.34.1
>
>
Re: [PATCH] include/hw/riscv/sifive_e.h: Fix the type of parent_obj of SiFiveEState.
Reviewed-by: Jim Shu
On Fri, Aug 19, 2022 at 3:11 PM Tommy Wu wrote:
>
> Fix the type of parent_obj of SiFiveEState from 'SysBusDevice'
> to 'MachineState'. Because the parent of SiFiveEState is 'MachineState'.
>
> Signed-off-by: Tommy Wu
> ---
> include/hw/riscv/sifive_e.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
> index 83604da805..24359f9fe5 100644
> --- a/include/hw/riscv/sifive_e.h
> +++ b/include/hw/riscv/sifive_e.h
> @@ -41,7 +41,7 @@ typedef struct SiFiveESoCState {
>
> typedef struct SiFiveEState {
> /*< private >*/
> -SysBusDevice parent_obj;
> +MachineState parent_obj;
>
> /*< public >*/
> SiFiveESoCState soc;
> --
> 2.27.0
>
>
Re: [PATCH for-7.2 v2 10/20] hw/ppc: set machine->fdt in spapr machine
On Fri, Aug 19, 2022 at 06:42:34AM -0300, Daniel Henrique Barboza wrote: > > > On 8/18/22 23:11, Alexey Kardashevskiy wrote: > > > > > > On 05/08/2022 19:39, Daniel Henrique Barboza wrote: > > > The pSeries machine never bothered with the common machine->fdt > > > attribute. We do all the FDT related work using spapr->fdt_blob. > > > > > > We're going to introduce HMP commands to read and save the FDT, which > > > will rely on setting machine->fdt properly to work across all machine > > > archs/types. > > > > > > Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt > > property enough? > > I tried to do the minimal changes needed for the commands to work. ms::fdt is > one of the few MachineState fields that hasn't been QOMified by > machine_class_init() yet. All pre-existing code that uses ms::fdt are using > the > pointer directly. To make a QOMified use of it would require extra patches > in machine.c to QOMify the property first. > > There's also the issue with how each machine is creating the FDT. Most are > using > helpers from device_tree.c, some are creating it from scratch, others required > a .dtb file, most of them are not doing a fdt_pack() and so on. To really > QOMify > the use of ms::fdt we would need some machine hooks that standardize all that. > I believe it's worth the trouble, but it would be too much to do > right now. Hmm.. I think this depends on what you mean by "QOM"ify exactly. If you're meaning make the full DT representation QOM objects, that you can look into in detail, then, yes, that's pretty complicated. I suspect what Alexey was suggesting though, was merely to make ms::fdt accessible as a single bytestring property on the machine QOM object. Effectively it's just "dumpdtb" but as a property get. I'm not 100% certain if QOM can safely represent arbitrary bytestrings as QOM properties, which would need checking. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
Re: [PATCH for-7.2 v2 10/20] hw/ppc: set machine->fdt in spapr machine
On 22/08/2022 13:05, David Gibson wrote: On Fri, Aug 19, 2022 at 06:42:34AM -0300, Daniel Henrique Barboza wrote: On 8/18/22 23:11, Alexey Kardashevskiy wrote: On 05/08/2022 19:39, Daniel Henrique Barboza wrote: The pSeries machine never bothered with the common machine->fdt attribute. We do all the FDT related work using spapr->fdt_blob. We're going to introduce HMP commands to read and save the FDT, which will rely on setting machine->fdt properly to work across all machine archs/types. Out of curiosity - why new HMP command, is not QOM'ing this ms::fdt property enough? I tried to do the minimal changes needed for the commands to work. ms::fdt is one of the few MachineState fields that hasn't been QOMified by machine_class_init() yet. All pre-existing code that uses ms::fdt are using the pointer directly. To make a QOMified use of it would require extra patches in machine.c to QOMify the property first. There's also the issue with how each machine is creating the FDT. Most are using helpers from device_tree.c, some are creating it from scratch, others required a .dtb file, most of them are not doing a fdt_pack() and so on. To really QOMify the use of ms::fdt we would need some machine hooks that standardize all that. I believe it's worth the trouble, but it would be too much to do right now. Hmm.. I think this depends on what you mean by "QOM"ify exactly. If you're meaning make the full DT representation QOM objects, that you can look into in detail, then, yes, that's pretty complicated. I suspect what Alexey was suggesting though, was merely to make ms::fdt accessible as a single bytestring property on the machine QOM object. Effectively it's just "dumpdtb" but as a property get. Yes, I meant the bytestream, as DTC can easily decompile it onto a DTS. I'm not 100% certain if QOM can safely represent arbitrary bytestrings as QOM properties, which would need checking. I am not sure either but rather than adding another command to HMP, I'd explore this option first. -- Alexey
Re: [PATCH] include/hw/riscv/sifive_e.h: Fix the type of parent_obj of SiFiveEState.
On Fri, Aug 19, 2022 at 5:12 PM Tommy Wu wrote:
>
> Fix the type of parent_obj of SiFiveEState from 'SysBusDevice'
> to 'MachineState'. Because the parent of SiFiveEState is 'MachineState'.
>
> Signed-off-by: Tommy Wu
Reviewed-by: Alistair Francis
Alistair
> ---
> include/hw/riscv/sifive_e.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/hw/riscv/sifive_e.h b/include/hw/riscv/sifive_e.h
> index 83604da805..24359f9fe5 100644
> --- a/include/hw/riscv/sifive_e.h
> +++ b/include/hw/riscv/sifive_e.h
> @@ -41,7 +41,7 @@ typedef struct SiFiveESoCState {
>
> typedef struct SiFiveEState {
> /*< private >*/
> -SysBusDevice parent_obj;
> +MachineState parent_obj;
>
> /*< public >*/
> SiFiveESoCState soc;
> --
> 2.27.0
>
>
Re: [PATCH v3 0/4] QEMU: Fix RISC-V virt & spike machines' dtbs
On Thu, Aug 11, 2022 at 5:09 AM Conor Dooley wrote: > > From: Conor Dooley > > The device trees produced automatically for the virt and spike machines > fail dt-validate on several grounds. Some of these need to be fixed in > the linux kernel's dt-bindings, but others are caused by bugs in QEMU. > > I mostly opted for what appeared to be the smallest change that would > fix the warnings, partly due to my inexperience with the QEMU codebase. > A "sister" patchset for the kernel will clear the remaining warnings. > Thanks to Rob Herring for reporting these issues [1], > Conor. > > Changes since v2: > - move the syscon subnodes back to the top level instead of into the > syscon node > Changes since v1: > - drop patch 1 > > To reproduce the errors: > ./build/qemu-system-riscv64 -nographic -machine virt,dumpdtb=qemu.dtb > dt-validate -p > /path/to/linux/kernel/Documentation/devicetree/bindings/processed-schema.json > qemu.dtb > (The processed schema needs to be generated first) > > 0 - > https://lore.kernel.org/linux-riscv/20220805162844.1554247-1-m...@conchuod.ie/ > 1 - > https://lore.kernel.org/linux-riscv/20220803170552.ga2250266-r...@kernel.org/ > > Conor Dooley (4): > hw/riscv: virt: fix uart node name > hw/riscv: virt: fix the plic's address cells > hw/riscv: virt: fix syscon subnode paths > hw/core: fix platform bus node name Thanks! Applied to riscv-to-apply.next Alistair > > hw/core/sysbus-fdt.c| 2 +- > hw/riscv/virt.c | 8 +--- > include/hw/riscv/virt.h | 1 + > 3 files changed, 7 insertions(+), 4 deletions(-) > > > base-commit: 2480f3bbd03814b0651a1f74959f5c6631ee5819 > -- > 2.37.1 > >
Re: [PATCH v1] target/riscv: Add xicondops in ISA entry
On Tue, Aug 16, 2022 at 2:54 PM Rahul Pathak wrote:
>
> XVentanaCondOps is Ventana custom extension. Add
> its extension entry in the ISA Ext array
>
> Signed-off-by: Rahul Pathak
Thanks!
Applied to riscv-to-apply.next
Alistair
> ---
>
> This patch is based on branch riscv-to-apply.next (Alistair qemu tree)
> Based on top commit: f2a91d8b78
>
> target/riscv/cpu.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 2498b93105..27d10bd6a6 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -104,6 +104,7 @@ static const struct isa_ext_data isa_edata_arr[] = {
> ISA_EXT_DATA_ENTRY(svinval, true, PRIV_VERSION_1_12_0, ext_svinval),
> ISA_EXT_DATA_ENTRY(svnapot, true, PRIV_VERSION_1_12_0, ext_svnapot),
> ISA_EXT_DATA_ENTRY(svpbmt, true, PRIV_VERSION_1_12_0, ext_svpbmt),
> +ISA_EXT_DATA_ENTRY(xventanacondops, true, PRIV_VERSION_1_12_0,
> ext_XVentanaCondOps),
> };
>
> static bool isa_ext_is_enabled(RISCVCPU *cpu,
> --
> 2.34.1
>
>
Re: [PATCH v3 2/4] hw/ssi: ibex_spi: fixup coverity issue
On Mon, Aug 22, 2022 at 9:53 AM Wilfred Mallawa
wrote:
>
> From: Wilfred Mallawa
>
> This patch addresses the coverity issues specified in [1],
> as suggested, `FIELD_DP32()`/`FIELD_EX32()` macros have been
> implemented to clean up the code.
>
> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg887713.html
>
> Fixes: Coverity CID 1488107
>
> Signed-off-by: Wilfred Mallawa
> Reviewed-by: Andrew Jones
> ---
> hw/ssi/ibex_spi_host.c | 130 +
> 1 file changed, 66 insertions(+), 64 deletions(-)
>
> diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c
> index 601041d719..1298664d2b 100644
> --- a/hw/ssi/ibex_spi_host.c
> +++ b/hw/ssi/ibex_spi_host.c
> @@ -108,18 +108,20 @@ static inline uint8_t div4_round_up(uint8_t dividend)
>
> static void ibex_spi_rxfifo_reset(IbexSPIHostState *s)
> {
> +uint32_t data = s->regs[IBEX_SPI_HOST_STATUS];
> /* Empty the RX FIFO and assert RXEMPTY */
> fifo8_reset(&s->rx_fifo);
> -s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXFULL_MASK;
> -s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_RXEMPTY_MASK;
> +data = FIELD_DP32(data, STATUS, RXEMPTY, 1);
Doesn't this no longer clear the RXFULL bits?
> +s->regs[IBEX_SPI_HOST_STATUS] = data;
> }
>
> static void ibex_spi_txfifo_reset(IbexSPIHostState *s)
> {
> +uint32_t data = s->regs[IBEX_SPI_HOST_STATUS];
> /* Empty the TX FIFO and assert TXEMPTY */
> fifo8_reset(&s->tx_fifo);
> -s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXFULL_MASK;
> -s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_TXEMPTY_MASK;
> +data = FIELD_DP32(data, STATUS, TXEMPTY, 1);
Same here about TXFULL
Otherwise the patch looks good
Alistair
> +s->regs[IBEX_SPI_HOST_STATUS] = data;
> }
>
> static void ibex_spi_host_reset(DeviceState *dev)
> @@ -162,37 +164,38 @@ static void ibex_spi_host_reset(DeviceState *dev)
> */
> static void ibex_spi_host_irq(IbexSPIHostState *s)
> {
> -bool error_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE]
> -& R_INTR_ENABLE_ERROR_MASK;
> -bool event_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE]
> -& R_INTR_ENABLE_SPI_EVENT_MASK;
> -bool err_pending = s->regs[IBEX_SPI_HOST_INTR_STATE]
> -& R_INTR_STATE_ERROR_MASK;
> -bool status_pending = s->regs[IBEX_SPI_HOST_INTR_STATE]
> -& R_INTR_STATE_SPI_EVENT_MASK;
> +uint32_t intr_test_reg = s->regs[IBEX_SPI_HOST_INTR_TEST];
> +uint32_t intr_en_reg = s->regs[IBEX_SPI_HOST_INTR_ENABLE];
> +uint32_t intr_state_reg = s->regs[IBEX_SPI_HOST_INTR_STATE];
> +
> +uint32_t err_en_reg = s->regs[IBEX_SPI_HOST_ERROR_ENABLE];
> +uint32_t event_en_reg = s->regs[IBEX_SPI_HOST_EVENT_ENABLE];
> +uint32_t err_status_reg = s->regs[IBEX_SPI_HOST_ERROR_STATUS];
> +uint32_t status_reg = s->regs[IBEX_SPI_HOST_STATUS];
> +
> +
> +bool error_en = FIELD_EX32(intr_en_reg, INTR_ENABLE, ERROR);
> +bool event_en = FIELD_EX32(intr_en_reg, INTR_ENABLE, SPI_EVENT);
> +bool err_pending = FIELD_EX32(intr_state_reg, INTR_STATE, ERROR);
> +bool status_pending = FIELD_EX32(intr_state_reg, INTR_STATE, SPI_EVENT);
> +
> int err_irq = 0, event_irq = 0;
>
> /* Error IRQ enabled and Error IRQ Cleared */
> if (error_en && !err_pending) {
> /* Event enabled, Interrupt Test Error */
> -if (s->regs[IBEX_SPI_HOST_INTR_TEST] & R_INTR_TEST_ERROR_MASK) {
> +if (FIELD_EX32(intr_test_reg, INTR_TEST, ERROR)) {
> err_irq = 1;
> -} else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE]
> -& R_ERROR_ENABLE_CMDBUSY_MASK) &&
> -s->regs[IBEX_SPI_HOST_ERROR_STATUS]
> -& R_ERROR_STATUS_CMDBUSY_MASK) {
> +} else if (FIELD_EX32(err_en_reg, ERROR_ENABLE, CMDBUSY) &&
> + FIELD_EX32(err_status_reg, ERROR_STATUS, CMDBUSY)) {
> /* Wrote to COMMAND when not READY */
> err_irq = 1;
> -} else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE]
> -& R_ERROR_ENABLE_CMDINVAL_MASK) &&
> -s->regs[IBEX_SPI_HOST_ERROR_STATUS]
> -& R_ERROR_STATUS_CMDINVAL_MASK) {
> +} else if (FIELD_EX32(err_en_reg, ERROR_ENABLE, CMDINVAL) &&
> + FIELD_EX32(err_status_reg, ERROR_STATUS, CMDINVAL)) {
> /* Invalid command segment */
> err_irq = 1;
> -} else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE]
> -& R_ERROR_ENABLE_CSIDINVAL_MASK) &&
> -s->regs[IBEX_SPI_HOST_ERROR_STATUS]
> -& R_ERROR_STATUS_CSIDINVAL_MASK) {
> +} else if (FIELD_EX32(err_en_reg, ERROR_ENABLE, CSIDINVAL) &&
> + FIELD_EX32(err_status_reg, ERROR_STATUS, CSIDINVAL)) {
> /* Invalid value for CSID */
> err_irq = 1;
> }
> @@ -204,22 +207,19 @@ static void ibex_spi_host_irq(IbexSPIHo
Re: [PATCH v3] target/riscv: Use official extension names for AIA CSRs
On Sat, Aug 20, 2022 at 2:30 PM Anup Patel wrote:
>
> The arch review of AIA spec is completed and we now have official
> extension names for AIA: Smaia (M-mode AIA CSRs) and Ssaia (S-mode
> AIA CSRs).
>
> Refer, section 1.6 of the latest AIA v0.3.1 stable specification at
> https://github.com/riscv/riscv-aia/releases/download/0.3.1-draft.32/riscv-interrupts-032.pdf)
>
> Based on above, we update QEMU RISC-V to:
> 1) Have separate config options for Smaia and Ssaia extensions
>which replace RISCV_FEATURE_AIA in CPU features
> 2) Not generate AIA INTC compatible string in virt machine
>
> Signed-off-by: Anup Patel
> Reviewed-by: Andrew Jones
Reviewed-by: Alistair Francis
Alistair
> ---
> Changes since v2:
> - Use env_archcpu() to get RISCVCPU * from CPURISCVState *
> Changes since v1:
> - Remove redundant "has_aia" parameter from riscv_cpu_pending_to_irq()
> ---
> hw/intc/riscv_imsic.c | 4 +++-
> hw/riscv/virt.c | 13 ++---
> target/riscv/cpu.c| 9 -
> target/riscv/cpu.h| 4 ++--
> target/riscv/cpu_helper.c | 3 ++-
> target/riscv/csr.c| 24 ++--
> 6 files changed, 31 insertions(+), 26 deletions(-)
>
> diff --git a/hw/intc/riscv_imsic.c b/hw/intc/riscv_imsic.c
> index 8615e4cc1d..4d4d5b50ca 100644
> --- a/hw/intc/riscv_imsic.c
> +++ b/hw/intc/riscv_imsic.c
> @@ -344,9 +344,11 @@ static void riscv_imsic_realize(DeviceState *dev, Error
> **errp)
>
> /* Force select AIA feature and setup CSR read-modify-write callback */
> if (env) {
> -riscv_set_feature(env, RISCV_FEATURE_AIA);
> if (!imsic->mmode) {
> +rcpu->cfg.ext_ssaia = true;
> riscv_cpu_set_geilen(env, imsic->num_pages - 1);
> +} else {
> +rcpu->cfg.ext_smaia = true;
> }
> riscv_cpu_set_aia_ireg_rmw_fn(env, (imsic->mmode) ? PRV_M : PRV_S,
>riscv_imsic_rmw, imsic);
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index e779d399ae..b041b33afc 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -261,17 +261,8 @@ static void create_fdt_socket_cpus(RISCVVirtState *s,
> int socket,
> qemu_fdt_add_subnode(mc->fdt, intc_name);
> qemu_fdt_setprop_cell(mc->fdt, intc_name, "phandle",
> intc_phandles[cpu]);
> -if (riscv_feature(&s->soc[socket].harts[cpu].env,
> - RISCV_FEATURE_AIA)) {
> -static const char * const compat[2] = {
> -"riscv,cpu-intc-aia", "riscv,cpu-intc"
> -};
> -qemu_fdt_setprop_string_array(mc->fdt, intc_name, "compatible",
> - (char **)&compat, ARRAY_SIZE(compat));
> -} else {
> -qemu_fdt_setprop_string(mc->fdt, intc_name, "compatible",
> -"riscv,cpu-intc");
> -}
> +qemu_fdt_setprop_string(mc->fdt, intc_name, "compatible",
> +"riscv,cpu-intc");
> qemu_fdt_setprop(mc->fdt, intc_name, "interrupt-controller", NULL,
> 0);
> qemu_fdt_setprop_cell(mc->fdt, intc_name, "#interrupt-cells", 1);
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index d3fbaa..3cf0c86661 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -101,6 +101,8 @@ static const struct isa_ext_data isa_edata_arr[] = {
> ISA_EXT_DATA_ENTRY(zve64f, true, PRIV_VERSION_1_12_0, ext_zve64f),
> ISA_EXT_DATA_ENTRY(zhinx, true, PRIV_VERSION_1_12_0, ext_zhinx),
> ISA_EXT_DATA_ENTRY(zhinxmin, true, PRIV_VERSION_1_12_0, ext_zhinxmin),
> +ISA_EXT_DATA_ENTRY(smaia, true, PRIV_VERSION_1_12_0, ext_smaia),
> +ISA_EXT_DATA_ENTRY(ssaia, true, PRIV_VERSION_1_12_0, ext_ssaia),
> ISA_EXT_DATA_ENTRY(sscofpmf, true, PRIV_VERSION_1_12_0, ext_sscofpmf),
> ISA_EXT_DATA_ENTRY(sstc, true, PRIV_VERSION_1_12_0, ext_sstc),
> ISA_EXT_DATA_ENTRY(svinval, true, PRIV_VERSION_1_12_0, ext_svinval),
> @@ -669,10 +671,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error
> **errp)
> }
> }
>
> -if (cpu->cfg.aia) {
> -riscv_set_feature(env, RISCV_FEATURE_AIA);
> -}
> -
> if (cpu->cfg.debug) {
> riscv_set_feature(env, RISCV_FEATURE_DEBUG);
> }
> @@ -1058,7 +1056,8 @@ static Property riscv_cpu_extensions[] = {
> DEFINE_PROP_BOOL("x-j", RISCVCPU, cfg.ext_j, false),
> /* ePMP 0.9.3 */
> DEFINE_PROP_BOOL("x-epmp", RISCVCPU, cfg.epmp, false),
> -DEFINE_PROP_BOOL("x-aia", RISCVCPU, cfg.aia, false),
> +DEFINE_PROP_BOOL("x-smaia", RISCVCPU, cfg.ext_smaia, false),
> +DEFINE_PROP_BOOL("x-ssaia", RISCVCPU, cfg.ext_ssaia, false),
>
> DEFINE_PROP_END_OF_LIST(),
> };
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 42edfa4558..15cad73def 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -85,7 +85,6 @@ enum {
> RISCV_FEATURE_PMP,
> RISCV_FEATURE_EPMP,
> RISCV_FEATURE_MISA,
> -RISCV_FEA
Re: [PATCH v3 2/4] hw/ssi: ibex_spi: fixup coverity issue
On Mon, 2022-08-22 at 13:42 +1000, Alistair Francis wrote:
> On Mon, Aug 22, 2022 at 9:53 AM Wilfred Mallawa
> wrote:
> >
> > From: Wilfred Mallawa
> >
> > This patch addresses the coverity issues specified in [1],
> > as suggested, `FIELD_DP32()`/`FIELD_EX32()` macros have been
> > implemented to clean up the code.
> >
> > [1]
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg887713.html
> >
> > Fixes: Coverity CID 1488107
> >
> > Signed-off-by: Wilfred Mallawa
> > Reviewed-by: Andrew Jones
> > ---
> > hw/ssi/ibex_spi_host.c | 130 +
> >
> > 1 file changed, 66 insertions(+), 64 deletions(-)
> >
> > diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c
> > index 601041d719..1298664d2b 100644
> > --- a/hw/ssi/ibex_spi_host.c
> > +++ b/hw/ssi/ibex_spi_host.c
> > @@ -108,18 +108,20 @@ static inline uint8_t div4_round_up(uint8_t
> > dividend)
> >
> > static void ibex_spi_rxfifo_reset(IbexSPIHostState *s)
> > {
> > + uint32_t data = s->regs[IBEX_SPI_HOST_STATUS];
> > /* Empty the RX FIFO and assert RXEMPTY */
> > fifo8_reset(&s->rx_fifo);
> > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXFULL_MASK;
> > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_RXEMPTY_MASK;
> > + data = FIELD_DP32(data, STATUS, RXEMPTY, 1);
>
> Doesn't this no longer clear the RXFULL bits?
>
> > + s->regs[IBEX_SPI_HOST_STATUS] = data;
> > }
> >
> > static void ibex_spi_txfifo_reset(IbexSPIHostState *s)
> > {
> > + uint32_t data = s->regs[IBEX_SPI_HOST_STATUS];
> > /* Empty the TX FIFO and assert TXEMPTY */
> > fifo8_reset(&s->tx_fifo);
> > - s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXFULL_MASK;
> > - s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_TXEMPTY_MASK;
> > + data = FIELD_DP32(data, STATUS, TXEMPTY, 1);
>
> Same here about TXFULL
>
> Otherwise the patch looks good
>
> Alistair
>
Good catch! I'll fix these up.
Wilfred
> > + s->regs[IBEX_SPI_HOST_STATUS] = data;
> > }
> >
> > static void ibex_spi_host_reset(DeviceState *dev)
> > @@ -162,37 +164,38 @@ static void ibex_spi_host_reset(DeviceState
> > *dev)
> > */
> > static void ibex_spi_host_irq(IbexSPIHostState *s)
> > {
> > - bool error_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE]
> > - & R_INTR_ENABLE_ERROR_MASK;
> > - bool event_en = s->regs[IBEX_SPI_HOST_INTR_ENABLE]
> > - & R_INTR_ENABLE_SPI_EVENT_MASK;
> > - bool err_pending = s->regs[IBEX_SPI_HOST_INTR_STATE]
> > - & R_INTR_STATE_ERROR_MASK;
> > - bool status_pending = s->regs[IBEX_SPI_HOST_INTR_STATE]
> > - & R_INTR_STATE_SPI_EVENT_MASK;
> > + uint32_t intr_test_reg = s->regs[IBEX_SPI_HOST_INTR_TEST];
> > + uint32_t intr_en_reg = s->regs[IBEX_SPI_HOST_INTR_ENABLE];
> > + uint32_t intr_state_reg = s->regs[IBEX_SPI_HOST_INTR_STATE];
> > +
> > + uint32_t err_en_reg = s->regs[IBEX_SPI_HOST_ERROR_ENABLE];
> > + uint32_t event_en_reg = s->regs[IBEX_SPI_HOST_EVENT_ENABLE];
> > + uint32_t err_status_reg = s->regs[IBEX_SPI_HOST_ERROR_STATUS];
> > + uint32_t status_reg = s->regs[IBEX_SPI_HOST_STATUS];
> > +
> > +
> > + bool error_en = FIELD_EX32(intr_en_reg, INTR_ENABLE, ERROR);
> > + bool event_en = FIELD_EX32(intr_en_reg, INTR_ENABLE,
> > SPI_EVENT);
> > + bool err_pending = FIELD_EX32(intr_state_reg, INTR_STATE,
> > ERROR);
> > + bool status_pending = FIELD_EX32(intr_state_reg, INTR_STATE,
> > SPI_EVENT);
> > +
> > int err_irq = 0, event_irq = 0;
> >
> > /* Error IRQ enabled and Error IRQ Cleared */
> > if (error_en && !err_pending) {
> > /* Event enabled, Interrupt Test Error */
> > - if (s->regs[IBEX_SPI_HOST_INTR_TEST] &
> > R_INTR_TEST_ERROR_MASK) {
> > + if (FIELD_EX32(intr_test_reg, INTR_TEST, ERROR)) {
> > err_irq = 1;
> > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE]
> > - & R_ERROR_ENABLE_CMDBUSY_MASK) &&
> > - s->regs[IBEX_SPI_HOST_ERROR_STATUS]
> > - & R_ERROR_STATUS_CMDBUSY_MASK) {
> > + } else if (FIELD_EX32(err_en_reg, ERROR_ENABLE, CMDBUSY)
> > &&
> > + FIELD_EX32(err_status_reg, ERROR_STATUS,
> > CMDBUSY)) {
> > /* Wrote to COMMAND when not READY */
> > err_irq = 1;
> > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE]
> > - & R_ERROR_ENABLE_CMDINVAL_MASK) &&
> > - s->regs[IBEX_SPI_HOST_ERROR_STATUS]
> > - & R_ERROR_STATUS_CMDINVAL_MASK) {
> > + } else if (FIELD_EX32(err_en_reg, ERROR_ENABLE,
> > CMDINVAL) &&
> > + FIELD_EX32(err_status_reg, ERROR_STATUS,
> > CMDINVAL)) {
> > /* Invalid command segment */
> > err_irq = 1;
> > - } else if ((s->regs[IBEX_SPI_HOST_ERROR_ENABLE]
> > - & R_ERROR_ENABLE_CSIDINVAL_MASK) &&
> > - s->regs[IBE
Re: [RFC PATCH v2 4/8] qapi: golang: Generate qapi's union types in Go
Hi,
On Fri, Aug 19, 2022 at 10:25:26AM -0500, Andrea Bolognani wrote:
> > func (s QCryptoBlockOpenOptions) MarshalJSON() ([]byte, error) {
> > var bytes []byte
> > var err error
> > if s.Qcow != nil {
> > tmp := struct {
> > QCryptoBlockOptionsQCow
> > Discriminator string `json:"format"`
> > }{
> > QCryptoBlockOptionsQCow: *s.Qcow,
> > Discriminator: "qcow",
> > }
> > if bytes, err = json.Marshal(tmp); err != nil {
> > return nil, err
> > }
> > }
> > if s.Luks != nil {
> > if len(bytes) != 0 {
> > return nil, errors.New(`multiple fields set for
> > QCryptoBlockOpenOptions`)
> > }
>
> Why are you checking this here? I would just have a check like
>
> if s.Qcow != nil && s.Luks != nil {
> return nil, errors.New(`multiple fields set for
> QCryptoBlockOpenOptions`)
> }
>
> at the top of the function, so that you can abort early and
> cheaply if the user has provided invalid input instead of
> having to wait after one of the fields has already been
> serialized.
In general, I too prefer to return early! So I agree with you
that it is nicer. At the same time, I was thinking a bit from the
code generator point of view and the overall expected diffs when
generating new code. More below.
> > tmp := struct {
> > QCryptoBlockOptionsLUKS
> > Discriminator string `json:"format"`
> > }{
> > QCryptoBlockOptionsLUKS: *s.Luks,
> > Discriminator: "luks",
> > }
> > if bytes, err = json.Marshal(tmp); err != nil {
> > return nil, err
> > }
> > }
> > if len(bytes) == 0 {
> > return nil, errors.New(`null not supported for
> > QCryptoBlockOpenOptions`)
> > }
>
> Similarly, this should be checked early. So the complete check
> could be turned into
>
> if (s.Qcow != nil && s.Luks != nil) || (s.Qcow == nil && s.Luks == nil) {
> return nil, errors.New("must set exactly one field")
> }
Main problem with this approach is that there is some big unions
like BlockdevOptions, this means that we would have to repeat all
fields by the number fields times (40+ in BlockdevOptions' case).
> You should have enough information to produce such a check when
> generating the function, right?
We do!
> If making sure all possible combinations are covered up ends up
> being too complicated, something
> like
>
> var setFields int
> if s.Field1 != nil {
> setFields += 1
> }
> if s.Field2 != nil {
> setFields += 1
> }
> // ...
> if setFields != 1 {
> return nil, errors.New("must set exactly one field")
> }
>
> would also work.
I started with this approach actually but then I realized that we
can just keep the if checks and instead of counter, do check the
variable bytes []byte. So, do you think that the counter is
actually preferred instead of inner check? I don't mind changing
it.
> > func (s *QCryptoBlockOpenOptions) UnmarshalJSON(data []byte) error {
> > tmp := struct {
> > Discriminator string `json:"format"`
> > }{}
> > if err := json.Unmarshal(data, &tmp); err != nil {
> > return err
> > }
> > switch tmp.Discriminator {
> > case "qcow":
> > s.Qcow = &QCryptoBlockOptionsQCow{}
> > if err := json.Unmarshal(data, s.Qcow); err != nil {
> > s.Qcow = nil
> > return err
> > }
> > case "luks":
> > s.Luks = &QCryptoBlockOptionsLUKS{}
> > if err := json.Unmarshal(data, s.Luks); err != nil {
> > s.Luks = nil
> > return err
> > }
> > }
> > return nil
> > }
>
> Alternatively, you could define a struct that covers all
> possible fields and deserialize everything in one go, then copy
> the various parts over to the appropriate struct:
>
> func (s *QCryptoBlockOpenOptions) UnmarshalJSON(data []byte) error {
> tmp := struct {
> Discriminator string `json:"format"`
> KeySecret *string `json:"key-secret,omitempty"`
> }{}
> if err := json.Unmarshal(data, &tmp); err != nil {
> return err
> }
> switch tmp.Discriminator {
> case "qcow":
> s.Qcow = &QCryptoBlockOptionsQCow{}
> s.Qcow.KeySecret = tmp.KeySecret
> case "luks":
> s.Luks = &QCryptoBlockOptionsLUKS{}
> s.Luks.KeySecret = tmp.KeySecret
I think this one adds a bit more complexity and I'm not convinced
that the gains are worth.
For example, with types that differ over fields with the same
name? We would need to move that inside the anonymous struct
somehow;
For example,if Luks's KeySecret was KeyScret
