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