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

[Sukadev Bhattiprolu]

Alexey Kardashevskiy address@hidden wrote:
<snip>

| >| When exactly does a socket become a module? The SPAPR spec uses "sockets" 
here.
| >
| >I am trying to get the terminology too :-) Is socket a slot where a
| >module is attached?
| 
| Sorry, no idea.

Ok.

| 
| 
| >
| >I will change the variable name 'modules' to 'sockets'.
| >|
| >|
| >| >+    modinfo->si[0].chips = chips;
| >| >+    modinfo->si[0].cores_per_chip = cores / chips;
| >|
| >|
| >| What if no "ibm,chip-id" was found and chips == 0?
| >
| >If we fail to readdir(xscom) or fail to read the 'ibm,chip-id',
| >we return an error which we check above.
| 
| 
| You assume that if there is /proc/device-tree, then there is always
| "xscom@" but this might not be always the case, like PR KVM on
| embedded PPC64.

For ibm,chip-id, we do try to read the file (and eliminate duplicates
chip ids) If we can't read the file (hash_table_add_contents()) we
return an error, but will check again.

| 
| 
| 
| >
| >|
| >|
| >| >+
| >| >+    return 0;
| >| >+}
| >| >diff --git a/hw/ppc/spapr_rtas_modinfo.h b/hw/ppc/spapr_rtas_modinfo.h
| >| >new file mode 100644
| >| >index 0000000..356a2b5
| >| >--- /dev/null
| >| >+++ b/hw/ppc/spapr_rtas_modinfo.h
| >|
| >|
| >| This file is missing a license and
| >| #ifndef SPAPR_RTAS_MODINFO_H
| >| #define SPAPR_RTAS_MODINFO_H
| >| #endif
| >|
| >| But I'd rather put the content to include/hw/ppc/spapr.h and avoid
| >| creating new files.
| >
| >Ok.
| >
| >|
| >|
| >| >@@ -0,0 +1,12 @@
| >| >+
| >| >+struct rtas_socket_info {
| >| >+    unsigned short sockets;
| >| >+    unsigned short chips;
| >| >+    unsigned short cores_per_chip;
| >| >+};
| >| >+struct rtas_module_info {
| >| >+    unsigned short module_types;
| >| >+    struct rtas_socket_info si[1];
| >| >+};
| >|
| >|
| >| Will the number of rtas_socket_info be always a constant or (sure,
| >| in the future) it may take different values?
| >
| >TBH, I am not sure. One thing though, array size of rtas_socket_info
| >depends on number of module _types_. Like Nishanth Aravamudan mentioned
| >offline, we are not sure if both a DCM and SCM types can be on a system
| >at once (or even if they can be added dynamically).
| 
| What are DCM and SCM? What type do we have in Tuleta?

Dual Chip Module and Single Chip Module. Maybe its standard configuration
but on our Tuleta, we have a DCM (2 chips per module): 

        $ lsprop /proc/device-tree/xscom*/ibm,hw-module-id
        /proc/device-tree/address@hidden/ibm,hw-module-id
                         00000000
        /proc/device-tree/address@hidden/ibm,hw-module-id
                         00000000
        /proc/device-tree/address@hidden/ibm,hw-module-id
                         00000001
        /proc/device-tree/address@hidden/ibm,hw-module-id
                         00000001

Each xscom represents a chip. Two chips have module id 0, two have
module id 1.

| 
| 
| >Another thing I have on my list to check is how this works with
| >hotplug CPU (in the host). If the device-tree is properly updated
| >then these calls will return the updated info? The values in the
| >array will change but the number of entries (types) in the array is
| >still 1.
| >
| >|
| >| Normally when you invent API like rtas_get_module_info(), you tell
| >| the helper how much memory is allocated in the rtas_module_info
| >| struct (in our case it is ARRAY_SIZE(rtas_module_info.si) which is
| >| fine) and then the helper signals somehow how many module types it
| >| has found (which is missing here).
| >
| >Can we assume that the number of types is static for now i.e both
| >caller and callee know the size (although I should fix the computation
| >of size in spapr_rtas.c and use MAX_MODULE_TYPES that David pointed out).
| 
| 
| I do not know what these modules can be so I cannot advise on this,
| sorry. I honestly do not see much point in implementing this system
| parameter - what will the guest do with this information?

AFAICT, this system parameter is needed for distro licensing on powerKVM :-)

| 
| You could get rid of rtas_module_info for now and have a helper for
| rtas_socket_info only - rtas_get_xxx_module_info() (where xxx is DCM
| or SCM or whatever it is now and what we support). Then it would be
| up to rtas_ibm_get_system_parameter() to decide how many modules
| types we want to expose to the guest (now - one) and when we get a
| new module type, we will either extend rtas_get_xxx_module_info() or
| implement another helper.
| 
| 
| -- 
| Alexey