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

[Kevin Wolf]

Am 08.12.2010 15:40, schrieb 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.

For example with ISA we never intialize it at all.

Kevin