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

[Sukadev Bhattiprolu]

David Gibson address@hidden wrote:
| On Mon, Nov 09, 2015 at 08:22:32PM -0800, Sukadev Bhattiprolu wrote:
| > David Gibson address@hidden wrote:
| > | On Wed, Nov 04, 2015 at 03:06:05PM -0800, 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.
| > | 
| > | PAPR v2.7 isn't available publically.  For upstream patches, please
| > | reference LoPAPR instead (where it's section 7.3.16.17 AFAICT).
| > 
| > Ok.
| > 
| > | 
| > | > 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.
| > | > 
| > | > Signed-off-by: Sukadev Bhattiprolu <address@hidden>
| > | 
| > | This isn't ready to go yet - you need to put some more consideration
| > | into the uncommon cases: PR KVM, TCG and non-Power hosts.
| > 
| > Ok. Is there a we can make this code applicable only a Powerpc host?
| > (would moving this code to target-ppc/kvm.c do that?)
| 
| Yes, moving it to target-ppc/kvm.c would mostly do that.  You'd need
| some logic to make sure it fails gracefully in other cases, of course.

Ok. I have added a stub under #ifndef KVM in target-ppc/kvm_ppc.h.

| 
| [snip]
| > | >      switch (parameter) {
| > | > +    case RTAS_SYSPARM_PROCESSOR_MODULE_INFO: {
| > | > +        int i;
| > | > +        int offset = 0;
| > | > +        int size;
| > | > +        struct rtas_module_info modinfo;
| > | > +
| > | > +        if (rtas_get_module_info(&modinfo)) {
| > | > +            break;
| > | > +        }
| > | 
| > | So, you handle the variable size of this structure before sending it
| > | to the guest, but you don't handle it in allocation of the structure
| > | right here.  You'll get away with that because for now you only ever
| > | have one entry in the sockets array, but it's a bit icky.
| > 
| > Can we assume that the size is static for now...
| > | 
| > | > +
| > | > +        size = sizeof(modinfo);
| > | > +        size += (modinfo.module_types - 1) * sizeof(struct 
rtas_socket_info);
| > | 
| > | More seriously, this calculation will break horribly if you change the
| > | size of the array in struct rtas_module_info.
| > 
| > and just set 'size' to sizeof(modinfo)?.
| 
| For purposes of allocation you could just use a fixed size.  But the
| guest might get confused by additional data beyond the declared size,
| so you do need to get the value correct that you send back to the guest.

If we use a fixed size and memset the buffer to 0 and then set the
values would that be sufficient?

| 
| [snip]
| > | > +/*
| > | > + * Each module's (aka socket's) id is contained in the 
'ibm,hw-module-id'
| > | > + * file in the "xscom" directory (/proc/device-tree/xscom*). Similarly 
each
| > | > + * chip's id is contained in the 'ibm,chip-id' file in the xscom 
directory.
| > | > + *
| > | > + * A module can contain more than one chip and a chip can contain more
| > | > + * than one core. So there are likely to be duplicates in the module
| > | > + * and chip identifiers (i.e more than one xscom directory can contain
| > | > + * the same module/chip id).
| > | > + *
| > | > + * Search the xscom directories and count the number of _UNIQUE_ module
| > | > + * and chip identifiers in the system.
| > | 
| > | There's no direct way to go from a core
| > | (i.e. /proc/device-tree/cpus/address@hidden) to the corresponding chip 
and/or
| > | module?
| > 
| > Yes, it would logical to find the chip and module from the core :-)
| > 
| > While 'ibm,chip-id' is in the core dir (/proc/device-tree/cpus/PowerPC,*/), 
| > the 'ibm,hw-module-id' is not there (on my Tuleta system). Maybe the
| > 'ibm,hw-module-id' will be added in the future?
| 
| Hm, I see.  Is there any device node that represents the "chip"?

AFAICT, in the current skiboot code, xscom represents a chip.

| 
| > I am using the xscom node to be consistent in counting chips and modules.
| 
| The trouble with xscom is that it's extremely specific to the way the
| current IBM servers present things.  It won't work on other types of
| host machine (which could happen with PR KVM), and could even break if
| IBM changes the way it organizes the SCOMs in a future machine.

Not sure yet how to future proof this at this point. If the SCOMs change
or are removed, we will end up returning zeroes (and break the licensing
again)

| 
| Working from the nodes in /cpus still has some dependencies on IBM
| specific properties, but it's at least partially based on OF
| standards.

yes, I will check with skiboot folks to see if they can add the
hw-module-id for each core. 

Sukadev