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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 05/17/2014 11:45 AM, Alexey Kardashevskiy wrote:
> On 05/17/2014 06:46 AM, Alexander Graf wrote:
>>
>> 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? :)
> 
> 
> 3 (three) properties are "much" now? :)
> 
>>>> 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.
> 
> Practically speaking, people will want to use kernels which can work on
> POWER8 in raw mode, no? Here we are trying to fix older guests.
> 
>> 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.

I just realized where misunderstanding happened - when I switch from 4
threads per core to 2 threads per core, the number of CPU nodes does not
change, only server#s and gserver#s properties do and this is the only way
of telling the guest the new number of threads.

So if you still want to disable some nodes, please explain more. Thanks!


>> 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.
> 
> We do not want to show more threads (i.e. bigger ibm,ppc-interrupt-server#s
> properties) if we do not support more threads as we do not really know what
> guest kernel (including AIX) or some ppc64_cpu/drmgr/whatever tool will do
> if they find more.
> 
>> What does pHyp do here?
> 
> I wish it was easy to verify :)
> 
> I fail to see what exactly your objections are all about, I definitely miss
> something big here. Is it a minor code duplication or some dangerous bug?
> If it is just a code duplication, I could do something about it.
> 
> 


-- 
Alexey