[Qemu-devel] [RFC] AMD IOMMU: emulate multiple devices

[Qemu-devel] [RFC] AMD IOMMU: emulate multiple devices

[David Kiarie]

Hello all,

This patch tries to solve a problem whereby real AMD IOMMUs exhibit both PCI and Platform device properties. AMD IOMMU properties that conflict with conventional PCI devices' features include the fact that its not a BusMaster device and reserves MMIO region without a BAR register among others.

There is some already ongoing work on Intel IOMMU Interrupt remapping with implements an IOMMU base class[1], as a platform device(which means the moment I inherit from this class my device loses it's PCI properties). I am therefore forced to find a way to combine both PCI and platform features(which I had previously avoided) into AMD IOMMU.
                                                X86-IOMMU(common code)
                                                    |
                                                    |
                                                   / \
                                                  /   \
                                        Intel IOMMU   AMD IOMMU

This patch implements a stripped down sample of how I plan to solve this issue. It basically implements PCI device which serves to 'steal' PCI config space while the main device remains a platform device. The platform device maintains a reference to the PCI device and hence the relevant PCI config space. This device will also require [2] to work.

Looking forward to your comments!

[1] http://thread.gmane.org/gmane.comp.emulators.qemu/414510
[2] http://thread.gmane.org/gmane.comp.emulators.qemu/413018

David Kiarie (1):
  hw/i386: Composite Bus and PCI device

 hw/i386/compositedevice.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)
 create mode 100644 hw/i386/compositedevice.c

-- 
2.1.4

[Qemu-devel] [RFC] hw/i386: Composite Bus and PCI device

[David Kiarie]

Sample composite SysBus and PCI device similar to AMD IOMMU setup

Signed-off-by: David Kiarie <davidkiarie4@gmail.com>
---
 hw/i386/compositedevice.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)
 create mode 100644 hw/i386/compositedevice.c

diff --git a/hw/i386/compositedevice.c b/hw/i386/compositedevice.c
new file mode 100644
index 0000000..349e98d
--- /dev/null
+++ b/hw/i386/compositedevice.c
@@ -0,0 +1,113 @@
+#include "qemu/osdep.h"
+#include "hw/pci/pci.h"
+#include "hw/i386/x86-iommu.h"
+#include "hw/pci/msi.h"
+#include "hw/pci/pci_bus.h"
+#include "hw/sysbus.h"
+#include "qom/object.h"
+#include "hw/i386/pc.h"
+
+#define AMDVI_MMIO_SIZE        0x4000
+#define AMDVI_CAPAB_SIZE       0x18
+#define AMDVI_CAPAB_REG_SIZE   0x04
+#define AMDVI_CAPAB_ID_SEC     0xff
+
+#define TYPE_AMD_IOMMU_DEVICE "amd-iommu"
+#define AMD_IOMMU_DEVICE(obj)\
+    OBJECT_CHECK(AMDVIState, (obj), TYPE_AMD_IOMMU_DEVICE)
+
+#define TYPE_AMD_IOMMU_PCI "AMDVI-PCI"
+#define AMD_IOMMU_PCI(obj)\
+    OBJECT_CHECK(AMDVIPCIState, (obj), TYPE_AMD_IOMMU_PCI)
+
+typedef struct AMDVIPCIState {
+    PCIDevice dev;
+    /* PCI specific properties */
+    uint8_t *capab;              /* capabilities registers       */
+    uint32_t capab_offset;       /* capability offset pointer    */
+} AMDVIPCIState;
+
+typedef struct AMDVIState {
+    X86IOMMUState iommu;        /* IOMMU bus device             */
+    AMDVIPCIState *dev;         /* IOMMU PCI device             */
+
+    uint8_t mmior[AMDVI_MMIO_SIZE];    /* read/write MMIO              */
+    uint8_t w1cmask[AMDVI_MMIO_SIZE];  /* read/write 1 clear mask      */
+    uint8_t romask[AMDVI_MMIO_SIZE];   /* MMIO read/only mask          */
+} AMDVIState;
+
+static void amdvi_init(AMDVIState *s)
+{
+    /* reset PCI device */
+    pci_config_set_vendor_id(s->dev->dev.config, PCI_VENDOR_ID_AMD);
+    pci_config_set_prog_interface(s->dev->dev.config, 00);
+    pci_config_set_class(s->dev->dev.config, 0x0806);
+}
+
+static void amdvi_reset(DeviceState *dev)
+{
+    AMDVIState *s = AMD_IOMMU_DEVICE(dev);
+
+    amdvi_init(s);
+}
+
+static void amdvi_realize(DeviceState *dev, Error **errp)
+{
+    AMDVIState *s = AMD_IOMMU_DEVICE(dev);
+    PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
+
+    /* This device should take care of IOMMU PCI properties */
+    PCIDevice *createddev = pci_create_simple(bus, -1, TYPE_AMD_IOMMU_PCI);
+    AMDVIPCIState *amdpcidevice = container_of(createddev, AMDVIPCIState, dev);
+    s->dev = amdpcidevice;
+}
+
+static void amdvi_class_init(ObjectClass *klass, void* data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    X86IOMMUClass *dc_class = X86_IOMMU_CLASS(klass);
+
+    dc->reset = amdvi_reset;
+
+    dc_class->realize = amdvi_realize;
+}
+
+static const TypeInfo amdvi = {
+    .name = TYPE_AMD_IOMMU_DEVICE,
+    .parent = TYPE_X86_IOMMU_DEVICE,
+    .instance_size = sizeof(AMDVIState),
+    .class_init = amdvi_class_init
+};
+
+static void amdviPCI_realize(PCIDevice *dev, Error **errp)
+{
+    AMDVIPCIState *s = container_of(dev, AMDVIPCIState, dev);
+
+    /* we need to report certain PCI capabilities */
+    s->capab_offset = pci_add_capability(&s->dev, AMDVI_CAPAB_ID_SEC, 0,
+                                         AMDVI_CAPAB_SIZE);
+    pci_add_capability(&s->dev, PCI_CAP_ID_MSI, 0, AMDVI_CAPAB_REG_SIZE);
+    pci_add_capability(&s->dev, PCI_CAP_ID_HT, 0, AMDVI_CAPAB_REG_SIZE);
+}
+
+static void amdviPCI_class_init(ObjectClass *klass, void* data)
+{
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->realize = amdviPCI_realize;
+}
+
+static const TypeInfo amdviPCI = {
+    .name = TYPE_AMD_IOMMU_PCI,
+    .parent = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(AMDVIPCIState),
+    .class_init = amdviPCI_class_init
+};
+
+static void amdviPCI_register_types(void)
+{
+    type_register_static(&amdviPCI);
+    type_register_static(&amdvi);
+}
+
+type_init(amdviPCI_register_types);
-- 
2.1.4

