Re: [Qemu-devel] [PATCH v1 0/3] add Homer/OCC common area emulation for PowerNV

[Balamuruhan S <bala24@linux.ibm.com>]

On Tue, Sep 10, 2019 at 01:45:55PM +0200, Cédric Le Goater wrote:
> On 10/09/2019 09:10, Balamuruhan S wrote:
> > Hi All,
> > 
> > This is follow-up patch that implements HOMER and OCC SRAM device
> > models to emulate homer memory and occ common area access for pstate
> > table, occ sensors, runtime data and slw.
> > 
> > This version addresses review comments in previous patchset and
> > breaks it to have separate patch series for Homer and OCC emulation,
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg00979.html
> > 
> > currently skiboot disables the homer/occ code path with `QUIRK_NO_PBA`,
> > this quirk have to be removed in skiboot for it to use HOMER and OCC
> > SRAM device models along with few bug fixes,
> > 
> > https://github.com/balamuruhans/skiboot/commit/a655514d2a730e0372a2faee277d1cf01f71a524
> > https://github.com/balamuruhans/skiboot/commit/fd3d93d92ec66a7494346d6d24ced7b48264c9a0
> 
> Can't we generate the sensors in QEMU ? I am not sure what this
> patch does. Is the Header Block invalid ? 

This doesn't directly affect Qemu, this is skiboot bug where it
creates device tree node for sensor-groups and does sensor
sanity check to initialize, but in negative scenario where there
is no sensors like in Qemu the sanity check fails but still device
tree populates the sensor-groups node wrongly. The cleanup is not
handled in skiboot and this patch does that.

> 
> It would be good to generate properties to control their values 
> on the monitor line, like Rashmica did for GPIO model in the 
> Aspeed machine.

> 
> > https://github.com/balamuruhans/skiboot/commit/165b3829a93bc177c18133945a8cca3a2d701173
> 
> This one is weird .

I did a miss here, in skiboot there is check whether parsed pstate id
for pmin and pmax is valid or not. In this check, pmax to pmin for P8
it is 0 to -N and for P9 0 to N. But in Qemu for the MemoryRegionOps
structure read() callback function can have only uint64_t as return
type, so for P8 I got error from skiboot as we return postive value
and misunderstood to make this skiboot change. Cedric how can we handle
this from Qemu ?

Thanks for review,

-- Bala

> 
> C. 
> 
> > 
> > changes from v1:
> >     * reuse PnvOCC device model to implement SRAM device.
> >     * implement PnvHomer as separate device model.
> >     * have core max base address as part of PnvHOMERClass.
> >     * reuse PNV_CHIP_INDEX() instead of introducing new `chip_num`.
> >     * define all the memory ops access address as macros.
> >     * few coding style warnings given by checkpatch.pl.
> > 
> > I request for review, comments and suggestions for the changes.
> > 
> > Balamuruhan S (3):
> >   hw/ppc/pnv_xscom: retrieve homer/occ base address from PBA BARs
> >   hw/ppc/pnv_occ: add sram device model for occ common area
> >   hw/ppc/pnv_homer: add PowerNV homer device model
> > 
> >  hw/ppc/Makefile.objs       |   1 +
> >  hw/ppc/pnv.c               |  87 ++++++++++++---
> >  hw/ppc/pnv_homer.c         | 258 +++++++++++++++++++++++++++++++++++++++++++++
> >  hw/ppc/pnv_occ.c           |  78 ++++++++++++++
> >  hw/ppc/pnv_xscom.c         |  34 +++++-
> >  include/hw/ppc/pnv.h       |  21 ++++
> >  include/hw/ppc/pnv_homer.h |  52 +++++++++
> >  include/hw/ppc/pnv_occ.h   |   3 +
> >  8 files changed, 513 insertions(+), 21 deletions(-)
> >  create mode 100644 hw/ppc/pnv_homer.c
> >  create mode 100644 include/hw/ppc/pnv_homer.h
> > 
>