[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [QEMU-PPC] [PATCH v3] powerpc/spapr: Add host threads par
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [QEMU-PPC] [PATCH v3] powerpc/spapr: Add host threads parameter to ibm, get_system_parameter |
Date: |
Thu, 4 Jul 2019 14:59:54 +1000 |
User-agent: |
Mutt/1.12.0 (2019-05-25) |
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 <address@hidden>
> >
> > 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