From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45550) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y1uL0-0003dg-Rx for qemu-devel@nongnu.org; Fri, 19 Dec 2014 04:59:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y1uKw-0006kk-2O for qemu-devel@nongnu.org; Fri, 19 Dec 2014 04:59:10 -0500 Received: from cantor2.suse.de ([195.135.220.15]:51639 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y1uKv-0006kc-OR for qemu-devel@nongnu.org; Fri, 19 Dec 2014 04:59:06 -0500 Message-ID: <5493F6E6.1070805@suse.de> Date: Fri, 19 Dec 2014 10:59:02 +0100 From: Alexander Graf MIME-Version: 1.0 References: <1418690628-20652-1-git-send-email-david@gibson.dropbear.id.au> <1418690628-20652-2-git-send-email-david@gibson.dropbear.id.au> <548F83D5.3000903@suse.de> <20141216012437.GH23547@voom.fritz.box> <96E329FA-8614-4FD4-8F6E-E345BB84AB7E@suse.de> <20141218054328.GF12159@voom.redhat.com> <549364EB.3070108@suse.de> <20141219053937.GI12159@voom.redhat.com> In-Reply-To: <20141219053937.GI12159@voom.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/5] pseries: Move sPAPR RTC code into its own file List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: "aik@ozlabs.ru" , "paulus@samba.org" , "qemu-devel@nongnu.org" , "mdroth@us.ibm.com" -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 19.12.14 06:39, David Gibson wrote: > 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 >>>>> : >>>>> >>>>>> 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 >>>>>>> --- 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. Sysbus is a big bucket used to represent a number of buses that have devices that * are MMIO or PIO mapped into CPU visible memory regions * have hard IRQ wirings Most of the devices on sPAPR are not mapped into MMIO space at all - they have their own hcall based access methods. For IRQs, sPAPR has its own allocation algorithm as well, which is vastly different from anything we have with SysBus today. I don't think we can reuse much more of SysBusDevice other than everything that we already have in DeviceState - and at that point we can just inherit sPAPRDeviceState from DeviceState and completely ignore SysBus. > >> 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. Yeah, but i think we can live with the 4 bytes of otherwise unused allocated memory ;) Alex -----BEGIN PGP SIGNATURE----- Version: GnuPG/MacGPG2 v2.0.22 (Darwin) Comment: GPGTools - http://gpgtools.org iQIcBAEBAgAGBQJUk/bmAAoJECszeR4D/txgh1cP+gND5VZIEM8DOi7RIBIJrqCi 1xX28arHqiqcNBpOg7zdqkYwdAaUU3XQ9H4TcDa9Xj3jZbU83Hp3UXwFgKZHbTKK MOcI7+aCVECsMrPfLnZvFCqPbuIS8zmTUzHNm8IMlDmt4AK16JbXJQSuCoMG+RB0 iKf0k55tprlEnW67ZO/AL7UTZWGCFq34cLhKqZFNb5GmBJpXu1MO3qg4TrekdlLh /OEPDWMtRVAjsR/kJbokhpe5jPgusDTHu+ZBDtYzTr6wqgTjt3gfs+Jja2VCsaVm vGGfdzM/Hyh04PfLEN22Iyd2y4sWUgeUCWzU3Q2Bjxrw+fPT3wsX78GqnQentFK8 0WQ2UL48vF7drdcVtsqtJYN379Bnz/CH0pUelIWNowjuhLPWwxbx3vDFpcIVq7dy uHvNveTblL56dvf/WLn3ubdwUZ1+WIUiRE1sbQHyc/77/nY+umS500y49tVT3Xuy rP36PyfKEOnciXEeWlOwhsyFWQg/8d3JKhqXAsW2FCuw5pYtKxsK+LVbCgI1GGJP XFM8QRsjbEVJXB5OR7aIxBCEnNrScGyTaH+SPQkoWnV6tlnbXPj3tyJnUyXvEMfk n59xvWvKka7LPERYLqY6TM4rD4524GVivuqdVaeMJ0c1NKp+J3jEBInKGQ/IicD2 yd2JOEvtS3PBNYonukOo =gTRM -----END PGP SIGNATURE-----