[RACL] Add RACL support for windows and enable RACL for mbx and spi_host.

[davidschrammel]

This PR is based on #25973 and #26015 and adds RACL support for windows (in addition to registers) and it also enables RACL support for mbx and spi_host.

[rswarbrick]

This looks good to me. Goodness me, I wish we didn't check in the autogenerated files: the commits are enormous! :-)

[rswarbrick]

CHANGE AUTHORIZED: hw/ip/spi_host/data/spi_host.hjson
CHANGE AUTHORIZED: hw/ip/spi_host/rtl/spi_host.sv
CHANGE AUTHORIZED: hw/ip/spi_host/rtl/spi_host_reg_top.sv
CHANGE AUTHORIZED: hw/ip/spi_host/rtl/spi_host_window.sv
CHANGE AUTHORIZED: hw/ip/tlul/rtl/tlul_adapter_reg_racl.sv
CHANGE AUTHORIZED: hw/ip/tlul/rtl/tlul_adapter_sram_racl.sv
CHANGE AUTHORIZED: hw/top_earlgrey/rtl/autogen/top_earlgrey.sv

These changes seem safe to me:

• The spi_host change should be a no-op if RACL is disabled (because the two tlul_adapter_*_racl modules are transparent in that situation).
• Changes to the RACL adapters are just typo fixes and assertion tidyups.
• The change in top_earlgrey.sv is just stubbing off some new (unused) ports.

[alees24]

Disclaimer: These comments are purely from reading the RTL and there is every chance that at this early stage I have misunderstood or overlooked something.

Suspected RACL issues:

• in _reg_top the signal 'racl_error_o' is not appropriately qualified with the reg_re and reg_we;
  it will respond to non-register addresses arriving at this IP block; I think you want to respond
  to the control lines 'reg_re' and 'reg_we' and not TL-UL at all.

  The logic should presently be functional only because (i) the address decoding is somewhat
  redundant; in _reg_top the full IP address is decoded for each register access ('reg_addr == ')
  even if the registers occupy only small portion of the IP's address window, and (ii) there is
  no register stage delay in the tlul_adapter_reg(_racl) implementation. Using the TL-UL input
  makes the logic fragile.

• in _reg_top, a rejected write will not raise an error if reading from that register is permitted,
  and vice-versa; racl_write_addr_hit_read/write are not qualified by the access type.

• in _sram_racl the signal 'rd_req' is incompletely-qualified; it does not include the appropriate
  a_valid signal, nor a_ready.

• in _sram_racl, '~rd_req' is not equivalent to 'wr_req'...there may be no current request, so in
  the generation of racl_error_o it does not suffice simply to invert rd_req.

• in spi_host_reg_top the tlul_adapter_reg has not been changed to _reg_racl. Is that intentional at this stage?

• as a precaution it may be preferable to set the default 'RaclErrorRsp' to zero (or to EnableRacl)
  for auto-generated registers (_reg_top) since the racl_error_o signal is always driven.

• is the intention to rive racl_error_o and racl_error_log_o.racl_role (in _reg_top) even when EnableRacl is
  zero, i.e. no RACL support?

Lesser points:

• The second qualification of 'addr_hit & ( | )' in racl_error_o generation in _reg_top is harmless
  but unnecessary because 'racl_addr_hit_read|write[x]' already depends upon the 'addr_hit[x]'
  begin set.

• a_ready is not timed and qualified appropriately in tlul_request_loopback; this could matter if
  one TL-UL host has an outstanding request and then a request from another host is rejected on
  the grounds of RACL permissions.
  This may give issues with shadow registers too or CDC registers.

Approving to avoid blocking merges, because I am presently confident that this does not break existing designs.

[alees24]

CHANGE AUTHORIZED: hw/ip/spi_host/data/spi_host.hjson
CHANGE AUTHORIZED: hw/ip/spi_host/rtl/spi_host.sv
CHANGE AUTHORIZED: hw/ip/spi_host/rtl/spi_host_reg_top.sv
CHANGE AUTHORIZED: hw/ip/spi_host/rtl/spi_host_window.sv
CHANGE AUTHORIZED: hw/ip/tlul/rtl/tlul_adapter_reg_racl.sv
CHANGE AUTHORIZED: hw/ip/tlul/rtl/tlul_adapter_sram_racl.sv
CHANGE AUTHORIZED: hw/top_earlgrey/rtl/autogen/top_earlgrey.sv

[Razer6]

• in _reg_top the signal 'racl_error_o' is not appropriately qualified with the reg_re and reg_we;
  it will respond to non-register addresses arriving at this IP block; I think you want to respond
  to the control lines 'reg_re' and 'reg_we' and not TL-UL at all.

