[RFC,8/9] isa: Clean up use of cannot_instantiate_with_device_add_yet

From: Markus Armbruster <armbru@redhat.com>

Drop it when there's no obvious reason why device_add could not work.
Else keep and document why.

* isa-fdc, port92, i8042, m48t59_isa, mc146818rtc, isa-pit, kvm-pit:
  drop (the last two by dropping it from their abstract base
  pit-common)

* pcspk: keep because of pointer property pit, and because realize
  sets global pcspk_state

* vmmouse: keep because of pointer property ps2_mouse

* vmport: keep because realize sets global port_state

* pic-common, isa-i8259, kvm-i8259: move from abstract base pic-common
  to its subtypes, keep in isa-i8259 because init sets global isa_pic
  and slave_pic, keep in kvm-i8259 with /* FIXME explain why */

Perhaps I should split this patch.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/audio/pcspk.c        | 3 ++-
 hw/block/fdc.c          | 1 -
 hw/i386/kvm/i8259.c     | 1 +
 hw/i386/pc.c            | 1 -
 hw/input/pckbd.c        | 1 -
 hw/input/vmmouse.c      | 3 ++-
 hw/intc/i8259.c         | 2 ++
 hw/intc/i8259_common.c  | 1 -
 hw/misc/vmport.c        | 3 ++-
 hw/timer/i8254_common.c | 1 -
 hw/timer/m48t59.c       | 1 -
 hw/timer/mc146818rtc.c  | 1 -
 12 files changed, 9 insertions(+), 10 deletions(-)

Paolo, or maybe Andreas,

To go beyond RFC with this series, I need to explain why isa-i8259 and
kvm-i8259 cannot_instantiate_with_device_add_yet, or drop that.  I'd
appreciate your help.

Both are derived from TYPE_PIC_COMMON, which is derived from
TYPE_ISA_DEVICE.

I figure isa-i8259 cannot_instantiate_with_device_add_yet, because it
sets global isa_pic and slave_pic.  slave_pic appears to be a lame way
to wire the slave PIC to the master PIC behind QOM's back.  isa_pic
appears to be a lame way to wire the master PIC to whatever it needs to
be wired to.  Is that a fair description?

If yes, is it sufficient reason for
cannot_instantiate_with_device_add_yet?

kvm-i8259 is the same device implemented with kernel support.  Does it
have its own reason for cannot_instantiate_with_device_add_yet?

If not, should it keep cannot_instantiate_with_device_add_yet for
symmetry with isa-i8259?

Il 15/10/2013 14:43, Markus Armbruster ha scritto:
> Paolo, or maybe Andreas,
> 
> To go beyond RFC with this series, I need to explain why isa-i8259 and
> kvm-i8259 cannot_instantiate_with_device_add_yet, or drop that.  I'd
> appreciate your help.
> 
> Both are derived from TYPE_PIC_COMMON, which is derived from
> TYPE_ISA_DEVICE.
> 
> I figure isa-i8259 cannot_instantiate_with_device_add_yet, because it
> sets global isa_pic and slave_pic.  slave_pic appears to be a lame way
> to wire the slave PIC to the master PIC behind QOM's back.  isa_pic
> appears to be a lame way to wire the master PIC to whatever it needs to
> be wired to.  Is that a fair description?

Yes.

> If yes, is it sufficient reason for
> cannot_instantiate_with_device_add_yet?
> 
> kvm-i8259 is the same device implemented with kernel support.  Does it
> have its own reason for cannot_instantiate_with_device_add_yet?
> 
> If not, should it keep cannot_instantiate_with_device_add_yet for
> symmetry with isa-i8259?

Both i8259 implementations have to be matched with an appropriate array
of qemu_irqs, such as the one returned by kvm_i8259_init.  I think this
is the reason why kvm-i8259 cannot be instantiated from the command-line.

Paolo

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 15/10/2013 14:43, Markus Armbruster ha scritto:
>> Paolo, or maybe Andreas,
>> 
>> To go beyond RFC with this series, I need to explain why isa-i8259 and
>> kvm-i8259 cannot_instantiate_with_device_add_yet, or drop that.  I'd
>> appreciate your help.
>> 
>> Both are derived from TYPE_PIC_COMMON, which is derived from
>> TYPE_ISA_DEVICE.
>> 
>> I figure isa-i8259 cannot_instantiate_with_device_add_yet, because it
>> sets global isa_pic and slave_pic.  slave_pic appears to be a lame way
>> to wire the slave PIC to the master PIC behind QOM's back.  isa_pic
>> appears to be a lame way to wire the master PIC to whatever it needs to
>> be wired to.  Is that a fair description?
>
> Yes.
>
>> If yes, is it sufficient reason for
>> cannot_instantiate_with_device_add_yet?
>> 
>> kvm-i8259 is the same device implemented with kernel support.  Does it
>> have its own reason for cannot_instantiate_with_device_add_yet?
>> 
>> If not, should it keep cannot_instantiate_with_device_add_yet for
>> symmetry with isa-i8259?
>
> Both i8259 implementations have to be matched with an appropriate array
> of qemu_irqs, such as the one returned by kvm_i8259_init.  I think this
> is the reason why kvm-i8259 cannot be instantiated from the command-line.

