Conversation
i2h2
force-pushed
the
imsic
branch
3 times, most recently
from
b717fbd to
021b6a0
Compare
|
@binno @arrv-sc @chihminchao can y'all lend a hand reviewing this large but useful PR? |
AIA: add hgeie/hgeip CSRs
aia_ireg_proxy_csr_t ignores writes and reads 0 by default
Access to stopei and siselect of 0x70--0xff in S mode should raise an illegal instruction exception.
arromanoff
left a comment
arromanoff
left a comment
There was a problem hiding this comment.
I would appreciate if someone else would also take a look at this. As I am not very familiar with APLIC
| if (!eiid || it == procs.end()) | ||
| return; |
There was a problem hiding this comment.
Shouldn't this be an internal error/assert instead of a NOP?
| val = target[idx]; | ||
| } | ||
|
|
||
| memcpy(bytes, (uint8_t *)&val, len); |
There was a problem hiding this comment.
[nit] uint8_t * cast inside memcpy is unnecessary as both source and destination accept void * and any pointer implicitly casts to void *
| sourcecfg[idx] = val; | ||
| } | ||
| } else if (addr >= SETIP_BASE && addr < SETIPNUM) { | ||
| auto idx = (addr - SETIP_BASE) / 4; |
There was a problem hiding this comment.
This 4 keeps appearing throughout this function. Can we name it somehow? constexpr variable or define would be nice.
| if (proc->extension_enabled_const(EXT_SSAIA)) { | ||
| add_hypervisor_csr(CSR_HGEIE, hgeie = std::make_shared<hgeie_csr_t>(proc, CSR_HGEIE, proc->geilen)); | ||
| add_hypervisor_csr(CSR_HGEIP, hgeip = std::make_shared<hgeip_csr_t>(proc, CSR_HGEIP)); | ||
| } else { | ||
| add_hypervisor_csr(CSR_HGEIE, std::make_shared<const_csr_t>(proc, CSR_HGEIE, 0)); | ||
| add_hypervisor_csr(CSR_HGEIP, std::make_shared<const_csr_t>(proc, CSR_HGEIP, 0)); | ||
| } |
There was a problem hiding this comment.
Q: Why don't we set hgeie/hgeip members under else branch?
| typedef std::shared_ptr<imsic_file_t> imsic_file_t_p; | ||
| class topei_csr_t: public csr_t { | ||
| public: | ||
| topei_csr_t(processor_t* const proc, const reg_t addr, imsic_file_t_p const imsic); |
There was a problem hiding this comment.
[nit][here and around] I am not a fan of moving trivial constructors to the .cc files. It does not reduce number of dependencies but negatively affects readability and prevents this function from being inlined. I don't think it's critical, however, as we already do that a lot.
| uint32_t deleg_mask[APLIC_MAX_DEVICES / 32]; | ||
|
|
||
| bool interrupt_enabled(uint32_t id); | ||
| bool accessible(uint32_t id); |
There was a problem hiding this comment.
| bool accessible(uint32_t id); | |
| bool accessible(uint32_t id) const; |
| // deleg_mask can be directly applied to other packed registers | ||
| uint32_t deleg_mask[APLIC_MAX_DEVICES / 32]; | ||
|
|
||
| bool interrupt_enabled(uint32_t id); |
There was a problem hiding this comment.
| bool interrupt_enabled(uint32_t id); | |
| bool interrupt_enabled(uint32_t id) const; |
| const fdt32_t *p; | ||
| int val; | ||
|
|
||
| for (int i = 0; i < 2; i++) { |
There was a problem hiding this comment.
What does this 2 represent? It is hard to read/maintain this code without knowing every part of APLIC spec. Please add a constexpr variable/define. Such constants deserve to be named.
| m = std::make_shared<imsic_file_t>(proc, MIP_MEIP, IMSIC_M_FILE_REGS); | ||
| if (proc->extension_enabled_const(EXT_SSAIA)) { | ||
| s = std::make_shared<imsic_file_t>(proc, MIP_SEIP, IMSIC_S_FILE_REGS); | ||
| assert(geilen <= 63); |
There was a problem hiding this comment.
[nitpick] This would be much more readable:
| assert(geilen <= 63); | |
| assert(geilen < 64); |
| if (dynamic_cast<aplic_m_t*>(&*dev_ptr)) { | ||
| assert(!aplic_m); | ||
| aplic_m = std::static_pointer_cast<aplic_m_t>(dev_ptr); | ||
| } |
There was a problem hiding this comment.
| if (dynamic_cast<aplic_m_t*>(&*dev_ptr)) { | |
| assert(!aplic_m); | |
| aplic_m = std::static_pointer_cast<aplic_m_t>(dev_ptr); | |
| } | |
| if (auto *aplic_m_new = dynamic_cast<aplic_m_t*>(&*dev_ptr)) { | |
| assert(!aplic_m); | |
| aplic_m = aplic_m_new; | |
| } |
3 participants
Boots SMP Linux as kvm guest with VS IMSIC interrupts. Dependent on riscv/riscv-opcodes#354
@binno @glg-rv