Re: [Qemu-devel] [PATCH 1/5] pseries: Move sPAPR RTC code into its own file

[David Gibson]

On Fri, Dec 19, 2014 at 12:36:11AM +0100, Alexander Graf wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> 
> 
> On 18.12.14 06:43, David Gibson wrote:
> > On Tue, Dec 16, 2014 at 10:41:16AM +0100, Alexander Graf wrote:
> >> 
> >> 
> >> 
> >>> Am 16.12.2014 um 02:24 schrieb David Gibson
> >>> <address@hidden>:
> >>> 
> >>>> On Tue, Dec 16, 2014 at 01:59:01AM +0100, Alexander Graf
> >>>> wrote:
> >>>> 
> >>>> 
> >>>>> On 16.12.14 01:43, David Gibson wrote: At the moment the
> >>>>> RTAS (firmware/hypervisor) time of day functions are 
> >>>>> implemented in spapr_rtas.c along with a bunch of other
> >>>>> things.  Since we're going to be expanding these a bit,
> >>>>> move the RTAS RTC related code out into new file
> >>>>> spapr_rtc.c.  Also add its own initialization function, 
> >>>>> spapr_rtc_init() called from the main machine init
> >>>>> routine.
> >>>>> 
> >>>>> Signed-off-by: David Gibson <address@hidden> 
> >>>>> --- hw/ppc/Makefile.objs   |  2 +- hw/ppc/spapr.c         |
> >>>>> 3 ++ hw/ppc/spapr_rtas.c    | 49
> >>>>> ----------------------------- hw/ppc/spapr_rtc.c     | 83
> >>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++ 
> >>>>> include/hw/ppc/spapr.h |  1 + 5 files changed, 88
> >>>>> insertions(+), 50 deletions(-) create mode 100644
> >>>>> hw/ppc/spapr_rtc.c
> >>>>> 
> >>>>> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs 
> >>>>> index 19d9920..437955d 100644 --- a/hw/ppc/Makefile.objs 
> >>>>> +++ b/hw/ppc/Makefile.objs @@ -3,7 +3,7 @@ obj-y += ppc.o
> >>>>> ppc_booke.o # IBM pSeries (sPAPR) obj-$(CONFIG_PSERIES) +=
> >>>>> spapr.o spapr_vio.o spapr_events.o obj-$(CONFIG_PSERIES) +=
> >>>>> spapr_hcall.o spapr_iommu.o spapr_rtas.o 
> >>>>> -obj-$(CONFIG_PSERIES) += spapr_pci.o 
> >>>>> +obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o ifeq
> >>>>> ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy) obj-y
> >>>>> += spapr_pci_vfio.o endif diff --git a/hw/ppc/spapr.c
> >>>>> b/hw/ppc/spapr.c index 30de25d..16377a3 100644 ---
> >>>>> a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1446,6 +1446,9 @@
> >>>>> static void ppc_spapr_init(MachineState *machine) /* Set up
> >>>>> EPOW events infrastructure */ spapr_events_init(spapr);
> >>>>> 
> >>>>> +    /* Set up the RTC RTAS interfaces */ +
> >>>>> spapr_rtc_init();
> >>>> 
> >>>> Do you think we could make it a device instead?
> >>> 
> >>> Um.. I guess.  Is there a standard place to put such
> >>> pseudo-devices in the bus heirarchy?
> >> 
> >> How about we combine this with some cleanup work and create a
> >> new spapr bus that (in the long term) exposes all the details we
> >> carry around in the spapr struct to its children via the bus?
> > 
> > I've thought about this a bit more.  It really doesn't make sense
> > to put the rtc device anywhere except the root bus (which
> > optionally could become an spapr sub-type of the default root bus
> > type).
> 
> Well, we could always just leave sysbus completely unused and instead
> create our own global spapr root bus. In fact, that's probably what we
> should do ;).

We could, but I absolutely disagree that it's what we should do.

What is the sysbus for, if not to represent devices on the system that
don't fall under some other bus bridge.  Putting an spapr bus under it
is indirection for no purpose.

> If that blows your mind for this patch set, we can make the RTC a
> sysbus device too though.
> 
> > But, while splitting this into its own device would be cleaner, I 
> > don't think we should delay real behavioural fixes for that
> > cleanup. Working out how to split out the device without breaking
> > old machine types, and keeping migration working is making my head
> > hurt.
> 
> I don't think it's that hard. Just create a compat version for version
> 2 of "spapr" vmstates and have a post_load hook in there that shoves
> the rtc offset into the new rtc device via a qom property. You should
> even be able to just find the RTC device by path.

Yeah, I kind of figured that out myself in the meantime.  It's bit
ugly, because we need to retain the otherwise useless rtc_offset field
in sPAPREnvironment just to be loaded by the vmstatdescription then
pushed into the rtc, but I guess it works.

> Keep in mind that we don't support new -> old migration.

-- 
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

pgpDBhWjw5PPL.pgp
Description: PGP signature