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

[David kiarie]

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.

> Thanks,
> Marcel
>
>