| Message ID | 20180115063235.7518-7-sjitindarsingh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 15, 2018 at 05:32:35PM +1100, Suraj Jitindar Singh wrote: > The new H-Call H_GET_CPU_CHARACTERISTICS is used by the guest to query > behaviours and available characteristics of the cpu. > > Implement the handler for this new H-Call which formulates its response > based on the setting of the spapr_caps cap-cfpc, cap-sbbc and cap-ibs. > > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> > --- > hw/ppc/spapr_hcall.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++ > include/hw/ppc/spapr.h | 1 + > 2 files changed, 67 insertions(+) > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > index 51eba52e86..a693d3b852 100644 > --- a/hw/ppc/spapr_hcall.c > +++ b/hw/ppc/spapr_hcall.c > @@ -1654,6 +1654,69 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, > return H_SUCCESS; > } > > +static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu, > + sPAPRMachineState *spapr, > + target_ulong opcode, > + target_ulong *args) > +{ > + uint64_t characteristics = H_CPU_CHAR_HON_BRANCH_HINTS & > + ~H_CPU_CHAR_THR_RECONF_TRIG; > + uint64_t behaviour = H_CPU_BEHAV_FAVOUR_SECURITY; > + uint8_t safe_cache = spapr_get_cap(spapr, SPAPR_CAP_CFPC); > + uint8_t safe_bounds_check = spapr_get_cap(spapr, SPAPR_CAP_SBBC); > + uint8_t safe_indirect_branch = spapr_get_cap(spapr, SPAPR_CAP_IBS); > + > + switch (safe_cache) { > + case SPAPR_CAP_WORKAROUND: > + characteristics |= H_CPU_CHAR_L1D_FLUSH_ORI30; > + characteristics |= H_CPU_CHAR_L1D_FLUSH_TRIG2; > + characteristics |= H_CPU_CHAR_L1D_THREAD_PRIV; > + behaviour |= H_CPU_BEHAV_L1D_FLUSH_PR; > + break; > + case SPAPR_CAP_FIXED: > + break; > + default: /* broken */ > + if (safe_cache != SPAPR_CAP_BROKEN) { I think you just assert() for this. The only way these could get a different value is if there's a bug elsewhere. > + error_report("Invalid value for cap-cfpc (%d), assuming broken", > + safe_cache); > + } > + behaviour |= H_CPU_BEHAV_L1D_FLUSH_PR; > + break; > + } > + > + switch (safe_bounds_check) { > + case SPAPR_CAP_WORKAROUND: > + characteristics |= H_CPU_CHAR_SPEC_BAR_ORI31; > + behaviour |= H_CPU_BEHAV_BNDS_CHK_SPEC_BAR; > + break; > + case SPAPR_CAP_FIXED: > + break; > + default: /* broken */ > + if (safe_bounds_check != SPAPR_CAP_BROKEN) { > + error_report("Invalid value for cap-sbbc (%d), assuming broken", > + safe_bounds_check); > + } > + behaviour |= H_CPU_BEHAV_BNDS_CHK_SPEC_BAR; > + break; > + } > + > + switch (safe_indirect_branch) { > + case SPAPR_CAP_FIXED: > + characteristics |= H_CPU_CHAR_BCCTRL_SERIALISED; > + default: /* broken */ > + if (safe_indirect_branch != SPAPR_CAP_BROKEN) { > + error_report("Invalid value for cap-ibs (%d), assuming broken", > + safe_indirect_branch); > + } > + break; > + } > + > + args[0] = characteristics; > + args[1] = behaviour; > + > + return H_SUCCESS; > +} > + > static spapr_hcall_fn papr_hypercall_table[(MAX_HCALL_OPCODE / 4) + 1]; > static spapr_hcall_fn kvmppc_hypercall_table[KVMPPC_HCALL_MAX - KVMPPC_HCALL_BASE + 1]; > > @@ -1733,6 +1796,9 @@ static void hypercall_register_types(void) > spapr_register_hypercall(H_INVALIDATE_PID, h_invalidate_pid); > spapr_register_hypercall(H_REGISTER_PROC_TBL, h_register_process_table); > > + /* hcall-get-cpu-characteristics */ > + spapr_register_hypercall(H_GET_CPU_CHARACTERISTICS, h_get_cpu_characteristics); > + > /* "debugger" hcalls (also used by SLOF). Note: We do -not- differenciate > * here between the "CI" and the "CACHE" variants, they will use whatever > * mapping attributes qemu is using. When using KVM, the kernel will > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 549d7a4134..62c077ac20 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -404,6 +404,7 @@ struct sPAPRMachineState { > #define H_GET_HCA_INFO 0x1B8 > #define H_GET_PERF_COUNT 0x1BC > #define H_MANAGE_TRACE 0x1C0 > +#define H_GET_CPU_CHARACTERISTICS 0x1C8 > #define H_FREE_LOGICAL_LAN_BUFFER 0x1D4 > #define H_QUERY_INT_STATE 0x1E4 > #define H_POLL_PENDING 0x1D8
On 18/01/18 16:20, David Gibson wrote: > On Mon, Jan 15, 2018 at 05:32:35PM +1100, Suraj Jitindar Singh wrote: >> The new H-Call H_GET_CPU_CHARACTERISTICS is used by the guest to query >> behaviours and available characteristics of the cpu. >> >> Implement the handler for this new H-Call which formulates its response >> based on the setting of the spapr_caps cap-cfpc, cap-sbbc and cap-ibs. >> >> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> >> --- >> hw/ppc/spapr_hcall.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> include/hw/ppc/spapr.h | 1 + >> 2 files changed, 67 insertions(+) >> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c >> index 51eba52e86..a693d3b852 100644 >> --- a/hw/ppc/spapr_hcall.c >> +++ b/hw/ppc/spapr_hcall.c >> @@ -1654,6 +1654,69 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, >> return H_SUCCESS; >> } >> >> +static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu, >> + sPAPRMachineState *spapr, >> + target_ulong opcode, >> + target_ulong *args) >> +{ >> + uint64_t characteristics = H_CPU_CHAR_HON_BRANCH_HINTS & >> + ~H_CPU_CHAR_THR_RECONF_TRIG; >> + uint64_t behaviour = H_CPU_BEHAV_FAVOUR_SECURITY; >> + uint8_t safe_cache = spapr_get_cap(spapr, SPAPR_CAP_CFPC); >> + uint8_t safe_bounds_check = spapr_get_cap(spapr, SPAPR_CAP_SBBC); >> + uint8_t safe_indirect_branch = spapr_get_cap(spapr, SPAPR_CAP_IBS); >> + >> + switch (safe_cache) { >> + case SPAPR_CAP_WORKAROUND: >> + characteristics |= H_CPU_CHAR_L1D_FLUSH_ORI30; >> + characteristics |= H_CPU_CHAR_L1D_FLUSH_TRIG2; >> + characteristics |= H_CPU_CHAR_L1D_THREAD_PRIV; >> + behaviour |= H_CPU_BEHAV_L1D_FLUSH_PR; >> + break; >> + case SPAPR_CAP_FIXED: >> + break; >> + default: /* broken */ >> + if (safe_cache != SPAPR_CAP_BROKEN) { > > I think you just assert() for this. The only way these could get a > different value is if there's a bug elsewhere. Why not return H_HARDWARE or other error?
On Thu, Jan 18, 2018 at 04:44:28PM +1100, Alexey Kardashevskiy wrote: > On 18/01/18 16:20, David Gibson wrote: > > On Mon, Jan 15, 2018 at 05:32:35PM +1100, Suraj Jitindar Singh wrote: > >> The new H-Call H_GET_CPU_CHARACTERISTICS is used by the guest to query > >> behaviours and available characteristics of the cpu. > >> > >> Implement the handler for this new H-Call which formulates its response > >> based on the setting of the spapr_caps cap-cfpc, cap-sbbc and cap-ibs. > >> > >> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> > >> --- > >> hw/ppc/spapr_hcall.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++ > >> include/hw/ppc/spapr.h | 1 + > >> 2 files changed, 67 insertions(+) > >> > >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > >> index 51eba52e86..a693d3b852 100644 > >> --- a/hw/ppc/spapr_hcall.c > >> +++ b/hw/ppc/spapr_hcall.c > >> @@ -1654,6 +1654,69 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, > >> return H_SUCCESS; > >> } > >> > >> +static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu, > >> + sPAPRMachineState *spapr, > >> + target_ulong opcode, > >> + target_ulong *args) > >> +{ > >> + uint64_t characteristics = H_CPU_CHAR_HON_BRANCH_HINTS & > >> + ~H_CPU_CHAR_THR_RECONF_TRIG; > >> + uint64_t behaviour = H_CPU_BEHAV_FAVOUR_SECURITY; > >> + uint8_t safe_cache = spapr_get_cap(spapr, SPAPR_CAP_CFPC); > >> + uint8_t safe_bounds_check = spapr_get_cap(spapr, SPAPR_CAP_SBBC); > >> + uint8_t safe_indirect_branch = spapr_get_cap(spapr, SPAPR_CAP_IBS); > >> + > >> + switch (safe_cache) { > >> + case SPAPR_CAP_WORKAROUND: > >> + characteristics |= H_CPU_CHAR_L1D_FLUSH_ORI30; > >> + characteristics |= H_CPU_CHAR_L1D_FLUSH_TRIG2; > >> + characteristics |= H_CPU_CHAR_L1D_THREAD_PRIV; > >> + behaviour |= H_CPU_BEHAV_L1D_FLUSH_PR; > >> + break; > >> + case SPAPR_CAP_FIXED: > >> + break; > >> + default: /* broken */ > >> + if (safe_cache != SPAPR_CAP_BROKEN) { > > > > I think you just assert() for this. The only way these could get a > > different value is if there's a bug elsewhere. > > > Why not return H_HARDWARE or other error? Because what's the guest supposed to do with it. This is an internal qemu problem, so it should be dealt with via an internal qemu mechanism.
On 18/01/18 16:53, David Gibson wrote: > On Thu, Jan 18, 2018 at 04:44:28PM +1100, Alexey Kardashevskiy wrote: >> On 18/01/18 16:20, David Gibson wrote: >>> On Mon, Jan 15, 2018 at 05:32:35PM +1100, Suraj Jitindar Singh wrote: >>>> The new H-Call H_GET_CPU_CHARACTERISTICS is used by the guest to query >>>> behaviours and available characteristics of the cpu. >>>> >>>> Implement the handler for this new H-Call which formulates its response >>>> based on the setting of the spapr_caps cap-cfpc, cap-sbbc and cap-ibs. >>>> >>>> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> >>>> --- >>>> hw/ppc/spapr_hcall.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> include/hw/ppc/spapr.h | 1 + >>>> 2 files changed, 67 insertions(+) >>>> >>>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c >>>> index 51eba52e86..a693d3b852 100644 >>>> --- a/hw/ppc/spapr_hcall.c >>>> +++ b/hw/ppc/spapr_hcall.c >>>> @@ -1654,6 +1654,69 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, >>>> return H_SUCCESS; >>>> } >>>> >>>> +static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu, >>>> + sPAPRMachineState *spapr, >>>> + target_ulong opcode, >>>> + target_ulong *args) >>>> +{ >>>> + uint64_t characteristics = H_CPU_CHAR_HON_BRANCH_HINTS & >>>> + ~H_CPU_CHAR_THR_RECONF_TRIG; >>>> + uint64_t behaviour = H_CPU_BEHAV_FAVOUR_SECURITY; >>>> + uint8_t safe_cache = spapr_get_cap(spapr, SPAPR_CAP_CFPC); >>>> + uint8_t safe_bounds_check = spapr_get_cap(spapr, SPAPR_CAP_SBBC); >>>> + uint8_t safe_indirect_branch = spapr_get_cap(spapr, SPAPR_CAP_IBS); >>>> + >>>> + switch (safe_cache) { >>>> + case SPAPR_CAP_WORKAROUND: >>>> + characteristics |= H_CPU_CHAR_L1D_FLUSH_ORI30; >>>> + characteristics |= H_CPU_CHAR_L1D_FLUSH_TRIG2; >>>> + characteristics |= H_CPU_CHAR_L1D_THREAD_PRIV; >>>> + behaviour |= H_CPU_BEHAV_L1D_FLUSH_PR; >>>> + break; >>>> + case SPAPR_CAP_FIXED: >>>> + break; >>>> + default: /* broken */ >>>> + if (safe_cache != SPAPR_CAP_BROKEN) { >>> >>> I think you just assert() for this. The only way these could get a >>> different value is if there's a bug elsewhere. >> >> >> Why not return H_HARDWARE or other error? > > Because what's the guest supposed to do with it. "oops" > This is an internal > qemu problem, so it should be dealt with via an internal qemu > mechanism. Do we have assert() enabled in production? If not, then assert == noop, error_report is just a noise.
On 01/18/2018 02:11 AM, Alexey Kardashevskiy wrote: >>>> I think you just assert() for this. The only way these could get a >>>> different value is if there's a bug elsewhere. >>> >>> >>> Why not return H_HARDWARE or other error? >> >> Because what's the guest supposed to do with it. > > "oops" > >> This is an internal >> qemu problem, so it should be dealt with via an internal qemu >> mechanism. > > Do we have assert() enabled in production? If not, then assert == noop, > error_report is just a noise. See commit 262a69f4. Yes, assert() is enabled in production, for security reasons (aka it's easier to do that than to audit that migration is still safe even with no-op asserts).
On Thu, Jan 18, 2018 at 07:11:41PM +1100, Alexey Kardashevskiy wrote: > On 18/01/18 16:53, David Gibson wrote: > > On Thu, Jan 18, 2018 at 04:44:28PM +1100, Alexey Kardashevskiy wrote: > >> On 18/01/18 16:20, David Gibson wrote: > >>> On Mon, Jan 15, 2018 at 05:32:35PM +1100, Suraj Jitindar Singh wrote: > >>>> The new H-Call H_GET_CPU_CHARACTERISTICS is used by the guest to query > >>>> behaviours and available characteristics of the cpu. > >>>> > >>>> Implement the handler for this new H-Call which formulates its response > >>>> based on the setting of the spapr_caps cap-cfpc, cap-sbbc and cap-ibs. > >>>> > >>>> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> > >>>> --- > >>>> hw/ppc/spapr_hcall.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++ > >>>> include/hw/ppc/spapr.h | 1 + > >>>> 2 files changed, 67 insertions(+) > >>>> > >>>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > >>>> index 51eba52e86..a693d3b852 100644 > >>>> --- a/hw/ppc/spapr_hcall.c > >>>> +++ b/hw/ppc/spapr_hcall.c > >>>> @@ -1654,6 +1654,69 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, > >>>> return H_SUCCESS; > >>>> } > >>>> > >>>> +static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu, > >>>> + sPAPRMachineState *spapr, > >>>> + target_ulong opcode, > >>>> + target_ulong *args) > >>>> +{ > >>>> + uint64_t characteristics = H_CPU_CHAR_HON_BRANCH_HINTS & > >>>> + ~H_CPU_CHAR_THR_RECONF_TRIG; > >>>> + uint64_t behaviour = H_CPU_BEHAV_FAVOUR_SECURITY; > >>>> + uint8_t safe_cache = spapr_get_cap(spapr, SPAPR_CAP_CFPC); > >>>> + uint8_t safe_bounds_check = spapr_get_cap(spapr, SPAPR_CAP_SBBC); > >>>> + uint8_t safe_indirect_branch = spapr_get_cap(spapr, SPAPR_CAP_IBS); > >>>> + > >>>> + switch (safe_cache) { > >>>> + case SPAPR_CAP_WORKAROUND: > >>>> + characteristics |= H_CPU_CHAR_L1D_FLUSH_ORI30; > >>>> + characteristics |= H_CPU_CHAR_L1D_FLUSH_TRIG2; > >>>> + characteristics |= H_CPU_CHAR_L1D_THREAD_PRIV; > >>>> + behaviour |= H_CPU_BEHAV_L1D_FLUSH_PR; > >>>> + break; > >>>> + case SPAPR_CAP_FIXED: > >>>> + break; > >>>> + default: /* broken */ > >>>> + if (safe_cache != SPAPR_CAP_BROKEN) { > >>> > >>> I think you just assert() for this. The only way these could get a > >>> different value is if there's a bug elsewhere. > >> > >> > >> Why not return H_HARDWARE or other error? > > > > Because what's the guest supposed to do with it. > > "oops" Thereby making what is definitely a qemu bug appear to be a guest problem.
On Thu, Jan 18, 2018 at 02:30:27PM -0600, Eric Blake wrote: > On 01/18/2018 02:11 AM, Alexey Kardashevskiy wrote: > > >>>> I think you just assert() for this. The only way these could get a > >>>> different value is if there's a bug elsewhere. > >>> > >>> > >>> Why not return H_HARDWARE or other error? > >> > >> Because what's the guest supposed to do with it. > > > > "oops" > > > >> This is an internal > >> qemu problem, so it should be dealt with via an internal qemu > >> mechanism. > > > > Do we have assert() enabled in production? If not, then assert == noop, > > error_report is just a noise. > > See commit 262a69f4. Yes, assert() is enabled in production, for > security reasons (aka it's easier to do that than to audit that > migration is still safe even with no-op asserts). TBH, assert()s usually aren't disabled in production. That's the theory behind them, but in practice AFAICT, the debugging utility almost always outweighs the performance cost which is usually pretty small.
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c index 51eba52e86..a693d3b852 100644 --- a/hw/ppc/spapr_hcall.c +++ b/hw/ppc/spapr_hcall.c @@ -1654,6 +1654,69 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu, return H_SUCCESS; } +static target_ulong h_get_cpu_characteristics(PowerPCCPU *cpu, + sPAPRMachineState *spapr, + target_ulong opcode, + target_ulong *args) +{ + uint64_t characteristics = H_CPU_CHAR_HON_BRANCH_HINTS & + ~H_CPU_CHAR_THR_RECONF_TRIG; + uint64_t behaviour = H_CPU_BEHAV_FAVOUR_SECURITY; + uint8_t safe_cache = spapr_get_cap(spapr, SPAPR_CAP_CFPC); + uint8_t safe_bounds_check = spapr_get_cap(spapr, SPAPR_CAP_SBBC); + uint8_t safe_indirect_branch = spapr_get_cap(spapr, SPAPR_CAP_IBS); + + switch (safe_cache) { + case SPAPR_CAP_WORKAROUND: + characteristics |= H_CPU_CHAR_L1D_FLUSH_ORI30; + characteristics |= H_CPU_CHAR_L1D_FLUSH_TRIG2; + characteristics |= H_CPU_CHAR_L1D_THREAD_PRIV; + behaviour |= H_CPU_BEHAV_L1D_FLUSH_PR; + break; + case SPAPR_CAP_FIXED: + break; + default: /* broken */ + if (safe_cache != SPAPR_CAP_BROKEN) { + error_report("Invalid value for cap-cfpc (%d), assuming broken", + safe_cache); + } + behaviour |= H_CPU_BEHAV_L1D_FLUSH_PR; + break; + } + + switch (safe_bounds_check) { + case SPAPR_CAP_WORKAROUND: + characteristics |= H_CPU_CHAR_SPEC_BAR_ORI31; + behaviour |= H_CPU_BEHAV_BNDS_CHK_SPEC_BAR; + break; + case SPAPR_CAP_FIXED: + break; + default: /* broken */ + if (safe_bounds_check != SPAPR_CAP_BROKEN) { + error_report("Invalid value for cap-sbbc (%d), assuming broken", + safe_bounds_check); + } + behaviour |= H_CPU_BEHAV_BNDS_CHK_SPEC_BAR; + break; + } + + switch (safe_indirect_branch) { + case SPAPR_CAP_FIXED: + characteristics |= H_CPU_CHAR_BCCTRL_SERIALISED; + default: /* broken */ + if (safe_indirect_branch != SPAPR_CAP_BROKEN) { + error_report("Invalid value for cap-ibs (%d), assuming broken", + safe_indirect_branch); + } + break; + } + + args[0] = characteristics; + args[1] = behaviour; + + return H_SUCCESS; +} + static spapr_hcall_fn papr_hypercall_table[(MAX_HCALL_OPCODE / 4) + 1]; static spapr_hcall_fn kvmppc_hypercall_table[KVMPPC_HCALL_MAX - KVMPPC_HCALL_BASE + 1]; @@ -1733,6 +1796,9 @@ static void hypercall_register_types(void) spapr_register_hypercall(H_INVALIDATE_PID, h_invalidate_pid); spapr_register_hypercall(H_REGISTER_PROC_TBL, h_register_process_table); + /* hcall-get-cpu-characteristics */ + spapr_register_hypercall(H_GET_CPU_CHARACTERISTICS, h_get_cpu_characteristics); + /* "debugger" hcalls (also used by SLOF). Note: We do -not- differenciate * here between the "CI" and the "CACHE" variants, they will use whatever * mapping attributes qemu is using. When using KVM, the kernel will diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index 549d7a4134..62c077ac20 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -404,6 +404,7 @@ struct sPAPRMachineState { #define H_GET_HCA_INFO 0x1B8 #define H_GET_PERF_COUNT 0x1BC #define H_MANAGE_TRACE 0x1C0 +#define H_GET_CPU_CHARACTERISTICS 0x1C8 #define H_FREE_LOGICAL_LAN_BUFFER 0x1D4 #define H_QUERY_INT_STATE 0x1E4 #define H_POLL_PENDING 0x1D8
The new H-Call H_GET_CPU_CHARACTERISTICS is used by the guest to query behaviours and available characteristics of the cpu. Implement the handler for this new H-Call which formulates its response based on the setting of the spapr_caps cap-cfpc, cap-sbbc and cap-ibs. Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> --- hw/ppc/spapr_hcall.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++ include/hw/ppc/spapr.h | 1 + 2 files changed, 67 insertions(+)