[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH qemu] fdc: Exit if ISA controller does not suppo
From: |
Alexey Kardashevskiy |
Subject: |
Re: [Qemu-devel] [PATCH qemu] fdc: Exit if ISA controller does not support DMA |
Date: |
Wed, 22 Nov 2017 13:48:23 +1100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 |
On 07/11/17 11:58, John Snow wrote:
>
>
> On 10/26/2017 02:46 AM, Alexey Kardashevskiy wrote:
>> A "powernv" machine type defines an ISA bus but it does not add any DMA
>> controller to it so it is possible to hit assert(fdctrl->dma) by
>> adding "-machine powernv -device isa-fdc".
>>
>> This replaces assert() with an error message.
>>
>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>> ---
>>
>> Is it a must for ISA to have DMA controllers?
>>
>>
>> ---
>> hw/block/fdc.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index 67f78ac702..ed8b367572 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -2700,7 +2700,10 @@ static void isabus_fdc_realize(DeviceState *dev,
>> Error **errp)
>> fdctrl->dma_chann = isa->dma;
>> if (fdctrl->dma_chann != -1) {
>> fdctrl->dma = isa_get_dma(isa_bus_from_device(isadev), isa->dma);
>> - assert(fdctrl->dma);
>> + if (!fdctrl->dma) {
>> + error_setg(errp, "ISA controller does not support DMA,
>> exiting");
>> + return;
>> + }
>> }
>>
>> qdev_set_legacy_instance_id(dev, isa->iobase, 2);
>>
>
> I've been MIA for a little while, so I'm out of the loop -- but I am not
> sure this is entirely the right way to fix this problem. I think it is
> more the case that certain boards should not be able to ask for certain
> types of devices, and we should prohibit e.g. powernv from being able to
> ask for an ISA floppy disk controller.
>
> (It doesn't seem to have an ISA DMA controller by default, but I have no
> idea if that means it can't EVER have one...)
>
> Papering over this by making it a soft error when we fail to execute
> isa_get_dma and then assuming in retrospect it's because the machine
> type we're on cannot have an ISA DMA controller seems a little
> wrong-headed. It also leaves side-effects from isa_register_portio_list
> and isa_init_irq, so we can't just bail here -- it's only marginally
> better than the assert() it's doing.
>
> That said, I am not really sure what the right thing to do is ... I
> suspect the "right thing" is to express the dependency that isa-fdc
> requires an ISA DMA controller -- and maybe that check happens here when
> isa_get_dma fails and we have to unwind the realize function, but we
> need to do it gracefully.
>
> Give me a day to think about it, but I do want to make sure this is in
> the next release.
The day has passed, any news? :)
--
Alexey