qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: David Gibson
Subject: Re: [Qemu-devel] [PATCH v3 2/2] target-ppc: Implement rtas_get_sysparm(PROCESSOR_MODULE_INFO)
Date: Wed, 2 Dec 2015 13:32:57 +1100
User-agent: Mutt/1.5.24 (2015-08-30)

On Tue, Dec 01, 2015 at 11:49:31AM -0800, Sukadev Bhattiprolu wrote:
> 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"

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

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: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]