Re: [Qemu-ppc] [PATCH v3 2/2] target-ppc: Implement rtas_get_sysparm(PROCESSOR_MODULE_INFO)

[Sukadev Bhattiprolu]

David Gibson address@hidden wrote:
| > @@ -240,6 +241,36 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU 
*cpu,
| >      target_ulong ret = RTAS_OUT_SUCCESS;
| >  
| >      switch (parameter) {
| > +    case RTAS_SYSPARM_PROCESSOR_MODULE_INFO: {
| > +        struct sPAPRRTASModuleInfo modinfo;
| > +        int i, size = sizeof(modinfo), offset = 0;
| > +
| > +        memset(&modinfo, 0, size);
| > +        if (kvmppc_rtas_get_module_info(&modinfo)) {
| > +            ret = RTAS_OUT_HW_ERROR;
| > +            break;
| > +        }
| > +
| > +        stw_be_phys(&address_space_memory, buffer+offset, size);
| 
| You're still advertising the full structure size to the guest, even
| though it may be only partially populated.
| 
| That will probably work in practice, but I think we should be
| PAPRishly correct and only output the size that we actually use here.

Ok. Will have kvmppc_rtas_get_module_info() take/update a size parameter
and use that here.

|

<snip>

| > +/* Each core in the system is represented by a directory with the prefix
| > + * 'PowerPC,POWER' in directory /proc/device-tree/cpus/.  Process that
| > + * directory and count the number of cores in the system.
| > + *
| > + * Return 0 if one or more cores are found. Return -1 otherwise.
| > + */
| > +static int kvmppc_count_cores_dt(int *num_cores)
| > +{
| > +    int rc;
| > +    glob_t dtglob;
| > +    const char *cpus_pattern = "/proc/device-tree/cpus/PowerPC,POWER*";
| 
| Under KVM PR, this could still be too specific to IBM machines.  I
| think it's probably safer to just use /proc/device-tree/cpus/*, I
| don't *think* we get anything under /cpus that isn't a cpu node.

Well, on my Tuleta system (3.18.22-355.el7_1.pkvm3_1_0.3700.3.ppc64le)
I see several l2-cache, l3-cache entries as well as some properties
(like phandle, #size-cells) besides the PowerPC,POWER* entries.

$ cd /proc/device-tree/cpus

$ lsprop address@hidden/device_type
address@hidden/device_type
                 "cache"

$ lsprop address@hidden/device_type
address@hidden/device_type
                 "cache"

$ lsprop PowerPC,address@hidden/device_type
PowerPC,address@hidden/device_type
                 "cpu"

Should we walk the /proc/device-tree/cpus/ tree and count only dirs with
device-type "cpu" (rather than relying on the pattern PowerPC,POWER*)?

| 
| In a number of ways I'd actually prefer to move to /cpus/address@hidden in
| general, since that follows the OF generic names recommendation we
| follow for most other nodes.

Do you mean rename '/proc/device-tree/cpus/PowerPC,address@hidden' to
/proc/device-tree/cpus/address@hidden

Thanks,

Sukadev