[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 14/16] ahci: Do not map cmd_fis to generate resp
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH 14/16] ahci: Do not map cmd_fis to generate response |
Date: |
Tue, 30 Jun 2015 15:50:45 +0100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Mon, Jun 29, 2015 at 11:07:18AM -0400, John Snow wrote:
> On 06/29/2015 10:51 AM, Stefan Hajnoczi wrote:
> > On Fri, Jun 26, 2015 at 01:31:12PM -0400, John Snow wrote:
> >> On 06/26/2015 11:59 AM, Stefan Hajnoczi wrote:
> >>> On Mon, Jun 22, 2015 at 08:21:13PM -0400, John Snow wrote:
> >>>> @@ -744,8 +722,8 @@ static void ahci_write_fis_pio(AHCIDevice
> >>>> *ad, uint16_t len) pio_fis[9] = s->hob_lcyl; pio_fis[10] =
> >>>> s->hob_hcyl; pio_fis[11] = 0; - pio_fis[12] = cmd_fis[12]; -
> >>>> pio_fis[13] = cmd_fis[13]; + pio_fis[12] = s->nsector & 0xFF;
> >>>> + pio_fis[13] = (s->nsector >> 8) & 0xFF;
> >>>
> >>> hw/ide/core.c decreases s->nsector until it reaches 0 and the
> >>> request ends.
> >>>
> >>> Will the values reported back to the guest be correct if we use
> >>> s->nsector?
> >>>
> >>
> >> See the commit message for justification of this one. Ultimately, it
> >> doesn't matter what gets put in here (for data transfer commands) --
> >> but getting RID of the cmd_fis mapping is a strong positive.
> >
> > Getting rid of cmd_fis mapping is good.
> >
> > Putting s->nsector into the undefined fields makes the code confusing.
> >
> > It is clearer to zero the bytes with a comment saying the value does not
> > matter according to the spec.
> >
>
> Well, it's not that it doesn't matter /ever/, it's more that for
> standard IO routines it doesn't matter. (See the normative output spec
> in ATA8-AC3 -- for most cases it's N/A, but for a handful of cases it
> carries a diagnostic signature.)
>
> What's really the case is that the FIS always dutifully copies out what
> the SATA registers are (or should be.)
>
> There are still a handful of commands that, if we choose to support
> them, copying the nsector register would be the "correct thing" to do,
> so I decided to copy that field here to serve as documentation and
> support future command additions.
>
> I would argue that if this field ever does the /wrong thing/, it would
> be a fix in the S/ATA layer, and not a change to the FIS generator here.
>
> I am inclined to leave it as-is, since for the current cases, nsector is
> going to empty to zero anyway. I believe the behavior presented here is
> correct.
I'm trying to understand the guest-visible change in behavior. Guests
might take different code paths from before.
For example, I think that after this patch the CHECK POWER MODE command
works correctly for the first time with AHCI. It sets ->nsector to
0xff.
Anyway, I'm happy with assigning s->nsector.
pgpZeOvGtnlph.pgp
Description: PGP signature
[Qemu-devel] [PATCH 16/16] ahci: fix sdb fis semantics, John Snow, 2015/06/22
Re: [Qemu-devel] [PATCH 00/16] ahci: ncq cleanup, part 2, Stefan Hajnoczi, 2015/06/26