P J P <ppan...@redhat.com> 于2020年9月29日周二 下午2:22写道: > > Hello Li, > > +-- On Fri, 18 Sep 2020, Li Qiang wrote --+ > | P J P <ppan...@redhat.com> 于2020年9月18日周五 下午6:26写道: > | > +-- On Fri, 18 Sep 2020, Li Qiang wrote --+ > | > | Update v2: use an assert() call > | > | > ->https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg08336.html > | > | In 'ide_ioport_write' the guest can set 'bus->unit' to 0 or 1 by issue > | 'ATA_IOPORT_WR_DEVICE_HEAD'. So this case the guest can set the active ifs. > | If the guest set this to 1. > | > | Then in 'idebus_active_if' will return 'IDEBus.ifs[1]' and thus the 's->blk' > | will be NULL. > > Right, guest does select the drive via > > portio_write > ->ide_ioport_write > case ATA_IOPORT_WR_DEVICE_HEAD: > /* FIXME: HOB readback uses bit 7 */ > bus->ifs[0].select = (val & ~0x10) | 0xa0; > bus->ifs[1].select = (val | 0x10) | 0xa0; > /* select drive */ > bus->unit = (val >> 4) & 1; <== set bus->unit=0x1 > break; > > > | So from your (Peter's) saying, we need to check the value in > | 'ATA_IOPORT_WR_DEVICE_HEAD' handler. To say if the guest > | set a valid 'bus->unit'. This can also work I think. > > Yes, with the following fix, an assert(3) in ide_cancel_dma_sync fails. > > === > diff --git a/hw/ide/core.c b/hw/ide/core.c > index f76f7e5234..cb55cc8b0f 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -1300,7 +1300,11 @@ void ide_ioport_write(void *opaque, uint32_t addr, > uint_) > bus->ifs[0].select = (val & ~0x10) | 0xa0; > bus->ifs[1].select = (val | 0x10) | 0xa0; > /* select drive */ > + uint8_t bu = bus->unit; > bus->unit = (val >> 4) & 1; > + if (!bus->ifs[bus->unit].blk) { > + bus->unit = bu; > + } > break; > default: > > qemu-system-x86_64: ../hw/ide/core.c:724: ide_cancel_dma_sync: Assertion > `s->bus->dma->aiocb == NULL' failed. > Aborted (core dumped)
This is what I am worried, in the 'ide_ioport_write' set the 'bus->unit'. It also change the 'buf->ifs[0].select'. Also there maybe some other corner case that causes some inconsistent. And if we choice this method we need to deep into the more ahci-spec to know how things really going. > === > > | As we the 'ide_exec_cmd' and other functions in 'hw/ide/core.c' check the > | 's->blk' directly. I think we just check it in 'ide_cancel_dma_sync' is > | enough and also this is more consistent with the other functions. > | 'ide_cancel_dma_sync' is also called by 'cmd_device_reset' which is one of > | the 'ide_cmd_table' handler. > > Yes, I'm okay with either approach. Earlier patch v1 checks 's->blk' in > ide_cancel_dma_sync(). I prefer the 'check the s->blk in the beginning of ide_cancel_dma_sync' method. Some little different with your earlier patch. Anyway, let the maintainer do the choices. Thanks, Li Qiang > > | BTW, where is the Peter's email saying this, just want to learn something, > | :). > > -> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg05820.html > > Thank you. > -- > Prasad J Pandit / Red Hat Product Security Team > 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D