On 09/11/16 14:52, David Gibson wrote:
> On Wed, Nov 09, 2016 at 12:27:47PM +1100, Alexey Kardashevskiy wrote:
>> On 08/11/16 16:18, David Gibson wrote:
>>> On Fri, Nov 04, 2016 at 03:01:40PM +1100, Alexey Kardashevskiy wrote:
>>>> On 30/10/16 22:12, David Gibson wrote:
>>>>> Once a compatiblity mode is negotiated with the guest,
>>>>> h_client_architecture_support() uses run_on_cpu() to update each CPU to
>>>>> the new mode. We're going to want this logic somewhere else shortly,
>>>>> so make a helper function to do this global update.
>>>>>
>>>>> We put it in target-ppc/compat.c - it makes as much sense at the CPU level
>>>>> as it does at the machine level. We also move the cpu_synchronize_state()
>>>>> into ppc_set_compat(), since it doesn't really make any sense to call that
>>>>> without synchronizing state.
>>>>>
>>>>> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
>>>>> ---
>>>>> hw/ppc/spapr_hcall.c | 31 +++++--------------------------
>>>>> target-ppc/compat.c | 36 ++++++++++++++++++++++++++++++++++++
>>>>> target-ppc/cpu.h | 3 +++
>>>>> 3 files changed, 44 insertions(+), 26 deletions(-)
>>>>>
>>>>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>>>>> index 3bd6d06..4eaf9a6 100644
>>>>> --- a/hw/ppc/spapr_hcall.c
>>>>> +++ b/hw/ppc/spapr_hcall.c
>>>>> @@ -881,20 +881,6 @@ static target_ulong h_set_mode(PowerPCCPU *cpu,
>>>>> sPAPRMachineState *spapr,
>>>>> return ret;
>>>>> }
>>>>>
>>>>> -typedef struct {
>>>>> - uint32_t compat_pvr;
>>>>> - Error *err;
>>>>> -} SetCompatState;
>>>>> -
>>>>> -static void do_set_compat(CPUState *cs, void *arg)
>>>>> -{
>>>>> - PowerPCCPU *cpu = POWERPC_CPU(cs);
>>>>> - SetCompatState *s = arg;
>>>>> -
>>>>> - cpu_synchronize_state(cs);
>>>>> - ppc_set_compat(cpu, s->compat_pvr, &s->err);
>>>>> -}
>>>>> -
>>>>> static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>>>>> sPAPRMachineState
>>>>> *spapr,
>>>>> target_ulong opcode,
>>>>> @@ -902,7 +888,6 @@ static target_ulong
>>>>> h_client_architecture_support(PowerPCCPU *cpu,
>>>>> {
>>>>> target_ulong list = ppc64_phys_to_real(args[0]);
>>>>> target_ulong ov_table;
>>>>> - CPUState *cs;
>>>>> bool explicit_match = false; /* Matched the CPU's real PVR */
>>>>> uint32_t max_compat = cpu->max_compat;
>>>>> uint32_t best_compat = 0;
>>>>> @@ -949,18 +934,12 @@ static target_ulong
>>>>> h_client_architecture_support(PowerPCCPU *cpu,
>>>>>
>>>>> /* Update CPUs */
>>>>> if (cpu->compat_pvr != best_compat) {
>>>>> - CPU_FOREACH(cs) {
>>>>> - SetCompatState s = {
>>>>> - .compat_pvr = best_compat,
>>>>> - .err = NULL,
>>>>> - };
>>>>> + Error *local_err = NULL;
>>>>>
>>>>> - run_on_cpu(cs, do_set_compat, &s);
>>>>> -
>>>>> - if (s.err) {
>>>>> - error_report_err(s.err);
>>>>> - return H_HARDWARE;
>>>>> - }
>>>>> + ppc_set_compat_all(best_compat, &local_err);
>>>>> + if (local_err) {
>>>>> + error_report_err(local_err);
>>>>> + return H_HARDWARE;
>>>>> }
>>>>> }
>>>>>
>>>>> diff --git a/target-ppc/compat.c b/target-ppc/compat.c
>>>>> index 1059555..0b12b58 100644
>>>>> --- a/target-ppc/compat.c
>>>>> +++ b/target-ppc/compat.c
>>>>> @@ -124,6 +124,8 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t
>>>>> compat_pvr, Error **errp)
>>>>> pcr = compat->pcr;
>>>>> }
>>>>>
>>>>> + cpu_synchronize_state(CPU(cpu));
>>>>> +
>>>>> cpu->compat_pvr = compat_pvr;
>>>>> env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
>>>>>
>>>>> @@ -136,6 +138,40 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t
>>>>> compat_pvr, Error **errp)
>>>>> }
>>>>> }
>>>>>
>>>>> +#if !defined(CONFIG_USER_ONLY)
>>>>> +typedef struct {
>>>>> + uint32_t compat_pvr;
>>>>> + Error *err;
>>>>> +} SetCompatState;
>>>>> +
>>>>> +static void do_set_compat(CPUState *cs, void *arg)
>>>>> +{
>>>>> + PowerPCCPU *cpu = POWERPC_CPU(cs);
>>>>> + SetCompatState *s = arg;
>>>>> +
>>>>> + ppc_set_compat(cpu, s->compat_pvr, &s->err);
>>>>> +}
>>>>> +
>>>>> +void ppc_set_compat_all(uint32_t compat_pvr, Error **errp)
>>>>> +{
>>>>> + CPUState *cs;
>>>>> +
>>>>> + CPU_FOREACH(cs) {
>>>>> + SetCompatState s = {
>>>>> + .compat_pvr = compat_pvr,
>>>>> + .err = NULL,
>>>>> + };
>>>>> +
>>>>> + run_on_cpu(cs, do_set_compat, &s);
>>>>> +
>>>>> + if (s.err) {
>>>>> + error_propagate(errp, s.err);
>>>>> + return;
>>>>> + }
>>>>> + }
>>>>> +}
>>>>> +#endif
>>>>> +
>>>>> int ppc_compat_max_threads(PowerPCCPU *cpu)
>>>>> {
>>>>> const CompatInfo *compat = compat_by_pvr(cpu->compat_pvr);
>>>>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>>>>> index 91e8be8..201a655 100644
>>>>> --- a/target-ppc/cpu.h
>>>>> +++ b/target-ppc/cpu.h
>>>>> @@ -1317,6 +1317,9 @@ static inline int cpu_mmu_index (CPUPPCState *env,
>>>>> bool ifetch)
>>>>> bool ppc_check_compat(PowerPCCPU *cpu, uint32_t compat_pvr,
>>>>> uint32_t min_compat_pvr, uint32_t max_compat_pvr);
>>>>> void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp);
>>>>> +#if !defined(CONFIG_USER_ONLY)
>>>>> +void ppc_set_compat_all(uint32_t compat_pvr, Error **errp);
>>>>> +#endif
>>>>
>>>> I would put all ppc*compat*() under #if defined(CONFIG_USER_ONLY) &&
>>>> defined(TARGET_PPC64) (or even moved this to target-ppc/Makefile.objs).
>>>
>>> I was originally going to do that, but decided against it.
>>>
>>>> Otherwise, functions like ppc_check_compat() have #if
>>>> !defined(CONFIG_USER_ONLY) which suggests that the rest of
>>>> ppc_check_compat() can actually be executed in ppc64-linux-user (while it
>>>> cannot, can it?).
>>>
>>> It won't be, but there's no theoretical reason they couldn't be. User
>>> mode, like spapr, doesn't execute hypervisor privilege code, and so
>>> the PCR isn't owned by the "guest" (if you can call the user mode
>>> executable that). Which means it could make sense to set it from the
>>> outside, although that's not something we currently do.
>>
>> Compatibility modes are designed to disable sets of instructions to keep
>> working old userspace software which relies on some opcodes to be invalid.
>>
>> linux-user is TCG, right? The user can pick any CPU he likes if there is
>> need to run such an old software, why on earth would anyone bother with
>> this compat mode in linux-user?
>
> True, I can't really see any reason to do that.
>
> On the other hand, compat mode does at least make theoretical sense,
> whereas, for example, compat mode on powernv is fundamentally
> nonsense. At this point I'm not terribly include to take away the
> (token) user-only support unless there's a compelling reason *not* to
> include it.What would make a compelling reason? :) This will make makefile simpler and will reduce number of #ifdef, and in fact it is not supported now anyway, it has not even been tried. -- Alexey
signature.asc
Description: OpenPGP digital signature
