Re: [Qemu-devel] [QEMU-PPC] [PATCH v3] powerpc/spapr: Add host threads parameter to ibm, get_system_parameter

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

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

On Thu, Jul 04, 2019 at 01:41:59PM +1000, Suraj Jitindar Singh wrote:
> On Wed, 2019-07-03 at 16:12 +1000, David Gibson wrote:
> > On Mon, Jul 01, 2019 at 04:19:46PM +1000, Suraj Jitindar Singh wrote:
> > > The ibm,get_system_parameter rtas call is used by the guest to
> > > retrieve
> > > data relating to certain parameters of the system. The SPLPAR
> > > characteristics option (token 20) is used to determin
> > > characteristics of
> > > the environment in which the lpar will run.
> > > 
> > > It may be useful for a guest to know the number of physical host
> > > threads
> > > present on the underlying system where it is being run. Add the
> > > characteristic "HostThrs" to the SPLPAR Characteristics
> > > ibm,get_system_parameter rtas call to expose this information to a
> > > guest and provide an implementation which determines this
> > > information
> > > based on the number of interrupt servers present in the device
> > > tree.
> > > 
> > > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > 
> > Hrm, as I said on our call, I have some misgivings about this.
> > 
> > Starting with the most general: this again publishes host information
> > to the guest without filtering, which has caused us problems before
> > (e.g. security issues with publishing the host serial and model
> > information).  Now, I can't immediately see what harm a guest could
> > do
> > with the host # threads (especially since it could in theory deduce
> > it
> > from the PVR, I think) but it still makes me uneasy.
> 
> Correct, a guest could pretty reliably determine this information
> anyway based on the PVR. It can't account for a POWER8 operating in
> split core mode, but I don't know any harm that could be done by
> introducing this information.
> 
> Additionally it doesn't really tell you anything about how you're going
> to be scheduled (at least on POWER9) since vcpus are scheduled on a per
> thread, not per core basis.

Hmm.

> > Secondly, the "HostThrs" tag doesn't seem to be documented in PAPR as
> > something that this system-parameter will include.  I don't much like
> > the idea of adding ad-hoc bits of information here without some
> > thought going into designing and specifying it first.
> 
> This isn't documented in papr, it has been decided that this is how the
> information will be communicated to a guest. This is the most
> appropriate place to put this information and the HostThrs name is
> consistent with the naming of other information in this property.

Grr.  If someone can decide this, they can bloody well document it
somewhere.

> We have other non-papr information in qemu, for example hcall numbers,
> so this isn't exactly a precedent.

I suppose

> > > ---
> > > 
> > > V1 -> V2:
> > > - Take into account that the core may be operating in split core
> > > mode
> > >   meaning a single core may be split into multiple subcores.
> > > V2 -> V3:
> > > - Add curly braces for single line if statements
> > > ---
> > >  hw/ppc/spapr_rtas.c | 62
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 62 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > > index 5bc1a93271..1bab71c90c 100644
> > > --- a/hw/ppc/spapr_rtas.c
> > > +++ b/hw/ppc/spapr_rtas.c
> > > @@ -229,6 +229,58 @@ static inline int sysparm_st(target_ulong
> > > addr, target_ulong len,
> > >      return RTAS_OUT_SUCCESS;
> > >  }
> > >  
> > > +#define CPUS_PATH       "/proc/device-tree/cpus/"
> > > +#define
> > > SUBCORE_PATH    "/sys/devices/system/cpu/subcores_per_core"
> > > +
> > > +static int rtas_get_num_host_threads(void)
> > > +{
> > > +    int num_threads = -1;
> > > +    unsigned long len;
> > > +    const char *entry;
> > > +    char *buf;
> > > +    GDir *dir;
> > > +
> > > +    if (!kvm_enabled()) {
> > > +        return 1;
> > > +    }
> > > +
> > > +    /* Read interrupt servers to determine number of threads per
> > > core */
> > > +    dir = g_dir_open(CPUS_PATH, 0, NULL);
> > > +    if (!dir) {
> > > +        return -1;
> > > +    }
> > > +
> > > +    while ((entry = g_dir_read_name(dir))) {
> > > +        if (!strncmp(entry, "PowerPC,POWER",
> > > strlen("PowerPC,POWER"))) {
> > > +            char *path;
> > > +
> > > +            path = g_strconcat(CPUS_PATH, entry, "/ibm,ppc-
> > > interrupt-server#s",
> > > +                               NULL);
> > > +            if (g_file_get_contents(path, &buf, &len, NULL)) {
> > > +                num_threads = len / sizeof(int);
> > > +                g_free(buf);
> > > +            }
> > > +
> > > +            g_free(path);
> > > +            break;
> > > +        }
> > > +    }
> > > +
> > > +    g_dir_close(dir);
> > > +
> > > +    /* Check if split core mode in use */
> > > +    if (g_file_get_contents(SUBCORE_PATH, &buf, &len, NULL)) {
> > > +        int subcores = g_ascii_strtoll(buf, NULL, 10);
> > > +
> > > +        if (subcores) {
> > > +            num_threads /= subcores;
> > > +        }
> > > +        g_free(buf);
> > > +    }
> > 
> > Finally, all the logic above is built on the assumption of a ppc host
> > - and not just that but an IBM POWER host...
> 
> RTAS services are defined as being provided by a papr platform, and the
> existence of the ibm,ppc-interrupt-server#s device tree property is a
> requirement of a papr platform. So I don't see this being an issue.

The *guest* is a PAPR platform, there's no guarantee the host has to
be a PAPR platform (in fact it usually won't be, it's just that
powernv has a lot of the same device tree properties).

> > 
> > > +    return num_threads;
> > > +}
> > > +
> > >  static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
> > >                                            SpaprMachineState
> > > *spapr,
> > >                                            uint32_t token, uint32_t
> > > nargs,
> > > @@ -250,6 +302,16 @@ static void
> > > rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
> > >                                            current_machine-
> > > >ram_size / MiB,
> > >                                            smp_cpus,
> > >                                            max_cpus);
> > > +        int num_host_threads = rtas_get_num_host_threads();
> > > +
> > > +        if (num_host_threads > 0) {
> > 
> > ... this sort of implements a fallback in other cases (KVM PR with a
> > non-IBM host, TCG, but the boundary conditions are not really well
> > defined.
> 
> This is essentially catching the error case of
> rtas_get_num_host_threads() returning a negative number or not finding
> the required properties (which as mentioned above are required). The
> KVM-PR case will work the same as the KVM-HV case where the host device
> tree will be queried.

Not if you're using PR on, say, an embedded ppc or an old Apple
machine that doesn't have the PAPR-ish properties in the host device
tree.

> For TCG we just default to 1 since this
> information shouldn't be relevant to a TCG guest.

Uh.. it doesn't though, it omits it entirely.

Also I don't really understand how it's relevant to a KVM guest in the
first place.

> 
> > 
> > > +            char *hostthr_val, *old = param_val;
> > > +
> > > +            hostthr_val = g_strdup_printf(",HostThrs=%d",
> > > num_host_threads);
> > > +            param_val = g_strconcat(param_val, hostthr_val, NULL);
> > > +            g_free(hostthr_val);
> > > +            g_free(old);
> > > +        }
> > >          ret = sysparm_st(buffer, length, param_val,
> > > strlen(param_val) + 1);
> > >          g_free(param_val);
> > >          break;
> > 
> > 
> 

-- 
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: 833 bytes --]