Re: [PATCH 2/3] aspeed: Add Scater-Gather support for HACE Hash

[Klaus Heinrich Kiwi]

Hi Joel,

On 3/25/2021 12:40 AM, Joel Stanley wrote:
> On Wed, 24 Mar 2021 at 22:39, Klaus Heinrich Kiwi
> <klaus@linux.vnet.ibm.com> wrote:
> > Complement the Aspeed HACE support with Scatter-Gather hash support for
> > sha256 and sha512. Scatter-Gather is only supported on AST2600-series.
>
> Please update the documentation at docs/system/arm/aspeed.rst too.
I've removed the 'no scatter-gather' from the doc.

> > +                                                                   false,
> > +                                                 MEMTXATTRS_UNSPECIFIED);
>
> In the direct access code, we use address_space_map to save copying
> the memory contents that is to be hashed. That's not the case for the
> scatter gather list.
>
> Instead of creating mappings to read the sg list, you could load the
> addr, len pairs using address_space_ldl_le. This would give you the
> pointer to create mappings for.

I've reworked the code to use address_space_ldl_le, also removed the redundant
isLast variable.

 
> > +    /*
> > +     * Set status bits to indicate completion. Testing shows hardware sets
> > +     * these irrespective of HASH_IRQ_EN.
>
> This is the same comment from the direct method. Have you confirmed
> this is true on hardware?
Yes, I was able to confirm that on real hardware (AST2600-A1)

> > -        do_hash_operation(s, algo);
> > +        if (data & HASH_SG_EN) {
> > +            s->regs[(R_HASH_SRC >> 2)] &= 0x7FFFFFF8;
>
> This is setting (0x20 / 4) >> 2 == 2, which is Crypto Data
> Destination. I suspect you wanted R_HASH_SRC, so you can omit the
> right shift.
>
> However I suggest you check that hardware masks the register when the
> write occurs, and if it does, implement that in the write callback for
> R_HASH_SRC. That way a guest code doing a read-write will see the
> correct thing.

I was able to check on real hardware that, even when requesting a
HASH_SG_EN operation, the masking on the src address register is only
0x7fffffff, so I removed the masking specific to HASH_SG_EN.


> >   };
> >
> > +#define ASPEED_HACE_MAX_SG      256
> > +struct aspeed_sg_list {
> > +        uint32_t len;
> > +        uint32_t phy_addr;
>
> Does "phy" mean physical? If so, we could call it phys_addr to avoid
> confusion with the addresses of PHYs.
>
> Alternatively, call it 'addr'.

Since I'm not using address_map_ldl_le to access these, I decided to
do so based on #defines offsets, so I no longer need a Struct here.


Will send a V2 soon.

Thanks,

 -Klaus
--
Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com>