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

[Alexander Graf]

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

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