Hi Horia, > From: Horia Geanta <[email protected]> > Sent: Wednesday, April 9, 2025 6:47 PM > > On 4/9/2025 9:19 AM, Gaurav Jain wrote: > > Hi Pawel > > > >> From: Paweł Kochanowski <[email protected]> > >> > >> Hi Gaurav, > >> > >> What we see is that the jr_enqueue() called by run_descriptor_jr() > >> swaps the endianness of the descriptor in place (by modifying the > >> data pointed by > >> desc_add): > >> > >> /* The descriptor must be submitted to SEC block as per endianness > >> * of the SEC Block. > >> * So, if the endianness of Core and SEC block is different, each > >> word > >> * of the descriptor will be byte-swapped. > >> */ > >> for (i = 0; i < length; i++) { > >> desc_word = desc_addr[i]; > >> sec_out32((uint32_t *)&desc_addr[i], desc_word); > >> } > >> > >> So the sequence looks like this: > >> 1. caam_rng_probe sets correct descriptor in ` caam_rng_priv *priv` 2. > >> caam_rng_read is called with 32B 3. > >> caam_rng_read_one->run_descriptor_jr() is called to generate 16B with > >> `priv->desc` containing valid descriptor 4. The descriptor is swapped > >> in jr_enqueue() before passing it to job ring > > > > I see you are right. In Linux, caam endianness is handled at the time of > creating the descriptor. > > But In Uboot, caam endianness is not handled at the time of descriptor > creation. > > > IMO the U-Boot approach is broken. > The handling of endianness must be done at creation time, only then we know > how to interpret the data and perform adequate byteswaps. > U-Boot makes the assumption that everything should be 4-byte swapped, but > there are cases when the swap has to be done 8-byte wide (for example > address pointers or 8-byte immediate data). >
I think you are right, although it might not be strictly necessary in U-boot as there seems to be no case where the 8-byte long data are used. As I see the append_ptr() is already prepared for that as the `struct ptr_addr_t` definition is different based on CAAM endianness (so words are swapped). The other place that could potentially cause a problem is append_u64() but I can not find anyplace that it is used in and I would propose to remove it. Do you see other places that I missed? I also crosschecked with TF-A repository and it is done in very similar way there (per word swap on usage and not on creation). BR, Pawel. > > 5. The loop in > >> caam_rng_read is called second time, this time the `priv->desc` > >> contain swapped data. > >> > >> Interesting thing is that the job still succeeds and that some data > >> are present in the buffers, but maybe swapped descriptor can also be a > valid one? > > I agree that the descriptor should be reinitialized for each RNG job. > I would advise against this, considering what I said above. > Handling endianness should be done only once. > > Regards, > Horia
