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

[David Gibson]

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


signature.asc
Description: PGP signature

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

[David Gibson]

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


signature.asc
Description: PGP signature

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

[Eric Blake]

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



signature.asc
Description: OpenPGP digital signature

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

[Alexey Kardashevskiy]

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



signature.asc
Description: OpenPGP digital signature

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

[David Gibson]

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


signature.asc
Description: PGP signature

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

[Alexey Kardashevskiy]

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



signature.asc
Description: OpenPGP digital signature