Re: [Qemu-devel] [PATCH v3 4/5] intel-iommu: add Intel IOMMU emulation to q35 and add a machine option "iommu" as a switch

[Le Tan]

2014-08-14 19:12 GMT+08:00 Michael S. Tsirkin <address@hidden>:
> On Mon, Aug 11, 2014 at 03:05:01PM +0800, Le Tan wrote:
>> Add Intel IOMMU emulation to q35 chipset and expose it to the guest.
>> 1. Add a machine option. Users can use "-machine iommu=on|off" in the command
>> line to enable/disable Intel IOMMU. The default is off.
>> 2. Accroding to the machine option, q35 will initialize the Intel IOMMU and
>> use pci_setup_iommu() to setup q35_host_dma_iommu() as the IOMMU function for
>> the pci bus.
>> 3. q35_host_dma_iommu() will return different address space according to the
>> bus_num and devfn of the device.
>>
>> Signed-off-by: Le Tan <address@hidden>
>> ---
>>  hw/core/machine.c         | 27 +++++++++++++++++---
>>  hw/pci-host/q35.c         | 64 
>> +++++++++++++++++++++++++++++++++++++++++++----
>>  include/hw/boards.h       |  1 +
>>  include/hw/pci-host/q35.h |  2 ++
>>  qemu-options.hx           |  5 +++-
>>  vl.c                      |  4 +++
>>  6 files changed, 94 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 7a66c57..f0046d6 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -235,6 +235,20 @@ static void machine_set_firmware(Object *obj, const 
>> char *value, Error **errp)
>>      ms->firmware = g_strdup(value);
>>  }
>>
>> +static bool machine_get_iommu(Object *obj, Error **errp)
>> +{
>> +    MachineState *ms = MACHINE(obj);
>> +
>> +    return ms->iommu;
>> +}
>> +
>> +static void machine_set_iommu(Object *obj, bool value, Error **errp)
>> +{
>> +    MachineState *ms = MACHINE(obj);
>> +
>> +    ms->iommu = value;
>> +}
>> +
>>  static void machine_initfn(Object *obj)
>>  {
>>      object_property_add_str(obj, "accel",
>> @@ -270,10 +284,17 @@ static void machine_initfn(Object *obj)
>>                               machine_set_dump_guest_core,
>>                               NULL);
>>      object_property_add_bool(obj, "mem-merge",
>> -                             machine_get_mem_merge, machine_set_mem_merge, 
>> NULL);
>> -    object_property_add_bool(obj, "usb", machine_get_usb, machine_set_usb, 
>> NULL);
>> +                             machine_get_mem_merge,
>> +                             machine_set_mem_merge, NULL);
>> +    object_property_add_bool(obj, "usb",
>> +                             machine_get_usb,
>> +                             machine_set_usb, NULL);
>>      object_property_add_str(obj, "firmware",
>> -                            machine_get_firmware, machine_set_firmware, 
>> NULL);
>> +                            machine_get_firmware,
>> +                            machine_set_firmware, NULL);
>> +    object_property_add_bool(obj, "iommu",
>> +                             machine_get_iommu,
>> +                             machine_set_iommu, NULL);
>>  }
>>
>>  static void machine_finalize(Object *obj)
>> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
>> index a0a3068..3342711 100644
>> --- a/hw/pci-host/q35.c
>> +++ b/hw/pci-host/q35.c
>> @@ -347,6 +347,53 @@ static void mch_reset(DeviceState *qdev)
>>      mch_update(mch);
>>  }
>>
>> +static AddressSpace *q35_host_dma_iommu(PCIBus *bus, void *opaque, int 
>> devfn)
>> +{
>> +    IntelIOMMUState *s = opaque;
>> +    VTDAddressSpace **pvtd_as;
>> +    VTDAddressSpace *vtd_as;
>> +    int bus_num = pci_bus_num(bus);
>> +
>> +    assert(devfn >= 0);
>> +
>> +    pvtd_as = s->address_spaces[bus_num];
>> +    if (!pvtd_as) {
>> +        /* No corresponding free() */
>> +        pvtd_as = g_malloc0(sizeof(VTDAddressSpace *) *
>> +                            VTD_PCI_SLOT_MAX * VTD_PCI_FUNC_MAX);
>> +        s->address_spaces[bus_num] = pvtd_as;
>> +    }
>> +
>> +    vtd_as = *(pvtd_as + devfn);
>
> pvtd_as[devfn] is cleaner.
> In fact, you can then drop vtd_as local variable, use pvtd_as[devfn]
> directly.
>
>> +    if (!vtd_as) {
>> +        vtd_as = g_malloc0(sizeof(*vtd_as));
>> +        *(pvtd_as + devfn) = vtd_as;
>> +
>> +        vtd_as->bus_num = bus_num;
>> +        vtd_as->devfn = devfn;
>> +        vtd_as->iommu_state = s;
>> +        memory_region_init_iommu(&vtd_as->iommu, OBJECT(s), &s->iommu_ops,
>> +                                 "intel_iommu", UINT64_MAX);
>> +        address_space_init(&vtd_as->as, &vtd_as->iommu, "intel_iommu");
>> +    }
>> +
>> +    return &vtd_as->as;
>> +}
>> +
>> +static void mch_init_dmar(MCHPCIState *mch)
>> +{
>> +    PCIBus *pci_bus = PCI_BUS(qdev_get_parent_bus(DEVICE(mch)));
>> +
>> +    mch->iommu = INTEL_IOMMU_DEVICE(qdev_create(NULL, 
>> TYPE_INTEL_IOMMU_DEVICE));
>> +    object_property_add_child(OBJECT(mch), "intel-iommu",
>> +                              OBJECT(mch->iommu), NULL);
>> +    qdev_init_nofail(DEVICE(mch->iommu));
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(mch->iommu), 0, 
>> Q35_HOST_BRIDGE_IOMMU_ADDR);
>> +
>> +    pci_setup_iommu(pci_bus, q35_host_dma_iommu, mch->iommu);
>> +}
>> +
>> +
>>  static int mch_init(PCIDevice *d)
>>  {
>>      int i;
>> @@ -363,13 +410,20 @@ static int mch_init(PCIDevice *d)
>>      memory_region_add_subregion_overlap(mch->system_memory, 0xa0000,
>>                                          &mch->smram_region, 1);
>>      memory_region_set_enabled(&mch->smram_region, false);
>> -    init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory, 
>> mch->pci_address_space,
>> -             &mch->pam_regions[0], PAM_BIOS_BASE, PAM_BIOS_SIZE);
>> +    init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
>> +             mch->pci_address_space, &mch->pam_regions[0], PAM_BIOS_BASE,
>> +             PAM_BIOS_SIZE);
>>      for (i = 0; i < 12; ++i) {
>> -        init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory, 
>> mch->pci_address_space,
>> -                 &mch->pam_regions[i+1], PAM_EXPAN_BASE + i * 
>> PAM_EXPAN_SIZE,
>> -                 PAM_EXPAN_SIZE);
>> +        init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory,
>> +                 mch->pci_address_space, &mch->pam_regions[i+1],
>> +                 PAM_EXPAN_BASE + i * PAM_EXPAN_SIZE, PAM_EXPAN_SIZE);
>
> Random formatting changes above? Make it a separate patch please.

