Re: [Qemu-devel] KVM call minutes 2013-01-29 - Port I/O

[Michael S. Tsirkin]

On Wed, Jan 30, 2013 at 11:29:58AM -0600, Anthony Liguori wrote:
> Andreas Färber <address@hidden> writes:
> 
> > Am 30.01.2013 17:33, schrieb Anthony Liguori:
> >> Gerd Hoffmann <address@hidden> writes:
> >> 
> >>>> hw/qxl.c:    portio_list_add(qxl_vga_port_list,
> >>>> pci_address_space_io(dev), 0x3b0);
> >>>> hw/vga.c:        portio_list_add(vga_port_list, address_space_io, 0x3b0);
> >>>
> >>> That reminds me I should solve this in a more elegant way.
> >>>
> >>> qxl takes over the vga io ports.  The reason it does this is because qxl
> >>> switches into vga mode in case the vga ports are accessed while not in
> >>> vga mode.  After doing the check (and possibly switching mode) the vga
> >>> handler is called to actually handle it.
> >> 
> >> The best way to handle this would be to remodel how we do VGA.
> >> 
> >> Make VGACommonState a proper QOM object and use it as the base class for
> >> QXL, CirrusVGA, QEMUVGA (std-vga), and VMwareVGA.
> >
> > That would require polymorphism since we already need to derive from
> > PCIDevice or ISADevice respectively for interfacing with the bus...
> 
> Nope.  You can use composition:
> 
> QXLDevice is-a VGACommonState
> 
> QXLPCI is-a PCIDevice
>        has-a QXLDevice

But why like this?
The distinction is artificial, isn't it?

> > Modern object-oriented languages have tried to avoid multi-inheritence
> > due to arising complications, I thought. Wouldn't object if someone
> > wanted to do the dirty implementation work though. ;)
> 
> There is no need for MI.
> 
> > Another such example is EHCI, with PCIDevice and SysBusDevice frontends,
> > sharing an EHCIState struct and having helper functions operating on
> > that core state only. Quite a few device share such a pattern today
> > actually (serial, m48t59, ...).
> 
> Yes, this is all about chipset modelling.  Chipsets should derive from
> device and then be embedded in the appropriate bus device.
> 
> For instance.
> 
> SerialState is-a DeviceState
> 
> ISASerialState is-a ISADevice, has-a SerialState
> MMIOSerialState is-a SysbusDevice, has-a SerialState

ISASerialState is not a SerialState?
Hmm but why?

> This is what we're doing in practice, we just aren't modeling the
> chipsets and we're open coding the relationships (often in subtley
> different ways).
> 
> Regards,
> 
> Anthony Liguori
> 
> >> The VGA accessors should be exposed as a memory region but the sub class
> >> ought to be responsible for actually adding it to a subregion.
> >> 
> >>>
> >>> That twist makes it a bit hard to convert vga ...
> >>>
> >>> Anyone knows how one would do that with the memory api instead? I think
> >>> taking over the ports is easy as the memory regions have priorities so I
> >>> can simply register a region with higher priority. I have no clue how to
> >>> forward the access to the vga code though.
> >>>
> >> 
> >> That should be possible with priorities, but I think it's wrong.  There
> >> aren't two VGA devices.  QXL is-a VGA device and the best way to
> >> override behavior of base VGA device is through polymorphism.
> >
> > In this particular case QXL is-a PCI VGA device though, so we can
> > decouple it from core VGA modeling. Placing the MemoryRegionOps inside
> > the Class (rather than static const) might be a short-term solution for
> > overriding read/write handlers of a particular VGA MemoryRegion. :)
> >
> > Cheers,
> > Andreas
> >
> >> This isn't really a memory API issue, it's a modeling issue.
> >> 
> >> Regards,
> >> 
> >> Anthony Liguori
> >> 
> >>> Anyone has clues / suggestions?
> >>>
> >>> thanks,
> >>>   Gerd
> >
> > -- 
> > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg