Re: [Qemu-devel] [PULL 07/15] q35: Check propery to determine if iommu is set

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

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);
>       }
>   }
>