Re: [Qemu-devel] [RFC] hw/i386: Composite Bus and PCI device

[Eduardo Habkost]

On Wed, Jun 08, 2016 at 01:00:32PM +0300, David Kiarie wrote:
> Sample composite SysBus and PCI device similar to AMD IOMMU setup
> 
> Signed-off-by: David Kiarie <davidkiarie4@gmail.com>
> ---
>  hw/i386/compositedevice.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 113 insertions(+)
>  create mode 100644 hw/i386/compositedevice.c

The filename is very generic (hw/i386/compositedevice.c), but it
has lots of AMD-specific names.

Is your plan to provide generic helpers for implementing
SysBus+PCI devices, or is it going to be inside a source file
specific for AMD IOMMU?

> 
> diff --git a/hw/i386/compositedevice.c b/hw/i386/compositedevice.c
> new file mode 100644
> index 0000000..349e98d
> --- /dev/null
> +++ b/hw/i386/compositedevice.c
> @@ -0,0 +1,113 @@
> +#include "qemu/osdep.h"
> +#include "hw/pci/pci.h"
> +#include "hw/i386/x86-iommu.h"
> +#include "hw/pci/msi.h"
> +#include "hw/pci/pci_bus.h"
> +#include "hw/sysbus.h"
> +#include "qom/object.h"
> +#include "hw/i386/pc.h"
> +
> +#define AMDVI_MMIO_SIZE        0x4000
> +#define AMDVI_CAPAB_SIZE       0x18
> +#define AMDVI_CAPAB_REG_SIZE   0x04
> +#define AMDVI_CAPAB_ID_SEC     0xff
> +
> +#define TYPE_AMD_IOMMU_DEVICE "amd-iommu"
> +#define AMD_IOMMU_DEVICE(obj)\
> +    OBJECT_CHECK(AMDVIState, (obj), TYPE_AMD_IOMMU_DEVICE)
> +
> +#define TYPE_AMD_IOMMU_PCI "AMDVI-PCI"
> +#define AMD_IOMMU_PCI(obj)\
> +    OBJECT_CHECK(AMDVIPCIState, (obj), TYPE_AMD_IOMMU_PCI)
> +
> +typedef struct AMDVIPCIState {
> +    PCIDevice dev;
> +    /* PCI specific properties */
> +    uint8_t *capab;              /* capabilities registers       */

Where is this field used?

> +    uint32_t capab_offset;       /* capability offset pointer    */
> +} AMDVIPCIState;
> +
> +typedef struct AMDVIState {
> +    X86IOMMUState iommu;        /* IOMMU bus device             */
> +    AMDVIPCIState *dev;         /* IOMMU PCI device             */
> +
> +    uint8_t mmior[AMDVI_MMIO_SIZE];    /* read/write MMIO              */
> +    uint8_t w1cmask[AMDVI_MMIO_SIZE];  /* read/write 1 clear mask      */
> +    uint8_t romask[AMDVI_MMIO_SIZE];   /* MMIO read/only mask          */
> +} AMDVIState;
> +
> +static void amdvi_init(AMDVIState *s)
> +{
> +    /* reset PCI device */
> +    pci_config_set_vendor_id(s->dev->dev.config, PCI_VENDOR_ID_AMD);
> +    pci_config_set_prog_interface(s->dev->dev.config, 00);
> +    pci_config_set_class(s->dev->dev.config, 0x0806);
> +}
> +
> +static void amdvi_reset(DeviceState *dev)
> +{
> +    AMDVIState *s = AMD_IOMMU_DEVICE(dev);
> +
> +    amdvi_init(s);
> +}
> +
> +static void amdvi_realize(DeviceState *dev, Error **errp)
> +{
> +    AMDVIState *s = AMD_IOMMU_DEVICE(dev);
> +    PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
> +
> +    /* This device should take care of IOMMU PCI properties */
> +    PCIDevice *createddev = pci_create_simple(bus, -1, TYPE_AMD_IOMMU_PCI);

This pci_create_simple() call can be basically expanded to:
    DeviceState *dev = object_new(TYPE_AMD_IOMMU_PCI)
    qdev_set_parent_bus(dev, bus);
    qdev_prop_set_int32(dev, "addr", -1);
    qdev_prop_set_bit(dev, "multifunction", false);
    object_property_set_bool(OBJECT(dev), true, "realized", &err);

You can embed AMDVIPCIState inside AMDVIState, instead of
creating the object on realize, and the "addr"/"multifunction"
properties already default to -1/false. The code would look like
this:

  typedef struct AMDVIState {
      X86IOMMUState iommu;
      AMDVIPCIState pci;
      [...]
  } AMDVIState;

  static void amdvi_instance_init(...)
  {
      object_initialize(&s->pci, sizeof(s->pci, TYPE_AMD_IOMMU_PCI);
  }

  static void amdvi_realize(DeviceState *dev, Error **errp)
  {
      [...]
      PCIBus *bus = PC_MACHINE(qdev_get_machine())->bus;
      qdev_set_parent_bus(DEVICE(&s->pci), &bus->qbus);
      object_property_set_bool(OBJECT(&s->pci), true, "realized", &err);
  }

Also, a object_property_add_child(obj, "pci-device", &s->pci)
call would help place the PCI object inside the AMD IOMMU device
in the device tree.

> +    AMDVIPCIState *amdpcidevice = container_of(createddev, AMDVIPCIState, dev);
> +    s->dev = amdpcidevice;

Can't you simply use the AMD_IOMMU_PCI macro here?

> +}
> +
> +static void amdvi_class_init(ObjectClass *klass, void* data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    X86IOMMUClass *dc_class = X86_IOMMU_CLASS(klass);
> +
> +    dc->reset = amdvi_reset;
> +
> +    dc_class->realize = amdvi_realize;
> +}
> +
> +static const TypeInfo amdvi = {
> +    .name = TYPE_AMD_IOMMU_DEVICE,
> +    .parent = TYPE_X86_IOMMU_DEVICE,
> +    .instance_size = sizeof(AMDVIState),
> +    .class_init = amdvi_class_init
> +};
> +
> +static void amdviPCI_realize(PCIDevice *dev, Error **errp)
> +{
> +    AMDVIPCIState *s = container_of(dev, AMDVIPCIState, dev);

You can use the AMD_IOMMU_PCI macro here.

> +
> +    /* we need to report certain PCI capabilities */
> +    s->capab_offset = pci_add_capability(&s->dev, AMDVI_CAPAB_ID_SEC, 0,
> +                                         AMDVI_CAPAB_SIZE);

What is s->capab_offset used for?

> +    pci_add_capability(&s->dev, PCI_CAP_ID_MSI, 0, AMDVI_CAPAB_REG_SIZE);
> +    pci_add_capability(&s->dev, PCI_CAP_ID_HT, 0, AMDVI_CAPAB_REG_SIZE);
> +}
> +

Do you have any plans to allow TYPE_AMD_IOMMU_PCI to be used
without TYPE_AMD_IOMMU_DEVICE? If it is never going to be
possible, I would do everything inside
amdvi_instance_init()/amdvi_realize().

I think you don't even need to introduce a TYPE_AMD_IOMMU_PCI
type at all (just use TYPE_PCI_DEVICE/PCIState inside
AMDVIState). But I don't know if this would make the device tree
too confusing (probably it will be OK if you use
object_property_add_child() like I suggested above).

> +static void amdviPCI_class_init(ObjectClass *klass, void* data)
> +{
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +    k->realize = amdviPCI_realize;

You need to set DEVICE_CLASS(klass)->hotpluggable=false, or
people will be able to manually hotplug TYPE_AMD_IOMMU_PCI
devices.

> +}
> +
> +static const TypeInfo amdviPCI = {
> +    .name = TYPE_AMD_IOMMU_PCI,
> +    .parent = TYPE_PCI_DEVICE,
> +    .instance_size = sizeof(AMDVIPCIState),
> +    .class_init = amdviPCI_class_init
> +};
> +
> +static void amdviPCI_register_types(void)
> +{
> +    type_register_static(&amdviPCI);
> +    type_register_static(&amdvi);
> +}
> +
> +type_init(amdviPCI_register_types);
> -- 
> 2.1.4
> 

-- 
Eduardo

Re: [Qemu-devel] [RFC] hw/i386: Composite Bus and PCI device

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

On 2016-06-08 17:25, Eduardo Habkost wrote:
> On Wed, Jun 08, 2016 at 01:00:32PM +0300, David Kiarie wrote:
>> Sample composite SysBus and PCI device similar to AMD IOMMU setup
>>
>> Signed-off-by: David Kiarie <davidkiarie4@gmail.com>
>> ---
>>  hw/i386/compositedevice.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 113 insertions(+)
>>  create mode 100644 hw/i386/compositedevice.c
> 
> The filename is very generic (hw/i386/compositedevice.c), but it
> has lots of AMD-specific names.
> 
> Is your plan to provide generic helpers for implementing
> SysBus+PCI devices, or is it going to be inside a source file
> specific for AMD IOMMU?

As far as I understood - David, correct me - this patch is more of a
simplified demonstrator of the architecture to be applied on the actual
AMD IOMMU code. It is not for merge.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [RFC] hw/i386: Composite Bus and PCI device

[David Kiarie]

On Fri, Jun 10, 2016 at 8:30 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2016-06-08 17:25, Eduardo Habkost wrote:
>> On Wed, Jun 08, 2016 at 01:00:32PM +0300, David Kiarie wrote:
>>> Sample composite SysBus and PCI device similar to AMD IOMMU setup
>>>
>>> Signed-off-by: David Kiarie <davidkiarie4@gmail.com>
>>> ---
>>>  hw/i386/compositedevice.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 113 insertions(+)
>>>  create mode 100644 hw/i386/compositedevice.c
>>
>> The filename is very generic (hw/i386/compositedevice.c), but it
>> has lots of AMD-specific names.
>>
>> Is your plan to provide generic helpers for implementing
>> SysBus+PCI devices, or is it going to be inside a source file
>> specific for AMD IOMMU?
>
> As far as I understood - David, correct me - this patch is more of a
> simplified demonstrator of the architecture to be applied on the actual
> AMD IOMMU code. It is not for merge.

Right.

>
> Jan
>
>

[Qemu-devel] [RFC] AMD IOMMU: emulate multiple devices

[David Kiarie]

Hello all,

This patch tries to solve a problem whereby real AMD IOMMUs exhibit both PCI and Platform device properties. AMD IOMMU properties that conflict with conventional PCI devices' features include the fact that its not a BusMaster device, reserves MMIO region without a BAR register.

There is some already ongoing work on Intel IOMMU Interrupt remapping with implements an IOMMU base class, as a platform device(which means the moment I inherit from this class my device loses it's PCI properties). I am therefore forced to find a way to combine both PCI and platform features(which I had previously avoided) into AMD IOMMU. 

This patch implements a dummy PCI device which serves to 'steal' PCI config space while the rest of the device remains a platform device. The platform device maintains a reference to the PCI and hence the relevant PCI config space. Please ignore details in this patch and review the design. Also, some of the changes here are not related to the above issue.

Looking forward to your comments!

David Kiarie (1):
  Allow AMD IOMMU to have both SysBusDevice and PCIDevice properties.

 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

-- 
2.1.4