I'm confused about that. racl_error_o factors in addr_hit, shouldn't that prevent to respond to non-register addresses?

The logic should presently be functional only because (i) the address decoding is somewhat
redundant; in _reg_top the full IP address is decoded for each register access ('reg_addr == ')
even if the registers occupy only small portion of the IP's address window, and (ii) there is
no register stage delay in the tlul_adapter_reg(_racl) implementation. Using the TL-UL input
makes the logic fragile.

I agree, gating racl_error_o with a_valid and addr_hit is fragile and only works because the reg_adapter is not staged and a_valid is asserted in the same cycle as the computation of the address hit. Instead, the logic should be changed to the following snippet, which only consumes the output signals of the reg_adapter (which might be flopped).

assign racl_error_o = (reg_re | reg_we) &
                        (|addr_hit) & ~(|(racl_addr_hit_read | racl_addr_hit_write));

Note, this also removes the redundant qualification with addr_hit.

in _reg_top, a rejected write will not raise an error if reading from that register is permitted,
and vice-versa; racl_write_addr_hit_read/write are not qualified by the access type.

Agreed. With that it should be simplified to:

assign racl_error_o = (|addr_hit) & ~((reg_re & (|racl_addr_hit_read)) |
                                      (reg_we & (|racl_addr_hit_write)));

This qualifies the racl hit with the corresponding access. As a result, detecting a valid access via reg_re | reg_we can be dropped. I', not sure if |addr_hit can be dropped here as well but I think yes. Only one add_hit is asserted at a time and that is already incorporated in the racl_addr_hit.

assign racl_error_o =  ~((reg_re & (|racl_addr_hit_read)) |
                         (reg_we & (|racl_addr_hit_write)));

• in _sram_racl the signal 'rd_req' is incompletely-qualified; it does not include the appropriate
  a_valid signal, nor a_ready.

I agree. That is missing. That should probably changed to:

assign rd_req = tl_i.a_valid & tl_o.a_ready & (tl_i.a_opcode == tlul_pkg::Get);

Note, the same is true of the adpter_reg_racl.

• in _sram_racl, '~rd_req' is not equivalent to 'wr_req'...there may be no current request, so in
  the generation of racl_error_o it does not suffice simply to invert rd_req.

That's true because of the fac

assign req    = tl_i.a_valid & tl_o.a_ready;
assign rd_req = req & (tl_i.a_opcode == tlul_pkg::Get);
assign wr_req = req & (tl_i.a_opcode == tlul_pkg::PutFullData | tl_i.a_opcode == tlul_pkg::PutPartialData);

Then wr_req is used what previously used to be ~rd_req.

in spi_host_reg_top the tlul_adapter_reg has not been changed to _reg_racl. Is that intentional at this stage?

Yes, that is intentional. Within the reg_top, the RACL checks happen after the register fan-out, on the decoded addresses. The tlul_adapter_reg_racl is used for manual RACL checks on register windows that are manually implemented outside of the reg_top, e.g, for the mailbox RDATA/WDATA window, which uses this adapter. Note, the tlul_adapter_reg_racl performs the RACL checks in front of the register fanout.

• as a precaution it may be preferable to set the default 'RaclErrorRsp' to zero (or to EnableRacl)
  for auto-generated registers (_reg_top) since the racl_error_o signal is always driven.

Could you elaborate on that? I don't understand that.

is the intention to rive racl_error_o and racl_error_log_o.racl_role (in _reg_top) even when EnableRacl is
zero, i.e. no RACL support?

Yes, the intention is to always drive racl_error_o, racl_error_log_o and therefore also racl_error_log_o.racl_role. When RACL is not enabled, these output signals on the corresponding IP top are left unconnected. Although racl_error_log_o.racl_role is set independent of EnableRacl, the assigned racl_role is set to '0 when RACL is disabled - does that make sense?

• a_ready is not timed and qualified appropriately in tlul_request_loopback; this could matter if
  one TL-UL host has an outstanding request and then a request from another host is rejected on
  the grounds of RACL permissions.
  This may give issues with shadow registers too or CDC registers.

Need to look into that in detail.

[alees24]

• in _reg_top the signal 'racl_error_o' is not appropriately qualified with the reg_re and reg_we;
  it will respond to non-register addresses arriving at this IP block; I think you want to respond
  to the control lines 'reg_re' and 'reg_we' and not TL-UL at all.

I'm confused about that. racl_error_o factors in addr_hit, shouldn't that prevent to respond to non-register addresses?

