[Qemu-devel] make qdev to use already assigned memory

[Qemu-devel] make qdev to use already assigned memory

[Juan Quintela]

Hi

main qdev function is:

DeviceState *qdev_create(BusState *bus, const char *name)

It calls qemu_malloc() itself to create new device.
I am wanting more and more the function:

DeviceState *qdev_create_here(DeviceState *dev, BusState *bus, const char *name)

The only change is that it don't want qemu_malloc(), it just initialize
the device in the memory that I bring there.  Why do I want this?

OK, VMState prefers very much static members vs pointers.  You can't
type check, and be sure that you are doing the right thing with static
fields than with pointers.

Now, the examples are almost always devices that contain devices, and
you always need to go from one to the other.

hw/piix_pci.c::

If I had qemu_create_here() I would be able to embed the two fields
pic and piix3 inside I440FXState, and then I didn't need to go through
hops to arrive to them.

irq_state variable and type exist only due to the fact that there is no
way that I can pass i440FX to it. (this case is still worse, because
when we create a bus, we need the northbridge (i440FXState and
southbridge piix3).  Notice that piix3 always exist, i.e. it is not as
if the field is optional, have a variable size, ....  We know statically
that we need it.

PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn, qemu_irq *pic)
{
    DeviceState *dev;
    PCIBus *b;
    PCIDevice *d;
    I440FXState *s;
    PIIX3IrqState *irq_state = qemu_malloc(sizeof(*irq_state));

    irq_state->pic = pic;
    dev = qdev_create(NULL, "i440FX-pcihost");
    s = FROM_SYSBUS(I440FXState, sysbus_from_qdev(dev));
    b = pci_register_bus(&s->busdev.qdev, "pci.0",
                         piix3_set_irq, pci_slot_get_pirq, irq_state, 0, 4);
    s->bus = b;
    qdev_init(dev);

    d = pci_create_simple(b, 0, "i440FX");
    *pi440fx_state = DO_UPCAST(PCII440FXState, dev, d);
    (*pi440fx_state)->irq_state = irq_state;

    irq_state->piix3 = DO_UPCAST(PIIX3State, dev,
                                 pci_create_simple(b, -1, "PIIX3"));
    *piix3_devfn = irq_state->piix3->dev.devfn;

    return b;
}

hw/pxa2xx.c:

i2c slave device.  we now that we need it, but we have to create it
through a pointer, anyways:

/* I2C Interface */
typedef struct {
    i2c_slave i2c;
    ...
} PXA2xxI2CSlaveState;

struct PXA2xxI2CState {
    PXA2xxI2CSlaveState *slave;
    .....
};

PXA2xxI2CState *pxa2xx_i2c_init(target_phys_addr_t base,
                qemu_irq irq, uint32_t region_size)
{
    int iomemtype;
    DeviceState *dev;
    PXA2xxI2CState *s = qemu_mallocz(sizeof(PXA2xxI2CState));

    /* FIXME: Should the slave device really be on a separate bus?  */
    dev = i2c_create_slave(i2c_init_bus(NULL, "dummy"), "pxa2xx-i2c-slave", 0);
    s->slave = FROM_I2C_SLAVE(PXA2xxI2CSlaveState, I2C_SLAVE_FROM_QDEV(dev));
    s->slave->host = s;
....

}

hw/fdc.c

Here there are no dependencies at all, just that we need memory that is
512bytes aligned.

struct fdctrl_t {
    ....
    /* Command FIFO */
    uint8_t *fifo;
    ....
};

static int fdctrl_init_common(fdctrl_t *fdctrl)
{
....
    fdctrl->fifo = qemu_memalign(512, FD_SECTOR_LEN);
    fdctrl->fifo_size = 512;
.....
}

qdev knows nothing about memory alignment.  If there were no qdev, my
preferred solution should have been something like:

struct fdctrl_t {
    /* Command FIFO */ /* 1st field */
    uint8_t fifo[512];
    ....
};

And now I will call qemu_memalign() for the whole structure.

