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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3329 bytes --]

On Tue, Dec 01, 2015 at 11:49:31AM -0800, Sukadev Bhattiprolu wrote:
> David Gibson [david@gibson.dropbear.id.au] 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 l3-cache@30000020/device_type
> l3-cache@30000020/device_type
> 		 "cache"
> 
> $ lsprop l2-cache@200008f0/device_type
> l2-cache@200008f0/device_type
> 		 "cache"
> 
> $ lsprop PowerPC,POWER8@860/device_type
> PowerPC,POWER8@860/device_type
> 		 "cpu"

Ah.. right, guess I was wrong.

> 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*)?

Yes, I think you'll have to.

> | 
> | In a number of ways I'd actually prefer to move to /cpus/cpu@NNN 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,POWER8@NNN' to
> /proc/device-tree/cpus/cpu@NNN?

Yes.  This is a firmware matter, so it's not something that can simply
be changed everywhere, but it's the approach that I'd prefer to
encourage for people making future machines and firmwares.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]