Re: [POC 2/2] add test exposing AHCI reset issue

[Thomas Huth]

On 24/08/2023 15.38, Fiona Ebner wrote:
> Fails without the previous commit "hw/ide: reset: cancel async DMA
> operation before reseting state".
>
> I haven't ever written such a test before, but I wanted something to
> expose the problem more easily. It hardcodes the behavior that the
> pending write actually is done during reset, which might not be ideal.
> It could just check that the first sector is still intact instead.
>
> If I should make this a proper test, I'd be happy about some guidance,
> but not sure if required for such a specific one-off issue. After all,
> a different variation of the bug might have written to some other
> sector not covered by this test.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>   tests/qtest/ahci-test.c | 81 +++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 81 insertions(+)
>
> diff --git a/tests/qtest/ahci-test.c b/tests/qtest/ahci-test.c
> index abab761c26..3ebeb4e255 100644
> --- a/tests/qtest/ahci-test.c
> +++ b/tests/qtest/ahci-test.c
> @@ -1401,6 +1401,84 @@ static void test_max(void)
>       ahci_shutdown(ahci);
>   }
>   
> +static void test_reset_with_pending_callback(void)
> +{
> +    AHCIQState *ahci;
> +
> +    ahci = ahci_boot(NULL);
> +    ahci_test_pci_spec(ahci);
> +    ahci_pci_enable(ahci);
> +
> +    int bufsize = 512 * 1024;
> +    int offset1 = 0;
> +    int offset2 = bufsize / AHCI_SECTOR_SIZE;
> +
> +    ahci_test_hba_spec(ahci);
> +    ahci_hba_enable(ahci);
> +    ahci_test_identify(ahci);
> +
> +    uint8_t port = ahci_port_select(ahci);
> +    ahci_port_clear(ahci, port);
> +
> +    unsigned char *tx1 = g_malloc(bufsize);
> +    unsigned char *tx2 = g_malloc(bufsize);
> +    unsigned char *rx1 = g_malloc0(bufsize);
> +    unsigned char *rx2 = g_malloc0(bufsize);
> +    uint64_t ptr1 = ahci_alloc(ahci, bufsize);
> +    uint64_t ptr2 = ahci_alloc(ahci, bufsize);

As Philippe already mentioned, please declare variables at the beginning of 
the functions to match our coding style.

I'd also oike to suggest to use g_autofree for the buffers that you 
malloced, so you can drop the g_frees at the end of the function.

> +    g_assert(ptr1 && ptr2);
> +
> +    /* Need two different patterns. */
> +    do {
> +        generate_pattern(tx1, bufsize, AHCI_SECTOR_SIZE);
> +        generate_pattern(tx2, bufsize, AHCI_SECTOR_SIZE);
> +    } while (memcmp(tx1, tx2, bufsize) == 0);
> +
> +    qtest_bufwrite(ahci->parent->qts, ptr1, tx1, bufsize);
> +    qtest_bufwrite(ahci->parent->qts, ptr2, tx2, bufsize);
> +
> +    /* Write to beginning of disk to check it wasn't overwritten later. */
> +    ahci_guest_io(ahci, port, CMD_WRITE_DMA_EXT, ptr1, bufsize, offset1);
> +
> +    /* Issue asynchronously to get a pending callback during reset. */
> +    AHCICommand *cmd = ahci_command_create(CMD_WRITE_DMA_EXT);
> +    ahci_command_adjust(cmd, offset2, ptr2, bufsize, 0);
> +    ahci_command_commit(ahci, cmd, port);
> +    ahci_command_issue_async(ahci, cmd);
> +
> +    ahci_set(ahci, AHCI_GHC, AHCI_GHC_HR);
> +
> +    ahci_command_free(cmd);
> +
> +    /* Start again. */
> +    ahci_clean_mem(ahci);
> +    ahci_pci_enable(ahci);
> +    ahci_hba_enable(ahci);
> +    port = ahci_port_select(ahci);
> +    ahci_port_clear(ahci, port);
> +
> +    /* Read and verify. */
> +    ahci_guest_io(ahci, port, CMD_READ_DMA_EXT, ptr1, bufsize, offset1);
> +    qtest_bufread(ahci->parent->qts, ptr1, rx1, bufsize);
> +    g_assert_cmphex(memcmp(tx1, rx1, bufsize), ==, 0);
> +
> +    ahci_guest_io(ahci, port, CMD_READ_DMA_EXT, ptr2, bufsize, offset2);
> +    qtest_bufread(ahci->parent->qts, ptr2, rx2, bufsize);
> +    g_assert_cmphex(memcmp(tx2, rx2, bufsize), ==, 0);
> +
> +    ahci_free(ahci, ptr1);
> +    ahci_free(ahci, ptr2);
> +    g_free(tx1);
> +    g_free(tx2);
> +    g_free(rx1);
> +    g_free(rx2);
> +
> +    ahci_clean_mem(ahci);
> +
> +    ahci_shutdown(ahci);
> +}
> +
>   static void test_reset(void)
>   {
>       AHCIQState *ahci;
> @@ -1915,6 +1993,9 @@ int main(int argc, char **argv)
>       g_assert(fd >= 0);
>       close(fd);
>   
> +    qtest_add_func("/ahci/reset_with_pending_callback",
> +                   test_reset_with_pending_callback);

It would make more sense to put this below the "Run the tests" comment below.

>       /* Run the tests */
>       qtest_add_func("/ahci/sanity",     test_sanity);
>       qtest_add_func("/ahci/pci_spec",   test_pci_spec);

 Thomas