[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
>
>