Apologies, I failed to delete that remark from my notes before posting. I had not at that point realized that our address decoding is somewhat redundant. 'addr_hit' does indeed factor in all of the address lines to this IP block as noted below. In some cases it would be necessary, but in others such as USBDEV where half of the address space is registers and the other half is a memory window, it could be simplified slightly.

The logic should presently be functional only because (i) the address decoding is somewhat
redundant; in _reg_top the full IP address is decoded for each register access ('reg_addr == ')
even if the registers occupy only small portion of the IP's address window...

in spi_host_reg_top the tlul_adapter_reg has not been changed to _reg_racl. Is that intentional at this stage?

Yes, that is intentional. Within the reg_top, the RACL checks happen after the register fan-out, on the decoded addresses. The tlul_adapter_reg_racl is used for manual RACL checks on register windows that are manually implemented outside of the reg_top, e.g, for the mailbox RDATA/WDATA window, which uses this adapter. Note, the tlul_adapter_reg_racl performs the RACL checks in front of the register fanout.

Ah, I had misunderstood that. I thought there was an additional layer of protection that applies to the entire IP, separated from the register-level protection.

• as a precaution it may be preferable to set the default 'RaclErrorRsp' to zero (or to EnableRacl)
  for auto-generated registers (_reg_top) since the racl_error_o signal is always driven.

Could you elaborate on that? I don't understand that.

For designs that don't use RACL setting 'RaclErrorRsp' to zero would eliminate the possibility of a spurious 'racl_error_o' assertion becoming a TL-UL error response. Presently, taking 'spi_host' in 'top_earlgrey' as an example, although 'EnableRacl' correctly defaults to zero, the RaclErrorRsp parameter defaults to 1.

Obviously we want the 'racl_error_o' logic to be correct but, as extra protection against faults, and because it makes no sense to return RACL-related TL-UL errors in a non-RACL design, perhaps the IP block should use 'parameter bit RaclErrorRsp = EnableRacl' as its default?

is the intention to rive racl_error_o and racl_error_log_o.racl_role (in _reg_top) even when EnableRacl is
zero, i.e. no RACL support?

Yes, the intention is to always drive racl_error_o, racl_error_log_o and therefore also racl_error_log_o.racl_role. When RACL is not enabled, these output signals on the corresponding IP top are left unconnected. Although racl_error_log_o.racl_role is set independent of EnableRacl, the assigned racl_role is set to '0 when RACL is disabled - does that make sense?

Understood.

• a_ready is not timed and qualified appropriately in tlul_request_loopback; this could matter if
  one TL-UL host has an outstanding request and then a request from another host is rejected on
  the grounds of RACL permissions.
  This may give issues with shadow registers too or CDC registers.

Need to look into that in detail.

Ditto; I don't yet have a clear understanding of how this logic should behave under all circumstances. Intuitively it seems as though 'a_ready' from the TL-UL fabric must be qualified with the squashing decision combinationally to reject the transaction in the current cycle. Also, it must be remembered that the TL-UL protocol permits 'a_valid' to become deasserted without a corresponding assertion of 'a_ready', retracting the desire to perform a transaction.

I shall aim to reason this through and learn more about our TL-UL components; in particular whether out-of-order responses are permitted when multiple sources are involved. This is important on RACL-based systems if there's a tardy response from the IP to an allowed request, and a request from another source is blocked immediately whilst that first request is still in progress.

[Razer6]

For designs that don't use RACL setting 'RaclErrorRsp' to zero would eliminate the possibility of a spurious 'racl_error_o' assertion becoming a TL-UL error response. Presently, taking 'spi_host' in 'top_earlgrey' as an example, although 'EnableRacl' correctly defaults to zero, the RaclErrorRsp parameter defaults to 1.

Obviously we want the 'racl_error_o' logic to be correct but, as extra protection against faults, and because it makes no sense to return RACL-related TL-UL errors in a non-RACL design, perhaps the IP block should use 'parameter bit RaclErrorRsp = EnableRacl' as its default?

Understood and agreed. I changed the default for the top-level IP interfaces and for the adapters themself in the follow up PR.

Apologies, I failed to delete that remark from my notes before posting. I had not at that point realized that our address decoding is somewhat redundant. 'addr_hit' does indeed factor in all of the address lines to this IP block as noted below. In some cases it would be necessary, but in others such as USBDEV where half of the address space is registers and the other half is a memory window, it could be simplified slightly.

add_hit only factors in the addresses that go to the register portion of the reg_top. If there is a window, and a request is routed there, addr_hit is not asserted. For these cases, you would need to use a adapter_reg_racl outside, which is plumbed between the window TLUL request and the actual register implementation.