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

[Marcel Apfelbaum]

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
>