[1/8] machine: query iommu machine property rather than qemu opts

Fixes a QEMU crash when passing iommu parameter in command line.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/core/machine.c   | 5 +++++
 hw/pci-host/q35.c   | 2 +-
 include/hw/boards.h | 1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

Marcel Apfelbaum <marcel@redhat.com> writes:

> Fixes a QEMU crash when passing iommu parameter in command line.
>
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
>  hw/core/machine.c   | 5 +++++
>  hw/pci-host/q35.c   | 2 +-
>  include/hw/boards.h | 1 +
>  3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index fbd91be..096eb10 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -403,6 +403,11 @@ bool machine_usb(MachineState *machine)
>      return machine->usb;
>  }
>  
> +bool machine_iommu(MachineState *machine)
> +{
> +    return machine->iommu;
> +}
> +
>  static const TypeInfo machine_info = {
>      .name = TYPE_MACHINE,
>      .parent = TYPE_OBJECT,

What does this buy us over a straight current_machine->iommu?

Same for the other wrapper functions.

[...]

On 02/04/2015 06:47 PM, Markus Armbruster wrote:
> Marcel Apfelbaum <marcel@redhat.com> writes:
>
>> Fixes a QEMU crash when passing iommu parameter in command line.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>>   hw/core/machine.c   | 5 +++++
>>   hw/pci-host/q35.c   | 2 +-
>>   include/hw/boards.h | 1 +
>>   3 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index fbd91be..096eb10 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -403,6 +403,11 @@ bool machine_usb(MachineState *machine)
>>       return machine->usb;
>>   }
>>
>> +bool machine_iommu(MachineState *machine)
>> +{
>> +    return machine->iommu;
>> +}
>> +
>>   static const TypeInfo machine_info = {
>>       .name = TYPE_MACHINE,
>>       .parent = TYPE_OBJECT,
>
> What does this buy us over a straight current_machine->iommu?
Following QOM guidelines/conventions all object fields are private
to machine files only. The *only* ways that they can be exposed are:
1. A wrapper
2. object_property_get...

I chose the wrapper because the other variant would be really
ugly and should be used only on a generic code.

This is the real reason I used this, but pure theoretical speaking
using  wrappers will give us the opportunity to change machine_iommu
implementation without the need to change the call sites.

To wrap it up, I just followed previous comments I received on QOM handling.

Thanks,
Marcel

>
> Same for the other wrapper functions.
>
> [...]
>

Marcel Apfelbaum <marcel@redhat.com> writes:

> On 02/04/2015 06:47 PM, Markus Armbruster wrote:
>> Marcel Apfelbaum <marcel@redhat.com> writes:
>>
>>> Fixes a QEMU crash when passing iommu parameter in command line.
>>>
>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>> ---
>>>   hw/core/machine.c   | 5 +++++
>>>   hw/pci-host/q35.c   | 2 +-
>>>   include/hw/boards.h | 1 +
>>>   3 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>> index fbd91be..096eb10 100644
>>> --- a/hw/core/machine.c
>>> +++ b/hw/core/machine.c
>>> @@ -403,6 +403,11 @@ bool machine_usb(MachineState *machine)
>>>       return machine->usb;
>>>   }
>>>
>>> +bool machine_iommu(MachineState *machine)
>>> +{
>>> +    return machine->iommu;
>>> +}
>>> +
>>>   static const TypeInfo machine_info = {
>>>       .name = TYPE_MACHINE,
>>>       .parent = TYPE_OBJECT,
>>
>> What does this buy us over a straight current_machine->iommu?
> Following QOM guidelines/conventions all object fields are private
> to machine files only. The *only* ways that they can be exposed are:
> 1. A wrapper
> 2. object_property_get...
>
> I chose the wrapper because the other variant would be really
> ugly and should be used only on a generic code.
>
> This is the real reason I used this, but pure theoretical speaking
> using  wrappers will give us the opportunity to change machine_iommu
> implementation without the need to change the call sites.

So we pay the notational and cognitive overhead of wrappers now and
going forward to maybe some day save us fixing up a couple of
machine->iommu instances.

A theory that makes sense for interfacing complex machinery with
non-trivial implementation details becomes silly when applied to a
couple of straightforward configuration flags.  As Doug McIlroy
observed, "KISS became MICAHI: make it complicated and hide it."

> To wrap it up, I just followed previous comments I received on QOM handling.

In other words, it's somebody else's fault.  I'll shut up now ;)

[...]

On 02/04/2015 05:43 PM, Marcel Apfelbaum wrote:
> Fixes a QEMU crash when passing iommu parameter in command line.
>
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>

Please amend commit message:

Running
     x86_64-softmmu/qemu-system-x86_64 -machine pc,iommu=on -enable-kvm
leads to crash:
     qemu-system-x86_64: qemu/util/qemu-option.c:387: qemu_opt_get_bool_helper: Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed.
     Aborted (core dumped)

This happens because the commit e79d5a6 ("machine: remove qemu_machine_opts global list")
removed the global option descriptions and moved them to MachineState's QOM properties.

Fix this by querying machine properties through designated wrappers.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>


> ---
>   hw/core/machine.c   | 5 +++++
>   hw/pci-host/q35.c   | 2 +-
>   include/hw/boards.h | 1 +
>   3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index fbd91be..096eb10 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -403,6 +403,11 @@ bool machine_usb(MachineState *machine)
>       return machine->usb;
>   }
>
> +bool machine_iommu(MachineState *machine)
> +{
> +    return machine->iommu;
> +}
> +
>   static const TypeInfo machine_info = {
>       .name = TYPE_MACHINE,
>       .parent = TYPE_OBJECT,
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index b20bad8..dfd8bbf 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -415,7 +415,7 @@ static int mch_init(PCIDevice *d)
>                    PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
>       }
>       /* Intel IOMMU (VT-d) */
> -    if (qemu_opt_get_bool(qemu_get_machine_opts(), "iommu", false)) {
> +    if (machine_iommu(current_machine)) {
>           mch_init_dmar(mch);
>       }
>       return 0;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 3ddc449..a12f041 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -66,6 +66,7 @@ MachineClass *find_default_machine(void);
>   extern MachineState *current_machine;
>
>   bool machine_usb(MachineState *machine);
> +bool machine_iommu(MachineState *machine);
>
>   /**
>    * MachineClass:
>