Yes, I was told that I could fix some coding style issues around
places that my patch touches. If that is improper, I will just first
leave them out. :)
Thanks for your reviews!

Le

>> +    }
>> +
>> +    /* Intel IOMMU (VT-d) */
>> +    if (qemu_opt_get_bool(qemu_get_machine_opts(), "iommu", false)) {
>> +        mch_init_dmar(mch);
>>      }
>> +
>>      return 0;
>>  }
>>
>> diff --git a/include/hw/boards.h b/include/hw/boards.h
>> index 605a970..dfb6718 100644
>> --- a/include/hw/boards.h
>> +++ b/include/hw/boards.h
>> @@ -123,6 +123,7 @@ struct MachineState {
>>      bool mem_merge;
>>      bool usb;
>>      char *firmware;
>> +    bool iommu;
>>
>>      ram_addr_t ram_size;
>>      ram_addr_t maxram_size;
>> diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h
>> index d9ee978..025d6e6 100644
>> --- a/include/hw/pci-host/q35.h
>> +++ b/include/hw/pci-host/q35.h
>> @@ -33,6 +33,7 @@
>>  #include "hw/acpi/acpi.h"
>>  #include "hw/acpi/ich9.h"
>>  #include "hw/pci-host/pam.h"
>> +#include "hw/i386/intel_iommu.h"
>>
>>  #define TYPE_Q35_HOST_DEVICE "q35-pcihost"
>>  #define Q35_HOST_DEVICE(obj) \
>> @@ -60,6 +61,7 @@ typedef struct MCHPCIState {
>>      uint64_t pci_hole64_size;
>>      PcGuestInfo *guest_info;
>>      uint32_t short_root_bus;
>> +    IntelIOMMUState *iommu;
>>  } MCHPCIState;
>>
>>  typedef struct Q35PCIHost {
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 96516c1..7406a17 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -35,7 +35,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>>      "                kernel_irqchip=on|off controls accelerated irqchip 
>> support\n"
>>      "                kvm_shadow_mem=size of KVM shadow MMU\n"
>>      "                dump-guest-core=on|off include guest memory in a core 
>> dump (default=on)\n"
>> -    "                mem-merge=on|off controls memory merge support 
>> (default: on)\n",
>> +    "                mem-merge=on|off controls memory merge support 
>> (default: on)\n"
>> +    "                iommu=on|off controls emulated Intel IOMMU (VT-d) 
>> support (default=off)\n",
>>      QEMU_ARCH_ALL)
>>  STEXI
>>  @item -machine address@hidden,address@hidden,...]]
>> @@ -58,6 +59,8 @@ Include guest memory in a core dump. The default is on.
>>  Enables or disables memory merge support. This feature, when supported by
>>  the host, de-duplicates identical memory pages among VMs instances
>>  (enabled by default).
>> address@hidden iommu=on|off
>> +Enables or disables emulated Intel IOMMU (VT-d) support. The default is off.
>>  @end table
>>  ETEXI
>>
>> diff --git a/vl.c b/vl.c
>> index a8029d5..2ab1643 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -388,6 +388,10 @@ static QemuOptsList qemu_machine_opts = {
>>              .name = PC_MACHINE_MAX_RAM_BELOW_4G,
>>              .type = QEMU_OPT_SIZE,
>>              .help = "maximum ram below the 4G boundary (32bit boundary)",
>> +        },{
>> +            .name = "iommu",
>> +            .type = QEMU_OPT_BOOL,
>> +            .help = "Set on/off to enable/disable Intel IOMMU (VT-d)",
>>          },
>>          { /* End of list */ }
>>      },
>> --
>> 1.9.1