Re: [Qemu-devel] [PATCH v3 064/197] killall VIOsPAPRDeviceInfo

[David Gibson <dwg@au1.ibm.com>]

On Mon, Dec 12, 2011 at 08:25:51PM -0600, Anthony Liguori wrote:
> On 12/12/2011 08:22 PM, Michael Ellerman wrote:
> >On Mon, 2011-12-12 at 20:10 -0600, Anthony Liguori wrote:
> >>On 12/12/2011 08:04 PM, Michael Ellerman wrote:
> >>>On Mon, 2011-12-12 at 14:19 -0600, Anthony Liguori wrote:
> >>>>This was doing something evil building a dt tree so we broke the device.
> >>>
> >>>>@@ -711,8 +711,12 @@ VIOsPAPRBus *spapr_vio_bus_init(void)
> >>>>       spapr_rtas_register("ibm,set-tce-bypass", rtas_set_tce_bypass);
> >>>>       spapr_rtas_register("quiesce", rtas_quiesce);
> >>>>
> >>>>+#if 0
> >>>>+    /* Evil and broken */
> >>>
> >>>By which you mean: works fine, broken by your patch?

Um, yeah.  It may have been evil, but it wasn't broken, whereas it
certainly is broken by this patch.

> >>These patches were never supposed to go out.  Ignore this series entirely.

Phew.

> >But I just read all 197 of them ! ;)
> >
> >>>>       for (qinfo = device_info_list; qinfo; qinfo = qinfo->next) {
> >>>>           VIOsPAPRDeviceInfo *info = (VIOsPAPRDeviceInfo *)qinfo;
> >>>>+        VIOsPAPRDeviceClass *pc = VIO_SPAPR_DEVICE_GET_CLASS(dev);
> >>>>
> >>>>           if (qinfo->bus_info !=&spapr_vio_bus_info) {
> >>>>               continue;
> >>>>@@ -722,6 +726,7 @@ VIOsPAPRBus *spapr_vio_bus_init(void)
> >>>>               info->hcalls(bus);
> >>>>           }
> >>>>       }
> >>>>+#endif
> >>>
> >>>It's registering hcalls for each class of device we find on the spapr
> >>>vio bus. I don't understand why that is evil, but what do you suggest we
> >>>do instead?
> >>
> >>I talked to David about this, the hcalls can just be registered as part the
> >>device_init entry points.
> >
> >OK I'll talk to him about it. I don't think device_init() works, because
> >we only want to register the hcalls if an instance of the device is
> >instantiated. But I guess we'll come up with something.

Hm, no, actually.  The reason we're stepping through the device infos
rather than vio devices is so that hcalls are registered per device
type rather than per device instance.

> Since the hcalls are well known and CPUState doesn't get touched at
> all, why not register all of the possible hcalls in the VIO bus and
> then use a higher level interface to dispatch to devices?  I think
> that conceptionally makes more sense than the devices directly
> registering hcalls themselves.

No, not really.  It means spapr_vio.c has to contain knowledge of
every possible VIO device.

> I know you're dealing with an existing virtual I/O model, but it's
> strange to have an hcall dispatched directly to a device without
> going through any type of controller/bus hierarchy.  It doesn't fit
> how hardware works very well.

But VIO devices don't work much like real hardware.  Other than the
CRQ and a few other hcalls, which are already handled at the bus
level, there is *no* common processing we can do for the device
specific hcalls.  Routing them through spapr_vio.c would just be
pointless redirection.


Anyway, I'll make a [atch to move the hypercall registrations to
device_init functions.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson