Re: [Qemu-devel] [PATCH 18/19] target-ppc: Enhance the CPU node labels for the guest device tree for pseries.

[Andreas Färber]

Am 10.07.2013 08:38, schrieb Prerna Saxena:
> On 07/08/2013 10:15 PM, Andreas Färber wrote:
>> Am 08.07.2013 17:49, schrieb Prerna Saxena:
>>> On 07/08/2013 02:32 PM, Andreas Färber wrote:
>>>> Am 08.07.2013 03:09, schrieb David Gibson:
>>>>> On Sat, Jul 06, 2013 at 11:54:15PM +1000, Alexey Kardashevskiy
>>>>> wrote:
>>>>>> @@ -1342,6 +1346,13 @@ static void
>>>>>> ppc_spapr_init(QEMUMachineInitArgs *args) 
>>>>>> register_savevm_live(NULL, "spapr/htab", -1, 1, 
>>>>>> &savevm_htab_handlers, spapr);
>>>>>>
>>>>>> +    /* Ensure that cpu_model is correctly reflected for a KVM
>>>>>> guest */ +    if (kvm_enabled() && !strcmp(cpu_model, "host")) { 
>>>>>> +        asm ("mfpvr %0" +            : "=r"(pvr)); +
>>>>>> cpu_model = ppc_cpu_alias_by_pvr(pvr);
>>>>>
>>>>> This needs to be protected by an ifdef CONFIG_KVM or similar.  If
>>>>> the compiler optimization level is turned down, so that it doesn't 
>>>>> recognize that the kvm_enabled() is always false, then this could 
>>>>> attempt to compile the ppc asm instructions on an x86 (or
>>>>> whatever) host.
>>>>
>>>> This hunk can be completely replaced by QOM mechanisms - just didn't
>>>> get to replying yet...
>>>
>>> Sorry I already sent out a v2, and only then saw your message. Could you
>>> pls explain how I could use QOM to replace this code block ?
>>
>> Well, in short the thing is it has not much to do with KVM. The
>> KVM-specific host-powerpc64-cpu type is derived from the one you're
>> looking for and thus you can use object_class_get_parent() to obtain the
>> parent type and look at its name - stripping "-" TYPE_POWERPC_CPU from
>> it should be much more efficient but will give you the detailed name
>> including revision. I was planning to propose an alternative patch for that.
> 
> This is what my patch does :-)
> 
> +const char *ppc_cpu_alias_by_pvr(uint32_t pvr)
> +{
> +    int i;
> +    const char *cpu_alias;
> +    char *offset, *model;
> +
> +    cpu_alias  = object_class_get_name(OBJECT_CLASS
> +                            (ppc_cpu_class_by_pvr(pvr)));
> + ....[snip]

And I am complaining about code duplication: Your use of
ppc_cpu_class_by_pvr() should be replaced with object_class_get_parent()
as I said above, because the PVR lookup is already done in KVM code for
you. :-)

>> Replacing a concrete model name with its simpler alias is a secondary
>> issue (separate patch) that is not specific to KVM or -cpu host. Compare
>> -cpu POWER8_v1.0 printing .../address@hidden/... presumably.
>>
> 
> Agree that this is not specific to KVM. That is the reason I have set it
> in a separate function, which can be called otherwise as well.
> 
> Just to clarify your response, you want the function I coded to be split
> into 2 different pieces, to cater to the two specific requirements you
> mention ? That can be done, but not sure if it is too much code bloat.

Your function duplicates runtime functionality (while the model list
keeps growing...) and you are duplicating KVM code into sPAPR. I was
asking you to make better reuse of existing code and I asked you whether
we need the model-to-alias lookup at all. It should not be limited to
your KVMish cpu_model == host check but either be dropped or called
afterwards on any cpu_model for consistent results.

>> (Note that the cpu_model_str field may contain more than just the model
>> name, it is otherwise unused in softmmu and I was therefore preparing a
>> patch to ban its use to linux-user solely, so the type name seems the
>> most reliable indicator we have and as a bonus no PVR needed for it.)
>>
> 
> Hmm, maybe obsoleting PVR check is not such a great idea.
> I'm not sure if my earlier email clearly outlined the use-case this
> patch was attempting to fix. Here is a detailed explanation :
> 
> We will still need PVR based lookups for cases such as the one I have
> described. As an illustration, consider running in a KVM environment
> where QEMU hasnt been started with a specific CPU type via "-CPU
> PPC_MODEL". In this case, we will be required to do a PVR_based lookup
> only -- to make sure the guest gets initialized with the same CPU as
> host. The notion of _same_cpu_model_ can only be built over a PVR check.

Sorry? That is done in kvm.c (I wrote the current form of that code!)
and no one proposed changing it. What I am asking is not to introduce
yet another mfpvr in your *sPAPR* code.

There is no requirement to use -cpu host (the default) with KVM, you can
use -cpu some_model with KVM just as well. For instance, when your PVR
is not yet enabled in QEMU (e.g., try -cpu POWER7_v2.3 on POWER8 DD1 to
see what I mean).

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg