* [Qemu-devel] [RFC] AMD IOMMU: emulate multiple devices
@ 2016-06-05 16:54 David Kiarie
0 siblings, 0 replies; 6+ messages in thread
From: David Kiarie @ 2016-06-05 16:54 UTC (permalink / raw)
To: qemu-devel
Cc: imammedo, ehabkost, jasowang, marcel, mst, pbonzini, jan.kiszka,
rkrcmar, alex.williamson, wexu, valentine.sinitsyn, peterx,
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [RFC] AMD IOMMU: emulate multiple devices
@ 2016-06-08 10:00 David Kiarie
2016-06-08 10:00 ` [Qemu-devel] [RFC] hw/i386: Composite Bus and PCI device David Kiarie
0 siblings, 1 reply; 6+ messages in thread
From: David Kiarie @ 2016-06-08 10:00 UTC (permalink / raw)
To: qemu-devel
Cc: imammedo, ehabkost, marcel, mst, pbonzini, davidkiarie4, peterx,
wexu, alex.williamson, rkrcmar, jan.kiszka
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [RFC] hw/i386: Composite Bus and PCI device
2016-06-08 10:00 [Qemu-devel] [RFC] AMD IOMMU: emulate multiple devices David Kiarie
@ 2016-06-08 10:00 ` David Kiarie
2016-06-08 15:25 ` Eduardo Habkost
0 siblings, 1 reply; 6+ messages in thread
From: David Kiarie @ 2016-06-08 10:00 UTC (permalink / raw)
To: qemu-devel
Cc: imammedo, ehabkost, marcel, mst, pbonzini, davidkiarie4, peterx,
wexu, alex.williamson, rkrcmar, jan.kiszka
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [RFC] hw/i386: Composite Bus and PCI device
2016-06-08 10:00 ` [Qemu-devel] [RFC] hw/i386: Composite Bus and PCI device David Kiarie
@ 2016-06-08 15:25 ` Eduardo Habkost
2016-06-10 5:30 ` Jan Kiszka
0 siblings, 1 reply; 6+ messages in thread
From: Eduardo Habkost @ 2016-06-08 15:25 UTC (permalink / raw)
To: David Kiarie
Cc: qemu-devel, imammedo, marcel, mst, pbonzini, peterx, wexu,
alex.williamson, rkrcmar, jan.kiszka
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [RFC] hw/i386: Composite Bus and PCI device
2016-06-08 15:25 ` Eduardo Habkost
@ 2016-06-10 5:30 ` Jan Kiszka
2016-06-11 19:36 ` David Kiarie
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kiszka @ 2016-06-10 5:30 UTC (permalink / raw)
To: Eduardo Habkost, David Kiarie
Cc: qemu-devel, imammedo, marcel, mst, pbonzini, peterx, wexu,
alex.williamson, rkrcmar
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [RFC] hw/i386: Composite Bus and PCI device
2016-06-10 5:30 ` Jan Kiszka
@ 2016-06-11 19:36 ` David Kiarie
0 siblings, 0 replies; 6+ messages in thread
From: David Kiarie @ 2016-06-11 19:36 UTC (permalink / raw)
To: Jan Kiszka
Cc: Eduardo Habkost, QEMU Developers, imammedo, Marcel Apfelbaum,
Michael S. Tsirkin, pbonzini, Peter Xu, wexu, alex.williamson,
rkrcmar
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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-06-11 19:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-06-08 10:00 [Qemu-devel] [RFC] AMD IOMMU: emulate multiple devices David Kiarie
2016-06-08 10:00 ` [Qemu-devel] [RFC] hw/i386: Composite Bus and PCI device David Kiarie
2016-06-08 15:25 ` Eduardo Habkost
2016-06-10 5:30 ` Jan Kiszka
2016-06-11 19:36 ` David Kiarie
-- strict thread matches above, loose matches on Subject: below --
2016-06-05 16:54 [Qemu-devel] [RFC] AMD IOMMU: emulate multiple devices David Kiarie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).