Re: [Qemu-devel] [PATCH v2 00/14] sdcard: Proper implementation of CRC7

[Peter Maydell]

On 9 May 2018 at 04:46, Philippe Mathieu-Daudé <address@hidden> wrote:
> Hi,
>
> This series emerged after last Coverity scan and Peter suggestion in:
> http://lists.nongnu.org/archive/html/qemu-devel/2018-04/msg05046.html
>
>     (3) "proper" implementation of CRC, so that an sd controller
>     can either (a) mark the SDRequest as "no CRC" and have
>     sd_req_crc_validate() always pass, or (b) mark the SDRequest
>     as having a CRC and have sd_req_crc_validate() actually
>     do the check which it currently stubs out with "return 0"
>
> - Coverity issues fixed
> - new functions documented
> - qtests added
>
> This series would be much smaller without qtests (less refactor and code
> movement), but I feel more confident having them passing :)

This series isn't going in the direction I expected.

We have two cases:
 (1) we model an SD controller which in hardware calculates its
     own checksums. Most of our SD controllers are like that.
     Since in QEMU there is no chance of data corruption between
     the controller and the card, there's no point in calculating
     a checksum by calling a function, and then checking that we
     get the same result a few function calls later when we call
     the exact same function in the SD card model. So we should
     just unconditionally say "no checksum provided" in the
     SDRequest struct the controller fills in, and then skip
     the check in the card model.
 (2) we model an SD controller which leaves the checksum calculation
     to guest software. The only one of these we have that I know
     of is milkymist-memcard. In this case, the checksum is calculated
     by guest software, and so we do want to check it in the SD
     card model. So the controller should fill in the CRC field
     in SDRequest, and set the flag in SDRequest to say "checksum
     provided".

We don't need to provide a property on the device or the
card objects to control this behaviour, for either case.

I'm not clear why this patchset has removed the SDRequest struct
in favour of a raw buffer, either: that makes it harder to just
say "this request doesn't have a checksum you need to check".

thanks
-- PMM