Re: [PATCH] hw/sd/sdhci: Report error when guest access protected registers

[John Snow <jsnow@redhat.com>]

On 2/18/21 12:56 PM, Philippe Mathieu-Daudé wrote:
> The SDHC_SYSAD and SDHC_BLKSIZE can not be accessed while a
> transaction is in progress, see 'SD Host Controller Simplified
> Specification'
> 
>    1.5) SD Command Generation
> 
>    The Host Driver should not read the SDMA System Address, Block
>    Size and Block Count registers during a data transaction unless
>    the transfer is stopped because the value is changing and not
>    stable.
> 

Naive question: Is this an RFC2119 "SHOULD NOT"? (i.e., does it have a 
defined behavior that you are simply encouraged to avoid?)

Is it really an error?

> Report guest intents as errors.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Based-on: <1613447214-81951-1-git-send-email-bmeng.cn@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/sd/sdhci.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 5b8678110b0..98928c18542 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1136,6 +1136,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>                       sdhci_sdma_transfer_single_block(s);
>                   }
>               }
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: Transfer already in progress,"
> +                          " can not update SYSAD", __func__);
>           }
>           break;
>       case SDHC_BLKSIZE:
> @@ -1163,8 +1167,11 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>               if (blksize != s->blksize) {
>                   s->data_count = 0;
>               }
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: Transfer already in progress,"
> +                          " can not update BLKSIZE", __func__);
>           }
> -
>           break;
>       case SDHC_ARGUMENT:
>           MASKED_WRITE(s->argument, mask, value);
>