Re: [Qemu-devel] [RFC] Allow AMD IOMMU to have both SysBusDevice and PCIDevice properties.

[David Kiarie]

On Tue, Jun 7, 2016 at 10:12 PM, Eduardo Habkost <address@hidden> wrote:
> Hi,

Hello,

>
> I didn't review the amd_iommu.c code, but there seems to be some
> unrelated changes in the patch:

Thanks for looking at this but I actually wanted someone to look at
the amd_iommu.c. I mentioned in annotation that there are some
unrelated changes because this work is based on code that has not been
merged yet. I specifically sent this to have a review in amd_iommu.c
not the details but the design. I have patchset that implements AMD
IOMMU (translation only) which is implemented as a PCI device. It is
however not possible to work on interrupt remapping without converting
AMD IOMMU from a PCI device to a SysBusDevice. This device(AMD IOMMU),
the one on this patch unlike in previous patches, creates to devices ;
a PCI device and a SySBusDev which am not sure is acceptable.

>
> On Sun, Jun 05, 2016 at 07:54:33PM +0300, David Kiarie wrote:
>> Signed-off-by: David Kiarie <address@hidden>
>> ---
>>  hw/acpi/aml-build.c         |    2 +-
>>  hw/i386/amd_iommu.c         | 1471 
>> +++++++++++++++++++++++++++++++++++++++++++
>>  hw/i386/amd_iommu.h         |  348 ++++++++++
>>  hw/i386/kvm/pci-assign.c    |    2 +-
>>  hw/i386/pc_q35.c            |    1 +
>>  include/hw/acpi/acpi-defs.h |   13 +
>>  include/hw/acpi/aml-build.h |    1 +
>>  include/hw/pci/pci.h        |   10 +-
>>  qemu-options.hx             |    7 +-
>>  util/qemu-config.c          |    8 +-
>>  10 files changed, 1853 insertions(+), 10 deletions(-)
>>  create mode 100644 hw/i386/amd_iommu.c
>>  create mode 100644 hw/i386/amd_iommu.h
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index cedb74e..8d4bd01 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -227,7 +227,7 @@ static void build_extop_package(GArray *package, uint8_t 
>> op)
>>      build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
>>  }
>>
>> -static void build_append_int_noprefix(GArray *table, uint64_t value, int 
>> size)
>> +void build_append_int_noprefix(GArray *table, uint64_t value, int size)
>
> Why this change?
>
>>  {
>>      int i;
>>
> [...]
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 04aae89..431eaed 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -281,6 +281,7 @@ static void pc_q35_machine_options(MachineClass *m)
>>      m->default_machine_opts = "firmware=bios-256k.bin";
>>      m->default_display = "std";
>>      m->no_floppy = 1;
>> +    m->has_dynamic_sysbus = true;
>
> Why is this needed? Is it possible to do this change before
> adding the iommu code?  Can this be done in a separate patch that
> documents why it should be changed and why it is safe to set it
> to true?
>
>>  }
>>
> [...]
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index a30808b..ef0e8a6 100644
>> --- a/include/hw/pci/pci.h
>> +++ b/include/hw/pci/pci.h
>> @@ -11,10 +11,11 @@
>>  #include "hw/pci/pcie.h"
>>
>>  /* PCI bus */
>> -
>> +#define PCI_DEVID(bus, devfn)   ((((uint16_t)(bus)) << 8) | (devfn))
>>  #define PCI_DEVFN(slot, func)   ((((slot) & 0x1f) << 3) | ((func) & 0x07))
>>  #define PCI_SLOT(devfn)         (((devfn) >> 3) & 0x1f)
>>  #define PCI_FUNC(devfn)         ((devfn) & 0x07)
>> +#define PCI_BUILD_BDF(bus, devfn)     ((bus << 8) | (devfn))
>
> Missing parenthesis around (bus).
>
>>  #define PCI_SLOT_MAX            32
>>  #define PCI_FUNC_MAX            8
>>
>> @@ -328,7 +329,6 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
>>  int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
>>                         uint8_t offset, uint8_t size,
>>                         Error **errp);
>> -
>
> Unrelated whitespace change.
>
>>  void pci_del_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t 
>> cap_size);
>>
>>  uint8_t pci_find_capability(PCIDevice *pci_dev, uint8_t cap_id);
>> @@ -692,11 +692,13 @@ static inline uint32_t pci_config_size(const PCIDevice 
>> *d)
>>      return pci_is_express(d) ? PCIE_CONFIG_SPACE_SIZE : 
>> PCI_CONFIG_SPACE_SIZE;
>>  }
>>
>> -static inline uint16_t pci_requester_id(PCIDevice *dev)
>> +static inline uint16_t pci_get_bdf(PCIDevice *dev)
>>  {
>> -    return (pci_bus_num(dev->bus) << 8) | dev->devfn;
>> +    return PCI_BUILD_BDF(pci_bus_num(dev->bus), dev->devfn);
>>  }
>>
>> +uint16_t pci_requester_id(PCIDevice *dev);
>> +
>
> Why is pci_requester_id() being kept? Where's its implementation?
>
> pci_requester_id() is still being used at:
>
>   hw/pci/msi.c:    attrs.requester_id = pci_requester_id(dev);
>   hw/pci/pcie_aer.c:    err.source_id = pci_requester_id(dev);
>
>
>>  /* DMA access functions */
>>  static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
>>  {
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 9f33361..0aec287 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -38,7 +38,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
>>      "                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"
>> -    "                iommu=on|off controls emulated Intel IOMMU (VT-d) 
>> support (default=off)\n"
>> +    "                iommu=on|off controls emulated IOMMU support(default: 
>> off)\n"
>> +    "                x-iommu-type=amd|intel overrides emulated IOMMU to AMD 
>> IOMMU (default: intel)\n"
>
> Where is the new x-iommu-type option being used?
>
>>      "                igd-passthru=on|off controls IGD GFX passthrough 
>> support (default=off)\n"
>>      "                aes-key-wrap=on|off controls support for AES key 
>> wrapping (default=on)\n"
>>      "                dea-key-wrap=on|off controls support for DEA key 
>> wrapping (default=on)\n"
>> @@ -74,7 +75,9 @@ Enables or disables memory merge support. This feature, 
>> when supported by
>>  the host, de-duplicates identical memory pages among VMs instances
>>  (enabled by default).
>>  @item iommu=on|off
>> -Enables or disables emulated Intel IOMMU (VT-d) support. The default is off.
>> +Enables and disables IOMMU emulation. The default is off.
>> address@hidden x-iommu-type=on|off
>> +Overrides emulated IOMMU from AMD IOMMU. By default Intel IOMMU is emulated.
>>  @item aes-key-wrap=on|off
>>  Enables or disables AES key wrapping support on s390-ccw hosts. This feature
>>  controls whether AES wrapping keys will be created to allow
>> diff --git a/util/qemu-config.c b/util/qemu-config.c
>> index fb97307..8886abf 100644
>> --- a/util/qemu-config.c
>> +++ b/util/qemu-config.c
>> @@ -213,8 +213,12 @@ static QemuOptsList machine_opts = {
>>              .help = "firmware image",
>>          },{
>>              .name = "iommu",
>> -            .type = QEMU_OPT_BOOL,
>> -            .help = "Set on/off to enable/disable Intel IOMMU (VT-d)",
>> +            .type =  QEMU_OPT_BOOL,
>> +            .help = "Set on/off to enable iommu",
>> +        },{
>> +            .name = "x-iommu-type",
>> +            .type =  QEMU_OPT_STRING,
>> +            .help = "Overrides emulated IOMMU from Intel to AMD",
>>          },{
>>              .name = "suppress-vmdesc",
>>              .type = QEMU_OPT_BOOL,
>> --
>> 2.1.4
>>
>
> --
> Eduardo