On Fri, Oct 9, 2015 at 5:17 AM, David kiarie <davidkiar...@gmail.com> wrote: > On Thu, Oct 8, 2015 at 9:10 PM, Marcel Apfelbaum > <marcel.apfelb...@gmail.com> wrote: >> On 10/09/2015 05:53 AM, David Kiarie wrote: >>> >>> From: David <davidkiar...@gmail.com> >>> >>> Add iommu to machine properties in preparation of introducing >>> AMD IOMMU >>> >>> Signed-off-by: David Kiarie <davidkiar...@gmail.com> >>> --- >>> 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 >> >> >