Critique of #1853: the new `get_csrs` extension method is unnecessary, and has negative side effect

[liuyu81]

PR #1853 added a new get_csrs method to the public interface of extension_t, claiming to solve the alleged problem that "adding custom CSR's from extension modules was impossible". This was a false claim -- we don't need a new method for this (explained below) -- worse still, PR #1853 introduced a negative side effect causing changed CSR states to survive processor reset.

Before #1853, it was indeed possible to add custom CSR's from the extension_t::reset() method, i.e.,

void my_extension_t::reset() {
    this->p->get_state()->add_csr(...);
}

This does not require changing the public interface of extensions. And it ensures custom CSR's resume to their default states upon processor reset. We should keep it the old way.

In fact, all standard CSR's are being (re-)added during processor reset (call path processor_t::reset() -> state_t::reset() -> csr_init()). When we add custom CSR's from extension_t::reset, they equally have a chance of being reset (re-added) during processor reset (call path processor_t::reset() -> extension_t::reset()).

IMHO, what we actually need is an example of adding custom CSR's from extensions, and CI tests to make sure this works (PR #1853 has its merits in this regard). But changes to extension.h and processor.cc should perhaps be reverted.

[liuyu81]

Also note that, the initialization of custom CSR's may require access to the processor instance (i.e. to inspect a relevant feature / state of the processor). This is why there is already extension_t::set_processor() -> extension_t::p for the extension to access processor_t *.

But the new get_csrs() method chose to take another processor_t & as input. From the PR's implementation, I learned that this was a necessary hack, because get_csrs() is invoked before extension_t::set_processor(), thus extension_t::p is nullptr from within get_csrs().

But this all smells too hacky for a public interface between spike and extensions; and it most likely will confuse extension developers who knows about extension_t::p.

[jerryz123]

I agree with your critique of #1853 breaking the CSR initialization pattern. As a solution, processor_t->reset() should instead call extension_t->get_csrs() and add those CSRs to the CSR map after csr_init?.

I disagree that extension_t->reset() is a sufficient interface. It is not obvious that extension_t->reset can modify other fields of state_t (and perhaps, this is a bad pattern to follow anyways).

[liuyu81]

If an extension introduces custom CSR's to the processor, isn't it by definition modifying (extending) the processor's state?

The bright side of extension_t::reset() is that its name explicitly suggests that anything with internal states should be re-initialized from within it. The get_csrs() method, on the contrary, suggests a one-time thing, expecially when you also have get_instructions() and get_disasms().

With your proposed solution, extensions developers would have to dig into spike's internal implementation (not just public interfaces) to find out that get_csrs() will get called during processor reset (thus they can access extension_t::p from it), yet get_instructions() will get called upon initialization (with extension_t::p uninitialized). IMO, this difference in semantics would perhapse increase the burden of cognition for extension developers.

[jerryz123]

If an extension introduces custom CSR's to the processor, isn't it by definition modifying (extending) the processor's state?

Yes, but that doesn't mean that the extension_t->reset interface should be allowed to make arbitrary changes to processor_t and state_t.

With your proposed solution, extensions developers would have to dig into spike's internal implementation (not just public interfaces) to find out that get_csrs() will get called during processor reset (thus they can access extension_t::p from it),

I don't quite understand this critique, processor_t* is passed toget_csrs, so it should be clear to anyone implementing get_csrs that they can access members of processor_t. Additionally, the obvious approach to implementing get_csrs would result in the right behavior, even if the developer does not think about the behavior of processor_t->reset().

I've implemented #1864 for now

[liuyu81]

With your proposed solution, extensions developers would have to dig into spike's internal implementation (not just public interfaces) to find out that get_csrs() will get called during processor reset (thus they can access extension_t::p from it),

I don't quite understand this critique, processor_t* is passed toget_csrs, so it should be clear to anyone implementing get_csrs that they can access members of processor_t. Additionally, the obvious approach to implementing get_csrs would result in the right behavior, even if the developer does not think about the behavior of processor_t->reset().

I've implemented #1864 for now

Hi @jerryz123, this is more of a call-for-clarification than critique. You see, there is already extension_t::p which is a processor_t * initialized during extension registration (set through extension_t::set_processor). I was under the impression that this extension_t::p is the implied way to access / modify processor states from within a custom extension. It may not be a good design in terms of explicitness, but it was what we had, and there are people's extension modules relying on it.

