Re: [Qemu-devel] [PATCH 1/4] hw/core: Add iommu to machine properties

[Peter Crosthwaite]

On Fri, Oct 9, 2015 at 5:17 AM, David kiarie <address@hidden> wrote:
> On Thu, Oct 8, 2015 at 9:10 PM, Marcel Apfelbaum
> <address@hidden> wrote:
>> On 10/09/2015 05:53 AM, David Kiarie wrote:
>>>
>>> From: David <address@hidden>
>>>
>>>   Add iommu to machine properties in preparation of introducing
>>>   AMD IOMMU
>>>
>>> Signed-off-by: David Kiarie <address@hidden>
>>> ---
>>>   hw/core/machine.c   | 25 +++++++++++++++++++++++++
>>>   include/hw/boards.h |  2 ++
>>>   2 files changed, 27 insertions(+)
>>>
>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>> index 51ed6b2..8cc7461 100644
>>> --- a/hw/core/machine.c
>>> +++ b/hw/core/machine.c
>>> @@ -269,6 +269,20 @@ static void machine_set_iommu(Object *obj, bool
>>> value, Error **errp)
>>>       ms->iommu = value;
>>>   }
>>>
>>> +static bool machine_get_amd_iommu(Object *obj, Error **errp)
>>> +{
>>> +    MachineState *ms = MACHINE(obj);
>>> +
>>> +    return ms->amd_iommu;
>>> +}
>>> +
>>> +static void machine_set_amd_iommu(Object *obj, bool value, Error **errp)
>>> +{
>>> +    MachineState *ms = MACHINE(obj);
>>> +
>>> +    ms->amd_iommu = value;
>>> +}
>>> +
>>>   static void machine_set_suppress_vmdesc(Object *obj, bool value, Error
>>> **errp)
>>>   {
>>>       MachineState *ms = MACHINE(obj);
>>> @@ -420,6 +434,12 @@ static void machine_initfn(Object *obj)
>>>       object_property_set_description(obj, "iommu",
>>>                                       "Set on/off to enable/disable Intel
>>> IOMMU (VT-d)",
>>>                                       NULL);
>>> +    object_property_add_bool(obj, "amd-iommu",
>>> +                             machine_get_amd_iommu,
>>> +                             machine_set_amd_iommu, NULL);
>>> +    object_property_set_description(obj, "amd-iommu",
>>> +                                    "Set on/off to enable/disable
>>> AMD-Vi",
>>> +                                    NULL);
>>>       object_property_add_bool(obj, "suppress-vmdesc",
>>>                                machine_get_suppress_vmdesc,
>>>                                machine_set_suppress_vmdesc, NULL);
>>> @@ -456,6 +476,11 @@ bool machine_iommu(MachineState *machine)
>>>       return machine->iommu;
>>>   }
>>>
>>> +bool machine_amd_iommu(MachineState *machine)
>>> +{
>>> +    return machine->amd_iommu;
>>> +}
>>> +
>>>   bool machine_kernel_irqchip_allowed(MachineState *machine)
>>>   {
>>>       return machine->kernel_irqchip_allowed;
>>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>>> index 566a5ca..c8424f7 100644
>>> --- a/include/hw/boards.h
>>> +++ b/include/hw/boards.h
>>> @@ -54,6 +54,7 @@ extern MachineState *current_machine;
>>>
>>>   bool machine_usb(MachineState *machine);
>>>   bool machine_iommu(MachineState *machine);
>>> +bool machine_amd_iommu(MachineState *machine);
>>>   bool machine_kernel_irqchip_allowed(MachineState *machine);
>>>   bool machine_kernel_irqchip_required(MachineState *machine);
>>>   int machine_kvm_shadow_mem(MachineState *machine);
>>> @@ -140,6 +141,7 @@ struct MachineState {
>>>       bool igd_gfx_passthru;
>>>       char *firmware;
>>>       bool iommu;
>>> +    bool amd_iommu;
>>>       bool suppress_vmdesc;
>>>
>>>       ram_addr_t ram_size;
>>>
>>
>> Hi,
>>
>> I think we should add the property to PC_MACHINE class because it make sense
>> only
>> for PC and Q35 machines (right?).
>
> Right.
>
>> MACHINE is the base class for all the architectures.
>>
>> On the same note, "iommu" refers to intel_iommu and maybe we should
>> move it also to the PC_MACHINE.
>> I understand that it is not in the scope of your series, we can add a patch
>> later.
>>
>> Another possibility would be to keep the same "iommu" property, but change
>> it from boolean
>> to off|intel|amd|... . This will break backward compatibility, on the other
>> hand having
>> iommu referring to Intel only when we have  multiple iommu implementations
>> is ugly IMHO.
>>
>
> Okay. I think I could look into the last option.
>

Whether or not you can have an IOMMU at all can be architecture
specific. Having a top level switch doesn't consider the possibility
of heterogeneous systems that could in theory have multiple IOMMUs.
The base machine class should not have any singleton hardware-specific
definitions like this. I think it belongs in PC_MACHINE.

Regards,
Peter

>> Thanks,
>> Marcel
>>
>>
>