Re: [Qemu-devel] [PATCH 2/2] i82378: cleanup implementation

[Hervé Poussineau]

Andreas Färber a écrit :
> Hi,
>
> Am 23.07.2013 23:16, schrieb Hervé Poussineau:
> > - i82378 only exists on PCI bus ; do not split implementation in 2 structures
> > - remove BARs, which are not specified in datasheet
> > - replace custom isa_mmio implementation by PCI bus IO region usage
> > - use QOM casts when required
> >
> > Signed-off-by: Hervé Poussineau <address@hidden>
>
> Thanks for adopting some of the latest patterns without being asked to!
>
> I've queued this with some style changes, but apart from testing issues
> as CC'ed, I have a major question/concern in the bottom...
>
> > ---
> >  hw/isa/i82378.c |  220 ++++++++++++-------------------------------------------
> >  1 file changed, 48 insertions(+), 172 deletions(-)
> >
> > diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
> > index de71d81..f2045de 100644
> > --- a/hw/isa/i82378.c
> > +++ b/hw/isa/i82378.c

[...]

> > @@ -159,19 +52,36 @@ static void i82378_request_out0_irq(void *opaque, int irq, 
> > int level)
> >  static void i82378_request_pic_irq(void *opaque, int irq, int level)
> >  {
> >      DeviceState *dev = opaque;
> > -    PCIDevice *pci = DO_UPCAST(PCIDevice, qdev, dev);
> > -    PCIi82378State *s = DO_UPCAST(PCIi82378State, pci_dev, pci);
> > -
> > -    qemu_set_irq(s->state.i8259[irq], level);
> > +    qemu_set_irq(I82378(dev)->i8259[irq], level);
>
> Changed that back to a variable s - FOO(bar)->baz is undesired.

OK

> >  }
> >  
> > -static void i82378_init(DeviceState *dev, I82378State *s)
> > +static void i82378_realize(DeviceState *dev, Error **errp)
> >  {
> > -    ISABus *isabus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
> > -    ISADevice *pit;
> > +    PCIDevice *pci = PCI_DEVICE(dev);
> > +    I82378State *s = I82378(dev);
> > +    DeviceClass *dc;
> > +    uint8_t *pci_conf;
> > +    ISABus *isabus;
> >      ISADevice *isa;
> >      qemu_irq *out0_irq;
> >  
> > +    dc = DEVICE_CLASS(object_class_get_parent(object_get_class(OBJECT(dev))));
>
> This is going into uncharted territories. ;) I consider it wrong to use
> object_get_class() - we should use object_class_by_name() to allow for
> derived types and I'll put it into a macro that I'll try to align with
> Peter C.'s and my QOM work.

OK

> > +    assert(dc);
>
> This shouldn't be necessary?

OK. It can be removed.

> > +    dc->realize(dev, errp);
> > +    if (error_is_set(errp)) {
> > +        return;
>
> This doesn't do what you want. You need a local err variable to return
> here, errp might be NULL => !error_is_set(errp).

OK

[...]

> >  
> > -    k->init = pci_i82378_init;
> >      k->vendor_id = PCI_VENDOR_ID_INTEL;
> >      k->device_id = PCI_DEVICE_ID_INTEL_82378;
> >      k->revision = 0x03;
> >      k->class_id = PCI_CLASS_BRIDGE_ISA;
> > -    k->subsystem_vendor_id = 0x0;
> > -    k->subsystem_id = 0x0;
> > -    dc->vmsd = &vmstate_pci_i82378;
> > -    dc->props = i82378_properties;
> > +    dc->realize = i82378_realize;
> > +    dc->vmsd = &vmstate_i82378;
>
> (FWIW dc->categories has been merged here.)

Yes, but it has been merged after I sent this patch...

> > +    dc->no_user = 1;
>
> Why do you do this? For one, according to Anthony it should no longer be
> used, and for another, Paolo's endianness-test (make check) is using
> -device i82378 for various other ppc and sh4 machines IIUC. make check
> still succeeds for ppc with this patch though, so that might be due tot
> -device ignoring DeviceClass::no_user?

I probably copied it from another chipset device, maybe i440fx.
I don't really mind removing it.

Yes, I double-checked that make check still works for all architectures.

> Hoping to get this in shape for -rc1.

Sure. Should I send a v2, as it seems you already queued it?

Hervé