Re: [Qemu-devel] [Qemu-block] [PATCH 12/16] ahci: ncq migration

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1519 bytes --]

On Fri, Jun 26, 2015 at 12:46:07PM -0400, John Snow wrote:
> On 06/26/2015 11:48 AM, Stefan Hajnoczi wrote:
> > On Mon, Jun 22, 2015 at 08:21:11PM -0400, John Snow wrote:
> >> @@ -1555,6 +1573,35 @@ static int ahci_state_post_load(void
> >> *opaque, int version_id) return -1; }
> >> 
> >> +        for (j = 0; j < AHCI_MAX_CMDS; j++) { +
> >> ncq_tfs = &ad->ncq_tfs[j]; +            ncq_tfs->drive = ad; + +
> >> if (ncq_tfs->used != ncq_tfs->halt) { +                return
> >> -1; +            } +            if (!ncq_tfs->halt) { +
> >> continue; +            } +            if (!is_ncq(ncq_tfs->cmd))
> >> { +                return -1; +            } +            if
> >> (ncq_tfs->slot != ncq_tfs->tag) { +                return -1; +
> >> } +            if (ncq_tfs->slot > AHCI_MAX_CMDS) { +
> >> return -1; +            } +            ncq_tfs->cmdh =
> >> &((AHCICmdHdr *)ad->lst)[ncq_tfs->slot];
> > 
> > Is there a guarantee that ->lst has been mapped?  Maybe pr->cmd & 
> > PORT_CMD_START was 0.
> > 
> > We need to check that the HBA is in a valid state for NCQ
> > processing before attempting this.
> > 
> 
> Slipped my mind, there should be no way to have ncq_tfs->halt set
> under normal circumstances except if the engine is started (and lst is
> mapped.)
> 
> That said, stream corruption is always possible, so I'll add another
> validation check.
> 
> An "is not null" check here should be sufficient, due to the "if not
> halt" continue up above.

Sounds good.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]