Re: [Qemu-devel] [PATCH v2 1/1] target-ppc: Implement rtas_get_sysparm(PROCESSOR_MODULE_INFO)

[David Gibson]

On Mon, Nov 09, 2015 at 12:57:48PM +1100, Alexey Kardashevskiy wrote:
> On 11/05/2015 10:06 AM, Sukadev Bhattiprolu wrote:
> >Implement RTAS_SYSPARM_PROCESSOR_MODULE_INFO parameter to rtas_get_sysparm()
> >call in qemu. This call returns the processor module (socket), chip and core
> >information as specified in section 7.3.16.18 of PAPR v2.7.
> >
> >We walk the /proc/device-tree to determine the number of chips, cores and
> >modules in the _host_ system and return this info to the guest application
> >that makes the rtas_get_sysparm() call.
> >
> >We currently hard code the number of module_types to 1, but we should 
> >determine
> >that dynamically somehow later.
> >
> >Thanks to input from Nishanth Aravamudan and Alexey Kardashevsk.
> 
> "iy" is missing :)
> 
> 
> >
> >Signed-off-by: Sukadev Bhattiprolu <address@hidden>
> >---
> >Changelog[v2]:
> >         [Alexey Kardashevsk] Use existing interfaces like ldl_be_p(),
> >                 stw_be_phys(), g_hash_table_new_full(), error_report() 
> > rather
> >                 than re-inventing; fix indentation, function prottypes etc;
> >                 Drop the fts() interface (which doesn't seem to be available
> >                 on mingw32/mingw64) and use opendir() to walk specific
> >                 directories in the directory tree.
> >---
> >  hw/ppc/Makefile.objs        |   1 +
> >  hw/ppc/spapr_rtas.c         |  35 +++++++
> >  hw/ppc/spapr_rtas_modinfo.c | 230 
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  hw/ppc/spapr_rtas_modinfo.h |  12 +++
> >  include/hw/ppc/spapr.h      |   1 +
> >  5 files changed, 279 insertions(+)
> >  create mode 100644 hw/ppc/spapr_rtas_modinfo.c
> >  create mode 100644 hw/ppc/spapr_rtas_modinfo.h
> >
> >diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> >index c1ffc77..57c6b02 100644
> >--- a/hw/ppc/Makefile.objs
> >+++ b/hw/ppc/Makefile.objs
> >@@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
> >  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 spapr_rtc.o spapr_drc.o spapr_rng.o
> >+obj-$(CONFIG_PSERIES) += spapr_rtas_modinfo.o
> >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> >  obj-y += spapr_pci_vfio.o
> >  endif
> >diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> >index 34b12a3..41fd8a6 100644
> >--- a/hw/ppc/spapr_rtas.c
> >+++ b/hw/ppc/spapr_rtas.c
> >@@ -34,6 +34,8 @@
> >  #include "hw/ppc/spapr.h"
> >  #include "hw/ppc/spapr_vio.h"
> >  #include "qapi-event.h"
> >+
> >+#include "spapr_rtas_modinfo.h"
> >  #include "hw/boards.h"
> >
> >  #include <libfdt.h>
> >@@ -240,6 +242,39 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU 
> >*cpu,
> >      target_ulong ret = RTAS_OUT_SUCCESS;
> >
> >      switch (parameter) {
> >+    case RTAS_SYSPARM_PROCESSOR_MODULE_INFO: {
> >+        int i;
> >+        int offset = 0;
> >+        int size;
> 
> Nit - could be one line.
> 
> 
> >+        struct rtas_module_info modinfo;
> >+
> >+        if (rtas_get_module_info(&modinfo)) {
> >+            break;
> 
> 
> @ret will be still 0 in this case, set @ret to RTAS_OUT_HW_ERROR here.
> 
> Also, rtas_ibm_set_system_parameter() must return RTAS_OUT_NOT_AUTHORIZED on
> RTAS_SYSPARM_PROCESSOR_MODULE_INFO, as PAPR says.
> 
> 
> >+        }
> >+
> >+        size = sizeof(modinfo);
> >+        size += (modinfo.module_types - 1) * sizeof(struct 
> >rtas_socket_info);
> >+
> >+        stw_be_phys(&address_space_memory, buffer+offset, size);
> >+        offset += 2;
> >+
> >+        stw_be_phys(&address_space_memory, buffer+offset, 
> >modinfo.module_types);
> >+        offset += 2;
> >+
> >+        for (i = 0; i < modinfo.module_types; i++) {
> >+            stw_be_phys(&address_space_memory, buffer+offset,
> >+                                    modinfo.si[i].sockets);
> 
> 
> checkpatch.pl does not warn on this but new lines start under opening brace
> in the previous line.
> 
> In terms on vim, it would be:
> set expandtab
> set tabstop=4
> set shiftwidth=4
> set cino=:0,(0
> 
> 
> 
> >+            offset += 2;
> >+            stw_be_phys(&address_space_memory, buffer+offset,
> >+                                    modinfo.si[i].chips);
> >+            offset += 2;
> >+            stw_be_phys(&address_space_memory, buffer+offset,
> >+                                    modinfo.si[i].cores_per_chip);
> >+            offset += 2;
> >+        }
> >+        break;
> >+    }
> >+
> >      case RTAS_SYSPARM_SPLPAR_CHARACTERISTICS: {
> >          char *param_val = g_strdup_printf("MaxEntCap=%d,"
> >                                            "DesMem=%llu,"
> >diff --git a/hw/ppc/spapr_rtas_modinfo.c b/hw/ppc/spapr_rtas_modinfo.c
> >new file mode 100644
> >index 0000000..068fc2c
> >--- /dev/null
> >+++ b/hw/ppc/spapr_rtas_modinfo.c
> >@@ -0,0 +1,230 @@
> >+/*
> >+ * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System 
> >Emulator
> >+ *
> >+ * Hypercall based emulated RTAS
> 
> 
> This is a description of hw/ppc/spapr_rtas.c, not of the new file.
> 
> Not sure the new code deserves a separate file, I'd either:
> - add it into hw/ppc/spapr_rtas.c OR
> - move rtas_ibm_get_system_parameter() + rtas_ibm_set_system_parameter() to
> a separate file (let's call it  hw/ppc/spapr_rtas_syspar.c) and add this new
> parameter there as there will be new parameters in the future anyway.
> 
> But I'll leave to the maintainer (David, hi :) ).

Actually it probably should go in target-ppc/kvm.c.  Looking up things
on the host in this way only makes sense for KVM, and we have other
code in there that looks in the host /proc/device-tree.

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

signature.asc
Description: PGP signature