| Message ID | 1465380032-12807-2-git-send-email-davidkiarie4@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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 > >
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);
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