[Qemu-devel] [QEMU-PPC] [PATCH V3 6/6] target/ppc/spapr: Add H-Call H_GET_CPU_CHARACTERISTICS

Suraj Jitindar Singh posted 6 patches 8 years ago
There is a newer version of this series
[Qemu-devel] [QEMU-PPC] [PATCH V3 6/6] target/ppc/spapr: Add H-Call H_GET_CPU_CHARACTERISTICS
Posted by Suraj Jitindar Singh 8 years ago
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) {
+            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
-- 
2.13.6


Re: [Qemu-devel] [QEMU-PPC] [PATCH V3 6/6] target/ppc/spapr: Add H-Call H_GET_CPU_CHARACTERISTICS
Posted by David Gibson 8 years ago
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

-- 
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
Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [PATCH V3 6/6] target/ppc/spapr: Add H-Call H_GET_CPU_CHARACTERISTICS
Posted by Alexey Kardashevskiy 8 years ago
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?



-- 
Alexey

Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [PATCH V3 6/6] target/ppc/spapr: Add H-Call H_GET_CPU_CHARACTERISTICS
Posted by David Gibson 8 years ago
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.

-- 
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
Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [PATCH V3 6/6] target/ppc/spapr: Add H-Call H_GET_CPU_CHARACTERISTICS
Posted by Alexey Kardashevskiy 8 years ago
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.



-- 
Alexey

Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [PATCH V3 6/6] target/ppc/spapr: Add H-Call H_GET_CPU_CHARACTERISTICS
Posted by Eric Blake 8 years ago
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).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [PATCH V3 6/6] target/ppc/spapr: Add H-Call H_GET_CPU_CHARACTERISTICS
Posted by David Gibson 8 years ago
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.

-- 
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
Re: [Qemu-devel] [Qemu-ppc] [QEMU-PPC] [PATCH V3 6/6] target/ppc/spapr: Add H-Call H_GET_CPU_CHARACTERISTICS
Posted by David Gibson 8 years ago
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.

-- 
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