Re: [Qemu-devel] [PATCH 0/8] machine: query machine properties rather than qemu opts

[Marcel Apfelbaum <marcel.apfelbaum@gmail.com>]

On 02/04/2015 09:45 PM, Christian Borntraeger wrote:
> Am 04.02.2015 um 16:43 schrieb Marcel Apfelbaum:
>> Commit e79d5a6 ("machine: remove qemu_machine_opts global list") removed option
>> descriptions from the -machine QemuOptsList to avoid repeating MachineState's QOM properties.
>>
>> This results in a Qemu crash if a non string option is queried using qemu opts.
>> Fix this by querying machine properties through designated wrappers.
>>
>> I hope I didn't miss anything.
>> Comments are appreciated as always.
>>
>> Thanks,
>> Marcel
>>
>> Marcel Apfelbaum (8):
>>    machine: query iommu machine property rather than qemu opts
>>    hw/machine: kernel-irqchip property support for allowed/required
>>    machine: query kernel-irqchip machine property rather than qemu opts
>>    kvm: add machine state to kvm_arch_init
>>    machine: query kvm-shadow-mem machine property rather than qemu opts
>>    machine: query phandle-start machine property rather than qemu opts
>>    machine: query dump-guest-core machine property rather than qemu opts
>>    machine: query mem-merge machine property rather than qemu opts
>
> In general this seems to work.
Thanks!

> I have a question, though:
>
> What I like is a way to do some wrappers in the specific machines.
> For example, we plan to add
>
> static inline void s390_machine_initfn(Object *obj)
> {
>      object_property_add_bool(obj, "aes-key-wrap",
>                               machine_get_aes_key_wrap,
>                               machine_set_aes_key_wrap, NULL);
>      object_property_set_description(obj, "aes-key-wrap",
>              "enable/disable AES key wrapping using the CPACF wrapping key",
>              NULL);
>      object_property_add_bool(obj, "dea-key-wrap",
>                               machine_get_dea_key_wrap,
>                               machine_set_dea_key_wrap, NULL);
>      object_property_set_description(obj, "dea-key-wrap",
>              "enable/disable DEA key wrapping using the CPACF wrapping key",
>              NULL);
> }
OK, You add 2 QOM properties to TYPE_S390_CCW_MACHINE instances.
Of course your setters/getters need to save the property values into an actual field,
so you will need a S390State (derived from MachineState)

typedef struct {
   MachineState parent;

   bool   aes_key_wrap;
   ...

} S390State

>
>
>
> Previously we used
>   if (qemu_opt_get_bool(opts, "aes-key-wrap", false))
> if target-s390/kvm.c
> to query that.
>
> Now, these options are pretty specific to s390 and adding them to hw/core/machine.c
> to create wrappers seems strange.
Completely agree. This is the reason we wanted options(properties) per machine type.

  So implementing them in hw/s390/s390-virtio-ccw.c
> seems a much better place.
Indeed.

> Would a function there that does  gets S390_CCW_MACHINE(current_machine)->aes_key_wrap
> considered ok, or do we need to pollute hw/core/machine.c with architecture specific
> options?
We surely don't add this to hw/core/machine.c because is specific to TYPE_S390_CCW_MACHINE.

Let's say you want to use this property in kvm_arch_init of target-s390x/kvm.c.
  - After this series you already have an instance of MachineState initialized with your new properties.
  - My assumption is that TYPE_S390_CCW_MACHINE is the only s390 machine or the base type of all s390 machines.
You have three options here:
  1. Use QOM infrastucture:
     bool aes_key_wrap = object_property_get_bool(OBJECT(machine), "aes-key-wrap", NULL);
  2. Add a wrapper somewhere in  include/hw/s390x/
     that gets MachineState, cast it into S390State and return the field value.
  3. Directly downcast MachineState to S390State and get the value.
* All of the above use my assumption that all s390 machines derive from this one.

>
> Christian
>
> PS: The same is somewhat true for qemu-options.hx. Having such specific machine option
> in the global help offers room for improvement
Can you please elaborate? I didn't fully understand what exactly are you referring to.

Hope I helped,
Marcel
>
>