[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC] Allow AMD IOMMU to have both SysBusDevice and PCI
|
From: |
Eduardo Habkost |
|
Subject: |
Re: [Qemu-devel] [RFC] Allow AMD IOMMU to have both SysBusDevice and PCIDevice properties. |
|
Date: |
Tue, 7 Jun 2016 16:12:39 -0300 |
|
User-agent: |
Mutt/1.6.1 (2016-04-27) |
Hi,
I didn't review the amd_iommu.c code, but there seems to be some
unrelated changes in the patch:
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