Re: [Qemu-devel] [PATCH 8/9] spapr: Implement processor compatibility in ibm, client-architecture-support

[Alexander Graf <agraf@suse.de>]

On 16.05.14 17:57, Alexey Kardashevskiy wrote:
> On 05/17/2014 12:16 AM, Alexander Graf wrote:
>> On 15.05.14 13:28, Alexey Kardashevskiy wrote:
>>> Modern Linux kernels support last POWERPC CPUs so when a kernel boots,
>>> in most cases it can find a matching cpu_spec in the kernel's cpu_specs
>>> list. However if the kernel is quite old, it may be missing a definition
>>> of the actual CPU. To provide an ability for old kernels to work on modern
>>> hardware, a Processor Compatibility Mode has been introduced
>>> by the PowerISA specification.
>>>
>>>   From the hardware prospective, it is supported by the Processor
>>> Compatibility Register (PCR) which is defined in PowerISA. The register
>>> enables one of the compatibility modes (2.05/2.06/2.07).
>>> Since PCR is a hypervisor privileged register and cannot be
>>> accessed from the guest, the mode selection is done via
>>> ibm,client-architecture-support (CAS) RTAS call using which the guest
>>> specifies what "raw" and "architected" CPU versions it supports.
>>> QEMU works out the best match, changes a "cpu-version" property of
>>> every CPU and notifies the guest about the change by setting these
>>> properties in the buffer passed as a response on a custom H_CAS hypercall.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>>    hw/ppc/spapr.c       | 40 +++++++++++++++++++++++++
>>>    hw/ppc/spapr_hcall.c | 83
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>    trace-events         |  5 ++++
>>>    3 files changed, 128 insertions(+)
>>>
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index a2c9106..a0882a1 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -35,6 +35,7 @@
>>>    #include "kvm_ppc.h"
>>>    #include "mmu-hash64.h"
>>>    #include "cpu-models.h"
>>> +#include "qom/cpu.h"
>>>      #include "hw/boards.h"
>>>    #include "hw/ppc/ppc.h"
>>> @@ -592,11 +593,50 @@ int spapr_h_cas_compose_response(target_ulong addr,
>>> target_ulong size)
>>>    {
>>>        void *fdt;
>>>        sPAPRDeviceTreeUpdateHeader hdr = { .version_id = 1 };
>>> +    CPUState *cs;
>>> +    int smt = kvmppc_smt_threads();
>>>          size -= sizeof(hdr);
>>>          fdt = g_malloc0(size);
>>>        _FDT((fdt_create(fdt, size)));
>>> +    _FDT((fdt_begin_node(fdt, "cpus")));
>>> +
>>> +    CPU_FOREACH(cs) {
>>> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
>>> +        DeviceClass *dc = DEVICE_GET_CLASS(cpu);
>>> +        int smpt = spapr_get_compat_smp_threads(cpu);
>>> +        int i, index = ppc_get_vcpu_dt_id(cpu);
>>> +        uint32_t servers_prop[smpt];
>>> +        uint32_t gservers_prop[smpt * 2];
>>> +        char tmp[32];
>>> +
>>> +        if ((index % smt) != 0) {
>>> +            continue;
>>> +        }
>>> +
>>> +        snprintf(tmp, 32, "%s@%x", dc->fw_name, index);
>>> +        trace_spapr_cas_add_subnode(tmp);
>>> +
>>> +        _FDT((fdt_begin_node(fdt, tmp)));
>>> +        _FDT((fdt_property_cell(fdt, "cpu-version", cpu->cpu_version)));
>>> +
>>> +        /* Build interrupt servers and gservers properties */
>>> +        for (i = 0; i < smpt; i++) {
>>> +            servers_prop[i] = cpu_to_be32(index + i);
>>> +            /* Hack, direct the group queues back to cpu 0 */
>>> +            gservers_prop[i*2] = cpu_to_be32(index + i);
>>> +            gservers_prop[i*2 + 1] = 0;
>>> +        }
>>> +        _FDT((fdt_property(fdt, "ibm,ppc-interrupt-server#s",
>>> +                           servers_prop, sizeof(servers_prop))));
>>> +        _FDT((fdt_property(fdt, "ibm,ppc-interrupt-gserver#s",
>>> +                           gservers_prop, sizeof(gservers_prop))));
>>> +
>>> +        _FDT((fdt_end_node(fdt)));
>>> +    }
>>> +
>>> +    _FDT((fdt_end_node(fdt)));
>> Why is this so much code? Can we only replace full nodes?
> It is a diff tree, in only has properties to change.

So why do we change so much then? :)

>
>> Then please
>> extract the original CPU node creation into a function and just call it
>> from the original generation and from here.
>>
>> If we can also replace single properties I'd say we only need to override
>> cpu-version.
>
> Mmm. The user could run qemu with threads=8 on POWER8 with SLES11 which
> must not see 8 threads but it can update itself and reboot with the kernel
> which is aware of POWER8. You are saying we do not want to support that?

First off, I think that practically speaking people will want to use 
2-thread granularity on POWER8 because that gives them the best load 
exploitation on the system.

So if we just disable CPU nodes that would not be visible usually (there 
was a disabled flag in cpu nodes, right?) for threads that are 
incompatible with a new CAS mode, we can then remove the disabled flag 
when the guest accepts more threads again.

That means we could expose 8 threads (POWER8), guest only supports 2 
threads (POWER6), so we waste 6 threads. But the remaining 2 will be 
fast ;). We basically degrade the system to SMT2 mode.

What does pHyp do here?


Alex