[PULL,07/15] q35: Check propery to determine if iommu is set

From: Bandan Das <bsd@redhat.com>

The helper function machine_iommu() isn't necesary. We can
directly check for the property.

Signed-off-by: Bandan Das <bsd@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Bandan Das <bsd@redhat.com>
---
 include/hw/boards.h | 1 -
 hw/core/machine.c   | 5 -----
 hw/pci-host/q35.c   | 2 +-
 3 files changed, 1 insertion(+), 7 deletions(-)

On 11/19/2015 03:36 PM, Michael S. Tsirkin wrote:
> From: Bandan Das <bsd@redhat.com>
>
> The helper function machine_iommu() isn't necesary. We can
> directly check for the property.
>
> Signed-off-by: Bandan Das <bsd@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>   include/hw/boards.h | 1 -
>   hw/core/machine.c   | 5 -----
>   hw/pci-host/q35.c   | 2 +-
>   3 files changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 3e9a92c..24eb6f0 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -33,7 +33,6 @@ MachineClass *find_default_machine(void);
>   extern MachineState *current_machine;
>
>   bool machine_usb(MachineState *machine);
> -bool machine_iommu(MachineState *machine);
>   bool machine_kernel_irqchip_allowed(MachineState *machine);
>   bool machine_kernel_irqchip_required(MachineState *machine);
>   int machine_kvm_shadow_mem(MachineState *machine);
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index f4db340..acca00d 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -462,11 +462,6 @@ bool machine_usb(MachineState *machine)
>       return machine->usb;
>   }
>
> -bool machine_iommu(MachineState *machine)
> -{
> -    return machine->iommu;
> -}
> -
>   bool machine_kernel_irqchip_allowed(MachineState *machine)
>   {
>       return machine->kernel_irqchip_allowed;
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index c81507d..1fb4707 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -506,7 +506,7 @@ static void mch_realize(PCIDevice *d, Error **errp)
>                    PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
>       }
>       /* Intel IOMMU (VT-d) */
> -    if (machine_iommu(current_machine)) {
> +    if (object_property_get_bool(qdev_get_machine(), "iommu", NULL)) {

Maybe is too late, but this contradicts QEMU usage, as I understand
object_property_get_* should be used when we don't know object's type.

Why use "iommu" when you can simply call current_machine->iommu ?
(if you don't like the wrapper, which is pretty harmless in my opinion)


Thanks,
Marcel

>           mch_init_dmar(mch);
>       }
>   }
>

Hi Marcel,

Marcel Apfelbaum <marcel.apfelbaum@gmail.com> writes:
...
>
> Maybe is too late, but this contradicts QEMU usage, as I understand
Why late ? We can always revert it :)

> object_property_get_* should be used when we don't know object's type.
My understanding is that it's not mandatory to use it only when type
is unknown. Ofcourse, it makes it redundant when you do know the type.

I tend to follow convention, I noticed another call to qdev_get_machine,
and so opted for this. I am actually ok either way and don't prefer one way
over the other.

> Why use "iommu" when you can simply call current_machine->iommu ?
> (if you don't like the wrapper, which is pretty harmless in my opinion)

>
> Thanks,
> Marcel
>
>>           mch_init_dmar(mch);
>>       }
>>   }
>>

On 11/29/2015 08:22 PM, Bandan Das wrote:
> Hi Marcel,
>
> Marcel Apfelbaum <marcel.apfelbaum@gmail.com> writes:
> ...
>>
>> Maybe is too late, but this contradicts QEMU usage, as I understand
> Why late ? We can always revert it :)

Well, this is why I am so disappointment in myself that I didn't catch
this earlier. I *really* don't like doing this. :(

On the bright side this is a 're-factoring only' patch, so we can
take our time with it.

>
>> object_property_get_* should be used when we don't know object's type.
> My understanding is that it's not mandatory to use it only when type
> is unknown. Ofcourse, it makes it redundant when you do know the type.
>
> I tend to follow convention, I noticed another call to qdev_get_machine,
> and so opted for this. I am actually ok either way and don't prefer one way
> over the other.

This part actually makes sense. qdev_get_machine should be preferred over
current_machine global variable that should disappear (IMHO).

But once we have the machine as Object, we can simply cast it to machine
and get the field with MACHINE(qdev_get_machine())->iommu instead of
calling the property 'by name'.

And the wrapper machine_iommu was a "commodity method" requested by (some)
QOM guys who don't like calling the "object internals" in other files.

However since "iommmu" is a simple flag, I suppose we gain nothing from the wrapper.


Thanks,
Marcel

>
>> Why use "iommu" when you can simply call current_machine->iommu ?
>> (if you don't like the wrapper, which is pretty harmless in my opinion)
>
>>
>> Thanks,
>> Marcel
>>
>>>            mch_init_dmar(mch);
>>>        }
>>>    }
>>>