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 <address@hidden>
> ---
>  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