Let me try to elaborate, to make sure I understand.

Unlike ordinary ISA devices, the i8259 devices need additional wiring,
done by code.

For instance, board code like pc_q35_init(), pc_piix.c's pc_init1(),
mips_malta_init(), ... wire up their IRQ input lines.  The slave's IRQ
output line is wired to the master's IRQ2 in hw/intc/i8259.c for
isa-i8259, and the kernel for kvm-i8259.  The master's IRQ output line
is wired up by board code (it's complicated).

Correct?  If yes, I can turn it into a suitable comment.

Il 16/10/2013 11:51, Markus Armbruster ha scritto:
> Let me try to elaborate, to make sure I understand.
> 
> Unlike ordinary ISA devices, the i8259 devices need additional wiring,
> done by code.
> 
> For instance, board code like pc_q35_init(), pc_piix.c's pc_init1(),
> mips_malta_init(), ... wire up their IRQ input lines.  The slave's IRQ
> output line is wired to the master's IRQ2 in hw/intc/i8259.c for
> isa-i8259, and the kernel for kvm-i8259.  The master's IRQ output line
> is wired up by board code (it's complicated).
> 
> Correct?  If yes, I can turn it into a suitable comment.

The wiring of the slave to the master is hardcoded into i8259 code.

The wiring of all 16 lines is set up by board code.

Paolo

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 16/10/2013 11:51, Markus Armbruster ha scritto:
>> Let me try to elaborate, to make sure I understand.
>> 
>> Unlike ordinary ISA devices, the i8259 devices need additional wiring,
>> done by code.
>> 
>> For instance, board code like pc_q35_init(), pc_piix.c's pc_init1(),
>> mips_malta_init(), ... wire up their IRQ input lines.  The slave's IRQ
>> output line is wired to the master's IRQ2 in hw/intc/i8259.c for
>> isa-i8259, and the kernel for kvm-i8259.  The master's IRQ output line
>> is wired up by board code (it's complicated).
>> 
>> Correct?  If yes, I can turn it into a suitable comment.
>
> The wiring of the slave to the master is hardcoded into i8259 code.
>
> The wiring of all 16 lines is set up by board code.

Got it, thanks!

On Wed, 16 Oct 2013, Paolo Bonzini wrote:
> Il 16/10/2013 11:51, Markus Armbruster ha scritto:
>> Let me try to elaborate, to make sure I understand.
>>
>> Unlike ordinary ISA devices, the i8259 devices need additional wiring,
>> done by code.
>>
>> For instance, board code like pc_q35_init(), pc_piix.c's pc_init1(),
>> mips_malta_init(), ... wire up their IRQ input lines.  The slave's IRQ
>> output line is wired to the master's IRQ2 in hw/intc/i8259.c for
>> isa-i8259, and the kernel for kvm-i8259.  The master's IRQ output line
>> is wired up by board code (it's complicated).
>>
>> Correct?  If yes, I can turn it into a suitable comment.
>
> The wiring of the slave to the master is hardcoded into i8259 code.

A bit off topic but this reminded me of these patches:

http://patchwork.ozlabs.org/patch/206753/
http://patchwork.ozlabs.org/patch/208252/

which never got merged. Is there a chance that these fixes get merged 
sometimes or is there an explanation why it won't be fixed? As far as I 
remember the patches were reviewed and multiple versions were proposed but 
at the end no decision was reached on which one to merge and it was just 
left uncorrected.

Thanks,
BALATON Zoltan

Il 16/10/2013 18:21, BALATON Zoltan ha scritto:
> On Wed, 16 Oct 2013, Paolo Bonzini wrote:
>> Il 16/10/2013 11:51, Markus Armbruster ha scritto:
>>> Let me try to elaborate, to make sure I understand.
>>>
>>> Unlike ordinary ISA devices, the i8259 devices need additional wiring,
>>> done by code.
>>>
>>> For instance, board code like pc_q35_init(), pc_piix.c's pc_init1(),
>>> mips_malta_init(), ... wire up their IRQ input lines.  The slave's IRQ
>>> output line is wired to the master's IRQ2 in hw/intc/i8259.c for
>>> isa-i8259, and the kernel for kvm-i8259.  The master's IRQ output line
>>> is wired up by board code (it's complicated).
>>>
>>> Correct?  If yes, I can turn it into a suitable comment.
>>
>> The wiring of the slave to the master is hardcoded into i8259 code.
> 
> A bit off topic but this reminded me of these patches:
> 
> http://patchwork.ozlabs.org/patch/206753/
> http://patchwork.ozlabs.org/patch/208252/
> 
> which never got merged. Is there a chance that these fixes get merged
> sometimes or is there an explanation why it won't be fixed? As far as I
> remember the patches were reviewed and multiple versions were proposed
> but at the end no decision was reached on which one to merge and it was
> just left uncorrected.

Right, thank you very much.  ISTR the unanswered question was what to do
about migration, but I need to reread all the threads.

Paolo

On Wed, Oct 16, 2013 at 06:23:11PM +0200, Paolo Bonzini wrote:
> Il 16/10/2013 18:21, BALATON Zoltan ha scritto:
> > A bit off topic but this reminded me of these patches:
> >
> > http://patchwork.ozlabs.org/patch/206753/
> > http://patchwork.ozlabs.org/patch/208252/
> >
> > which never got merged. Is there a chance that these fixes get merged
> > sometimes or is there an explanation why it won't be fixed? As far as I
> > remember the patches were reviewed and multiple versions were proposed
> > but at the end no decision was reached on which one to merge and it was
> > just left uncorrected.
>
> Right, thank you very much.  ISTR the unanswered question was what to do
> about migration, but I need to reread all the threads.
>
> Paolo

Essentially correct.

Although the 8259 (interrupts) model is clearly wrong with respect
to clearing an IRQ request line, only one ancient unimportant guest
(Microport UNIX ca. 1987) seems to care, and there are potentially
significant risks to more important guests if we try to fix it:

Risks: The 8254 (timers) model is wrong in various ways, some of
which are hidden by the incorrect 8259 model, and fixing it could
potentially break migration, depending on exact circumstances.
Also, it isn't clear if there are other device models depending
on the incorrect 8259 that would also need to be fixed.

Similar changes are needed in KVM for consistency, although some of
the 8254 modes are implemented in a more simplistic way (pulses
handled "as fast as possible" directly, instead of
1-millisecond-long pulses on real hardware).  Note that I was
never able to get my guest running successfully under KVM; I'm
not sure what the remaining problems were.

Also, the patch series included a few other things:
  - A couple of low priority fixes that can still be worked
    around without code changes, but could probably qualify
    as "trivial patches".
  - Some test cases to test for the 8259 problem.
  - Plus an optional VGA hack to make it work when
    my ancient guest tries to directly (no BIOS) configure it
    for CGA text mode.
I didn't get much feedback about these.

-----

If someone actually showed real interest in actually merging
these, including the selection of a migration compatibility
strategy they would actually be willing to merge (and above:
other devices, KVM, etc), I could look into updating
the patches to match.  But the "if" parts aren't looking
particularly likely.  This seems like a rather core-level
wide-implication change for a newbie to be messing
with.  (I've already spent noticably more time on qemu
patches than I had intended to spend total on playing with
this guest, although I may continue if I have a clearly
defined strategy.)

                - Matthew Ogilvie

On Sat, 26 Oct 2013, Matthew Ogilvie wrote:
> Although the 8259 (interrupts) model is clearly wrong with respect
> to clearing an IRQ request line, only one ancient unimportant guest
> (Microport UNIX ca. 1987) seems to care, and there are potentially
> significant risks to more important guests if we try to fix it:

There's at least one more guest that cares I know about which is less 
ancient but maybe just as unimportant: OPENSTEP for Mach. But nevertheless 
it still is a now known bug which just seems to be tolerated by the OS-es 
that are most commonly run under Qemu. What was not clear to me is how 
significant are the risks of the fix and if they were considered or the 
patch was just forgotten without ever getting the thought about merging 
it.

> Risks: The 8254 (timers) model is wrong in various ways, some of
> which are hidden by the incorrect 8259 model, and fixing it could
> potentially break migration, depending on exact circumstances.
> Also, it isn't clear if there are other device models depending
> on the incorrect 8259 that would also need to be fixed.

I had the impression from previous discussion that the main risk was a 
potential lost timer interrupt in some circumstances at migration which 
may affect some guests but it was not clear (to me at least) how big of a 
risk is this. IMO if other models depend on a bug they are also buggy and 
should be fixed but I don't know how many models could that affect.

> If someone actually showed real interest in actually merging
> these, including the selection of a migration compatibility
> strategy they would actually be willing to merge (and above:
> other devices, KVM, etc), I could look into updating
> the patches to match.  But the "if" parts aren't looking
> particularly likely.  This seems like a rather core-level
> wide-implication change for a newbie to be messing
> with.  (I've already spent noticably more time on qemu
> patches than I had intended to spend total on playing with
> this guest, although I may continue if I have a clearly
> defined strategy.)

I think you have already provided detailed analysis, test cases and 
multiple options and patch versions so it is not you who should spend more 
time on this now. What I think would be needed is that people who have the 
knowledge and insight to analyse and decide about the patches give it some 
time to think about it and come to a decision then tell what to do or why 
it's better to leave it unfixed. Can this be done in this thread? Or maybe 
on one of the upcoming phone conferences where the right people are 
together anyway to discuss it?

Regards,
BALATON Zoltan