Re: [PATCH 3/9] hw/ide/ahci: write D2H FIS on when processing NCQ command

From: John Snow <jsnow@redhat.com>
To: Niklas Cassel <nks@flawful.org>
Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org,
	 Damien Le Moal <dlemoal@kernel.org>,
	Niklas Cassel <niklas.cassel@wdc.com>
Subject: Re: [PATCH 3/9] hw/ide/ahci: write D2H FIS on when processing NCQ command
Date: Wed, 17 May 2023 17:18:54 -0400	[thread overview]
Message-ID: <CAFn=p-b4wm2ZaFXnXkm22+tSExjQFWYMTrSy8zG4M-MzuLWtzQ@mail.gmail.com> (raw)
In-Reply-To: <20230428132124.670840-4-nks@flawful.org>

On Fri, Apr 28, 2023 at 9:22 AM Niklas Cassel <nks@flawful.org> wrote:
>
> From: Niklas Cassel <niklas.cassel@wdc.com>
>
> The way that BUSY + PxCI is cleared for NCQ (FPDMA QUEUED) commands is
> described in SATA 3.5a Gold:
>
> 11.15 FPDMA QUEUED command protocol
> DFPDMAQ2: ClearInterfaceBsy
> "Transmit Register Device to Host FIS with the BSY bit cleared to zero
> and the DRQ bit cleared to zero and Interrupt bit cleared to zero to
> mark interface ready for the next command."
>
> PxCI is currently cleared by handle_cmd(), but we don't write the D2H
> FIS to the FIS Receive Area that actually caused PxCI to be cleared.
>
> Similar to how ahci_pio_transfer() calls ahci_write_fis_pio() with an
> additional parameter to write a PIO Setup FIS without raising an IRQ,
> add a parameter to ahci_write_fis_d2h() so that ahci_write_fis_d2h()
> also can write the FIS to the FIS Receive Area without raising an IRQ.
>
> Change process_ncq_command() to call ahci_write_fis_d2h() without
> raising an IRQ (similar to ahci_pio_transfer()), such that the FIS
> Receive Area is in sync with the PxTFD shadow register.
>
> E.g. Linux reads status and error fields from the FIS Receive Area
> directly, so it is wise to keep the FIS Receive Area and the PxTFD
> shadow register in sync.

I think for some time I wondered if this mattered, because I wasn't
sure when the guest CPU would actually regain control to check an
intermediate state in the memory area before we wrote the next FIS.
But, trusting your quoted blurb, I think this is more obviously
correct.

ACK

(Although, there seems to be a conflict on latest origin/master - can
you rebase, please?)

>
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>  hw/ide/ahci.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index a36e3fb77c..62aebc8de7 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -43,7 +43,7 @@
>  static void check_cmd(AHCIState *s, int port);
>  static int handle_cmd(AHCIState *s, int port, uint8_t slot);
>  static void ahci_reset_port(AHCIState *s, int port);
> -static bool ahci_write_fis_d2h(AHCIDevice *ad);
> +static bool ahci_write_fis_d2h(AHCIDevice *ad, bool d2h_fis_i);
>  static void ahci_init_d2h(AHCIDevice *ad);
>  static int ahci_dma_prepare_buf(const IDEDMA *dma, int32_t limit);
>  static bool ahci_map_clb_address(AHCIDevice *ad);
> @@ -618,7 +618,7 @@ static void ahci_init_d2h(AHCIDevice *ad)
>          return;
>      }
>
> -    if (ahci_write_fis_d2h(ad)) {
> +    if (ahci_write_fis_d2h(ad, true)) {
>          ad->init_d2h_sent = true;
>          /* We're emulating receiving the first Reg H2D Fis from the device;
>           * Update the SIG register, but otherwise proceed as normal. */
> @@ -850,7 +850,7 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len, bool pio_fis_i)
>      }
>  }
>
> -static bool ahci_write_fis_d2h(AHCIDevice *ad)
> +static bool ahci_write_fis_d2h(AHCIDevice *ad, bool d2h_fis_i)
>  {
>      AHCIPortRegs *pr = &ad->port_regs;
>      uint8_t *d2h_fis;
> @@ -864,7 +864,7 @@ static bool ahci_write_fis_d2h(AHCIDevice *ad)
>      d2h_fis = &ad->res_fis[RES_FIS_RFIS];
>
>      d2h_fis[0] = SATA_FIS_TYPE_REGISTER_D2H;
> -    d2h_fis[1] = (1 << 6); /* interrupt bit */
> +    d2h_fis[1] = d2h_fis_i ? (1 << 6) : 0; /* interrupt bit */
>      d2h_fis[2] = s->status;
>      d2h_fis[3] = s->error;
>
> @@ -890,7 +890,10 @@ static bool ahci_write_fis_d2h(AHCIDevice *ad)
>          ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_TFES);
>      }
>
> -    ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_DHRS);
> +    if (d2h_fis_i) {
> +        ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_DHRS);
> +    }
> +
>      return true;
>  }
>
> @@ -1120,6 +1123,8 @@ static void process_ncq_command(AHCIState *s, int port, const uint8_t *cmd_fis,
>          return;
>      }
>
> +    ahci_write_fis_d2h(ad, false);
> +
>      ncq_tfs->used = 1;
>      ncq_tfs->drive = ad;
>      ncq_tfs->slot = slot;
> @@ -1506,7 +1511,7 @@ static void ahci_cmd_done(const IDEDMA *dma)
>      }
>
>      /* update d2h status */
> -    ahci_write_fis_d2h(ad);
> +    ahci_write_fis_d2h(ad, true);
>
>      if (ad->port_regs.cmd_issue && !ad->check_bh) {
>          ad->check_bh = qemu_bh_new(ahci_check_cmd_bh, ad);
> --
> 2.40.0
>