On Tue, Jun 7, 2016 at 10:12 PM, Eduardo Habkost <ehabk...@redhat.com> 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 <davidkiar...@gmail.com>
>> ---
>> 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.
>> +@item 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