Then this get_csrs() comes in, explicitly taking a reference to processor_t from its input. Shouldn't we stick to using extension_t::p to maintain consistency? Or, if we prefer explicitness to consistency, should we also refactor extension_t::reset() to take processor_t from input, and perhaps remove extension_t::set_processor and extension_t::p altogether?

p.s. #1864 looks good to me.

[jerryz123]

I think I just underestimate the complexity of extension_t that people are developing. Non-CSR architectural state that is only available to the extension can just be a member of the extension_t implementation. CSRs are passed as shared pointers (csr_t_p) to the csr registration code that populates/reset csrmap. I'd be interested in hearing about the more advanced uses of the extension_t interface, if you are free to share.

It does seem likeextension_t::p is unnecessary. The function signature for instructions already passes processor_t*.

Or, if we prefer explicitness to consistency, should we also refactor extension_t::reset() to take processor_t from input, and perhaps remove extension_t::set_processor and extension_t::p altogether?

This approach seems best to me.

[liuyu81]

It does seem likeextension_t::p is unnecessary. The function signature for instructions already passes processor_t*.

Or, if we prefer explicitness to consistency, should we also refactor extension_t::reset() to take processor_t from input, and perhaps remove extension_t::set_processor and extension_t::p altogether?

This approach seems best to me.

Looks like extension_t::set_processor and extension_t::p were initially added for RoCC (commit bbb0f21), and are now being used by extension_t::raise_interrupt for "interrupt delegation for co-processors" (commit 9e01246). I am not quite sure as to how these are currently being used in the wild. But I like the idea of dropping extension_t::p for explicitness.

I think I just underestimate the complexity of extension_t that people are developing. Non-CSR architectural state that is only available to the extension can just be a member of the extension_t implementation. CSRs are passed as shared pointers (csr_t_p) to the csr registration code that populates/reset csrmap. I'd be interested in hearing about the more advanced uses of the extension_t interface, if you are free to share.

In my case, we sometimes want to overwrite spike's builtin instruction / CSR implementations through extension modules to align the behaviors of SIM to RTL. Take #528 for example, we want sc.d to fail if the address was reserved by a mismatching lr.w, or vice versa (not implemented as such in spike, but valid per spec). For this, we created an extension module that remembers reserved addresses in extension states (separately for 32bit and 64bit lr / sc pairs). I guess this qualifies as a complex use case, because we have custom (overridden) instructions with additional states maintained by the extension.

p.s. I have created a Python-binding for Spike. It allows users to write ISA extensions and MMIO device models in Python, and plug them into Spike through --extlib. In case you'd like to check it out, https://github.com/liuyu81/pyspike.

[aap-sc]

@arrv-sc FYI

[jerryz123]

In my case, we sometimes want to overwrite spike's builtin instruction / CSR implementations through extension modules to align the behaviors of SIM to RTL.

Ok, this is an interesting use case. I assumed all uses of extension_t were simply adding new instructions or CSRs, not modifying the behavior of standard instructions.

[liuyu81]

It does seem like extension_t::p is unnecessary. The function signature for instructions already passes processor_t*.

Or, if we prefer explicitness to consistency, should we also refactor extension_t::reset() to take processor_t from input, and perhaps remove extension_t::set_processor and extension_t::p altogether?

This approach seems best to me.

Hi @jerryz123, do you think it's time to take on this approach to refactoring the public interface of extension_t (i.e. remove extension_t::p and change reset() to reset(processor_t&) ? I am pro explicitness, but am also worrying about the compatibility issues this may cause to existing extension modules. Especially, could you shed some light on how to deal with extension_t::raise_interrupt that internally depends on extension_t::p?

[arromanoff]

Hi! I like the Idea of re-inserting csrs on reset as in @jerryz123 's commit. I proposed the same thing in the comments PR discussed but unfortunately didn't get to implementing it at the time.

Or, if we prefer explicitness to consistency, should we also refactor extension_t::reset() to take processor_t from input, and
perhaps remove extension_t::set_processor and extension_t::p altogether?

I like this approach and its explicitness. If we are not afraid of compatibility issues I am willing to implement the refactoring

[arromanoff]

Regarding extension_t::raise_interrupt I don't see how it is being used now. After commit 9e01246 processor_t does not call it anymore. I guess you can call raise_interrupt from within your custom extension though

[jerryz123]

Thanks, @arrv-sc , I'm happy to review again if you PR the refactor. I'm not concerned about compatibility issues.

Yes, the intention is for custom extensions to call raise_interrupt. Perhaps raise interrupt should take processor_t* as an argument? I believe extensions would call raise_interrupt from within their implementation of insn_func_t, which receives processor_t*.