[Qemu-devel] Re: [PATCH 03/13] ide: Split out BMDMA code from ATA core

[Stefan Hajnoczi]

On Wed, Dec 8, 2010 at 2:35 PM, Kevin Wolf <address@hidden> wrote:
> Am 08.12.2010 15:26, schrieb Stefan Hajnoczi:
>> On Wed, Dec 8, 2010 at 12:13 PM, Alexander Graf <address@hidden> wrote:
>>> @@ -486,8 +440,8 @@ void ide_dma_error(IDEState *s)
>>>     ide_transfer_stop(s);
>>>     s->error = ABRT_ERR;
>>>     s->status = READY_STAT | ERR_STAT;
>>> -    ide_dma_set_inactive(s->bus->bmdma);
>>> -    s->bus->bmdma->status |= BM_STATUS_INT;
>>> +    ide_set_inactive(s);
>>> +    s->bus->dma.ops->set_status(s->bus->dma.opaque, BM_STATUS_INT);
>>
>> Is BM_STATUS_INT constant naming appropriate for a general DMA
>> abstraction?  Perhaps DMA_STATUS_INT.
>
> BM_STATUS_INT is a bit in the status register of busmaster IDE. So in
> theory it shouldn't appear in generic ATA code, but I'm not sure how
> much of this we can fix at this point.
>
>> Instead of fleshing out these functions, how about initializing
>> dma.ops to NULL?  The program crashes should anyone try to do DMA
>> before setting a real IDEDMAOps pointer.  That's not as robust as
>> limping along with non-working IDE, but should be straightforward to
>> debug if it ever happens.  It also requires less code.
>
> Allowing the guest to crash qemu is not an option. We'd have to check
> for NULL in all commands that initiate a DMA transfer.

You're right, I wasn't aware that the ops gets a chance to execute
before we initialize them to BMDMA.

Stefan