Re: [Qemu-devel] [PATCH qemu] fdc: Exit if ISA controller does not support DMA

[John Snow]

On 11/21/2017 09:48 PM, Alexey Kardashevskiy wrote:
> 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? :)
> 
> 

*cough* It turns out that understanding the intricacies of FDC and ISA
is nobody's favorite thing to do.

OK, so ehabkost pointed me to this:

https://www.mail-archive.com/address@hidden/msg496460.html

Where we declare that DMA devices generally can't be created by the user
for the inverse of the reason we're seeing here: these devices need to
be created precisely once: not zero times, not twice, exactly once.

So we made the ISA DMA devices themselves not user-creatable, so you are
indeed correct here that the absence of fdctrl->dma does more or less
mean that the current configuration "doesn't support DMA." ... but maybe
this won't always be true, and maybe some devices (TYPE_I82374?) are
user creatable, so let's make a "softer" error message:

"No ISA DMA device present, can't create ISA FDC device."

Then, on the other end, we need to unwind realize() gracefully, maybe we
can just shuffle up isa_get_dma() earlier so we don't have to unwind
anything if it comes back empty.

Then I'll take the patch, because fixing this more properly I think will
take more time or effort than I have to spend on the FDC device.

Thanks to Eduardo for looking this over with me.

--js

As a post-script, ehabkost dug this up too:
https://www.mail-archive.com/address@hidden/msg348764.html

It looks like Herve was working on decoupling floppies from i8257, but
perhaps didn't get all the way through -- I'm not actually clear on what
work remains to be done here, maybe he can chime in if he's still
interested in the project?