Re: [Qemu-devel] [PATCH v3 6/6] target-ppc: demonstrate new "vsx" property

[Alexey Kardashevskiy]

On 11/12/2013 12:31 AM, Andreas Färber wrote:
> Am 11.11.2013 08:44, schrieb Alexey Kardashevskiy:
>> This patch is to demonstrate a static property handling in PowerPC.
>> Running QEMU with -cpu host,-vsx disables VSX bit in
>> PowerPCCPU::env::flags.
>>
>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>> ---
>>  target-ppc/translate_init.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
>> index df0d81c..60ea235 100644
>> --- a/target-ppc/translate_init.c
>> +++ b/target-ppc/translate_init.c
>> @@ -29,6 +29,7 @@
>>  #include "mmu-hash64.h"
>>  #include "qemu/error-report.h"
>>  #include "qapi/visitor.h"
>> +#include "hw/qdev-properties.h"
>>  
>>  //#define PPC_DUMP_CPU
>>  //#define PPC_DEBUG_SPR
>> @@ -8740,6 +8741,12 @@ static void ppc_cpu_class_init(ObjectClass *oc, void 
>> *data)
>>  
>>      dc->fw_name = "PowerPC,UNKNOWN";
>>      cc->parse_options = cpu_default_parse_options_func;
>> +
>> +    static Property powerpc_properties[] = {
>> +        DEFINE_PROP_BIT("vsx", PowerPCCPU, env.flags, 
>> BITNR(POWERPC_FLAG_VSX), false),
>> +        DEFINE_PROP_END_OF_LIST(),
>> +    };
>> +    dc->props = powerpc_properties;
>>  }
>>  
>>  static const TypeInfo ppc_cpu_type_info = {
> 
> This type of static property looks good to me, but two problems apart
> from the BITNR() discussed elsewhere:
> 
> 1) Won't the default value of false always disable VSX for all models
> including POWER7 and higher?


I am not really planning on defining properties per CPU feature, this all
was just to demonstrate an ability to define those.

But even if I was planning, I would set default to true and move the whole
set of properties to POWER7 family and that would be it.

> 2) Please move the array out of the function to just before the
> containing function.

Again, this is just to show. But. Why do you want those properties to be
outside of the function? Less visibility is usually a good thing, no?

> Igor has been facing a similar issue in his refactoring of x86 models
> and I believe evaluated to define the properties per model so that they
> have different defaults. Another way is overwriting that default in the
> model's instance_init or via globals specific to the subtype.

Current x86 in QEMU does not have those CPU families as POWERPC does,
correct? So we have more flexibility with properties and everything...


-- 
Alexey