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

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

[Alexey Kardashevskiy]

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 <aik@ozlabs.ru>
---

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);
-- 
2.11.0

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

[John Snow]

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 <aik@ozlabs.ru>
> ---
> 
> 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.

--John

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

[Alexey Kardashevskiy]

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 <aik@ozlabs.ru>
>> ---
>>
>> 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

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 <aik@ozlabs.ru>
>>> ---
>>>
>>> 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/qemu-devel@nongnu.org/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/qemu-devel@nongnu.org/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?

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

[Hervé Poussineau]

Le 08/12/2017 à 22:29, John Snow a écrit :
[...]
> 
> 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?
> 

Indeed, I worked on decoupling floppies (and other ISA devices) from i8257, for three reasons:
1) having a working floppy on MIPS Magnum machine. fdc is PC-compatible, but DMA controller is not i8257-compatible.
2) reimplement another ISA bus with a somewhat i8257-compatible DMA
3) for fun, support multiple ISA buses on a same machine but on different PCI bridges.

I failed on point 1), mostly due to lack of documentation.
I succeeded on point 2), by having locally some patches for a ISAPNP bus, where IO/IRQ/DMA addresses of ISA devices can change dynamically, and devices can be discovered by operating system.
About point 3, most of the patches are ready, but some details are still to be fixed.

Regards,

Hervé

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

[Thomas Huth]

On 08.12.2017 22:29, John Snow wrote:
> 
> 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 <aik@ozlabs.ru>
>>>> ---
[...]
>>>> 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/qemu-devel@nongnu.org/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.

The problem still persists ... was there ever a follow-up to this patch
/ discussion?

 Thomas

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

[Philippe Mathieu-Daudé]

On 10/26/2017 03: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".

The same happens with the Alpha machine.

On 12/13/2017 03:19 AM, Hervé Poussineau wrote:
> Le 08/12/2017 à 22:29, John Snow a écrit :
> [...]
>>
>> 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?
>>
> 
> Indeed, I worked on decoupling floppies (and other ISA devices) from
> i8257, for three reasons:
> 1) having a working floppy on MIPS Magnum machine. fdc is PC-compatible,
> but DMA controller is not i8257-compatible.
> 2) reimplement another ISA bus with a somewhat i8257-compatible DMA
> 3) for fun, support multiple ISA buses on a same machine but on
> different PCI bridges.
> 
> I failed on point 1), mostly due to lack of documentation.
> I succeeded on point 2), by having locally some patches for a ISAPNP
> bus, where IO/IRQ/DMA addresses of ISA devices can change dynamically,
> and devices can be discovered by operating system.
> About point 3, most of the patches are ready, but some details are still
> to be fixed.

I'm having hard time refactoring all these PC chipsets, I'll make some
room to send my current work as RFC before soft freeze.

Regards,

Phil.

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

[Philippe Mathieu-Daudé]

On 03/05/2018 07:29 AM, Philippe Mathieu-Daudé wrote:
> On 10/26/2017 03: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".
> 
> The same happens with the Alpha machine.
> 
> On 12/13/2017 03:19 AM, Hervé Poussineau wrote:
>> Le 08/12/2017 à 22:29, John Snow a écrit :
>> [...]
>>>
>>> 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?
>>>
>>
>> Indeed, I worked on decoupling floppies (and other ISA devices) from
>> i8257, for three reasons:
>> 1) having a working floppy on MIPS Magnum machine. fdc is PC-compatible,
>> but DMA controller is not i8257-compatible.
>> 2) reimplement another ISA bus with a somewhat i8257-compatible DMA
>> 3) for fun, support multiple ISA buses on a same machine but on
>> different PCI bridges.
>>
>> I failed on point 1), mostly due to lack of documentation.
>> I succeeded on point 2), by having locally some patches for a ISAPNP
>> bus, where IO/IRQ/DMA addresses of ISA devices can change dynamically,
>> and devices can be discovered by operating system.
>> About point 3, most of the patches are ready, but some details are still
>> to be fixed.
> 
> I'm having hard time refactoring all these PC chipsets, I'll make some
> room to send my current work as RFC before soft freeze.

Sorry I misunderstood the problem; my series only modify the Super I/O
devices, being affected by the same problem Alexey has with the ISA DMA :(

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

[John Snow]

On 03/05/2018 05:15 AM, Thomas Huth wrote:
> On 08.12.2017 22:29, John Snow wrote:
>>
>> 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 <aik@ozlabs.ru>
>>>>> ---
> [...]
>>>>> 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/qemu-devel@nongnu.org/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.
> 
> The problem still persists ... was there ever a follow-up to this patch
> / discussion?
> 
>  Thomas
> 

No, I'll just stab at it during freeze. I can probably at least have it
fail gracefully, though what the "right" thing to do is still not
particularly clear to me as I don't really understand the platforms that
are failing.