ide/*

Last ide series from Gerd just add another 4 cases, where we have to
use pointers due to this qdev limitation.

What to do?

I don't know how to go from here.  I continue seeing this kind of
examples as I am converting devices to VMState.  We are using pointers
just due to the qdev limitations.

On the other hand, Gerd told me that qdev_create_here() and machine
description + -device .... is not going to fly.

Is there a nice solution that I am not able to see?
I am just having bad luck and have already hit all the cases where qdev
don't fit well?
Should we just admit this qdev limitation and use the pointers?
Should we just add qdev_create_here() and deal with the machine
description/-device handling in a case by case?

Anyone got the magic bullet?  Comments?

Later, Juan.

[Qemu-devel] Re: make qdev to use already assigned memory

[Gerd Hoffmann]

   Hi,

> DeviceState *qdev_create_here(DeviceState *dev, BusState *bus, const char *name)
>
> The only change is that it don't want qemu_malloc(), it just initialize
> the device in the memory that I bring there.  Why do I want this?

I'd prefer to not have such a function.

At the end of the qdev conversion we want to have all drivers in a state 
where you can create the devices using the basic sequence ...

   (1) qdev_create()
   (2) setting properties
   (2) qdev_init()

... from generic code.  qdev_create_here() simply doesn't fit in here. 
It can by definition not be called by generic code.  Only a very few 
special cases could actually make use of it.

It would make alot of sense to allow *bus* data structures being 
embedded though.  A bus is never ever created on its own, it is allways 
created by the parent device (lsi creates a scsi bus, ...).  Would that 
solve your problems?

>      PIIX3IrqState *irq_state = qemu_malloc(sizeof(*irq_state));
>
>      irq_state->pic = pic;
>      dev = qdev_create(NULL, "i440FX-pcihost");
>      s = FROM_SYSBUS(I440FXState, sysbus_from_qdev(dev));
>      b = pci_register_bus(&s->busdev.qdev, "pci.0",
>                           piix3_set_irq, pci_slot_get_pirq, irq_state, 0, 4);
>      s->bus = b;
>      qdev_init(dev);
>
>      d = pci_create_simple(b, 0, "i440FX");
>      *pi440fx_state = DO_UPCAST(PCII440FXState, dev, d);
>      (*pi440fx_state)->irq_state = irq_state;

I think that one can also be solved by splitting pci bus registration 
and pci bus irq setup into two functions.

> hw/fdc.c

> struct fdctrl_t {
>      /* Command FIFO */ /* 1st field */
>      uint8_t fifo[512];
>      ....
> };

> And now I will call qemu_memalign() for the whole structure.

Doesn't fly.  isa-fdc is pretty close to the state where it can be 
created via -device, we just need the drive windup.  When creating the 
floppy controller via '-device isa-fdc,driveA=foo,driveB=bar' or 
simliar, who will call qemu_memalign then?  Also note that DeviceState 
must be at offset zero of the device state struct.

cheers,
   Gerd

[Qemu-devel] Re: make qdev to use already assigned memory

[Juan Quintela]

Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
>> DeviceState *qdev_create_here(DeviceState *dev, BusState *bus, const char *name)
>>
>> The only change is that it don't want qemu_malloc(), it just initialize
>> the device in the memory that I bring there.  Why do I want this?
>
> I'd prefer to not have such a function.
>
> At the end of the qdev conversion we want to have all drivers in a
> state where you can create the devices using the basic sequence ...
>
>   (1) qdev_create()
>   (2) setting properties
>   (2) qdev_init()
>
> ... from generic code.  qdev_create_here() simply doesn't fit in
> here. It can by definition not be called by generic code.  Only a very
> few special cases could actually make use of it.
>
> It would make alot of sense to allow *bus* data structures being
> embedded though.  A bus is never ever created on its own, it is
> allways created by the parent device (lsi creates a scsi bus, ...).
> Would that solve your problems?

At least it is a big step in the right direction :)
I think we would have to set with this one for now.  If I found any
other pattern, would let you know.

> I think that one can also be solved by splitting pci bus registration
> and pci bus irq setup into two functions.

It will fix this one.

>> hw/fdc.c
>
>> struct fdctrl_t {
>>      /* Command FIFO */ /* 1st field */
>>      uint8_t fifo[512];
>>      ....
>> };
>
>> And now I will call qemu_memalign() for the whole structure.
>
> Doesn't fly.  isa-fdc is pretty close to the state where it can be
> created via -device, we just need the drive windup.  When creating the
> floppy controller via '-device isa-fdc,driveA=foo,driveB=bar' or
> simliar, who will call qemu_memalign then?  Also note that DeviceState
> must be at offset zero of the device state struct.

I know this one in borderline (to say the less), but I was just putting
the examples where i needed them in the last two days.  I.e. that they
weren't "theoretical" problems.

Thanks, Juan.