Re: [Qemu-devel] [PATCH 10/12] qdev/isa: convert ne2000

[Markus Armbruster <armbru@redhat.com>]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Also split the isa bits into a separate source file, so we don't drag in
> a dependency for isa-bus.o for machines which want ne2k_pci only.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Closer examination led to a few questions...

> ---
>  Makefile.target |    5 +-
>  hw/mips_r4k.c   |    2 +-
>  hw/ne2000-isa.c |  111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/ne2000.c     |  106 +++++++++-------------------------------------------
>  hw/ne2000.h     |   40 ++++++++++++++++++++
>  hw/pc.c         |    2 +-
>  hw/pc.h         |    2 +-
>  hw/ppc_prep.c   |    2 +-
>  8 files changed, 177 insertions(+), 93 deletions(-)
>  create mode 100644 hw/ne2000-isa.c
>  create mode 100644 hw/ne2000.h
>
> diff --git a/Makefile.target b/Makefile.target
> index 67bd4d7..2c03877 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -188,13 +188,14 @@ obj-i386-y += fdc.o mc146818rtc.o serial.o i8259.o i8254.o pcspk.o pc.o
>  obj-i386-y += cirrus_vga.o apic.o ioapic.o parallel.o acpi.o piix_pci.o
>  obj-i386-y += usb-uhci.o vmmouse.o vmport.o vmware_vga.o hpet.o
>  obj-i386-y += device-hotplug.o pci-hotplug.o smbios.o wdt_ib700.o
> +obj-i386-y += ne2000-isa.o
>  
>  # shared objects
>  obj-ppc-y = ppc.o ide/core.o ide/isa.o ide/pci.o ide/macio.o
>  obj-ppc-y += vga.o $(sound-obj-y) dma.o isa-bus.o openpic.o
>  # PREP target
>  obj-ppc-y += pckbd.o serial.o i8259.o i8254.o fdc.o mc146818rtc.o
> -obj-ppc-y += prep_pci.o ppc_prep.o
> +obj-ppc-y += prep_pci.o ppc_prep.o ne2000-isa.o
>  # Mac shared devices
>  obj-ppc-y += macio.o cuda.o adb.o mac_nvram.o mac_dbdma.o
>  # OldWorld PowerMac
> @@ -215,7 +216,7 @@ obj-mips-y += g364fb.o jazz_led.o dp8393x.o
>  obj-mips-y += ide/core.o ide/isa.o ide/pci.o
>  obj-mips-y += gt64xxx.o pckbd.o fdc.o mc146818rtc.o usb-uhci.o acpi.o ds1225y.o
>  obj-mips-y += piix4.o parallel.o cirrus_vga.o isa-bus.o pcspk.o $(sound-obj-y)
> -obj-mips-y += mipsnet.o
> +obj-mips-y += mipsnet.o ne2000-isa.o
>  obj-mips-y += pflash_cfi01.o
>  obj-mips-y += vmware_vga.o
>  
> diff --git a/hw/mips_r4k.c b/hw/mips_r4k.c
> index da5ca31..aacb256 100644
> --- a/hw/mips_r4k.c
> +++ b/hw/mips_r4k.c
> @@ -260,7 +260,7 @@ void mips_r4k_init (ram_addr_t ram_size,
>      isa_vga_init();
>  
>      if (nd_table[0].vlan)
> -        isa_ne2000_init(0x300, i8259[9], &nd_table[0]);
> +        isa_ne2000_init(0x300, 9, &nd_table[0]);
>  
>      if (drive_get_max_bus(IF_IDE) >= MAX_IDE_BUS) {
>          fprintf(stderr, "qemu: too many IDE bus\n");
> diff --git a/hw/ne2000-isa.c b/hw/ne2000-isa.c
> new file mode 100644
> index 0000000..3120035
> --- /dev/null
> +++ b/hw/ne2000-isa.c
> @@ -0,0 +1,111 @@
> +/*
> + * QEMU NE2000 emulation -- isa bus windup
> + *
> + * Copyright (c) 2003-2004 Fabrice Bellard
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +#include "hw.h"
> +#include "pc.h"
> +#include "isa.h"
> +#include "qdev.h"
> +#include "net.h"
> +#include "ne2000.h"
> +
> +typedef struct ISANE2000State {
> +    ISADevice dev;
> +    uint32_t iobase;
> +    uint32_t isairq;
> +    NE2000State ne2000;
> +} ISANE2000State;
> +
> +static void isa_ne2000_cleanup(VLANClientState *vc)
> +{
> +    NE2000State *s = vc->opaque;
> +    ISANE2000State *isa = container_of(s, ISANE2000State, ne2000);
> +
> +    unregister_savevm("ne2000", s);
> +
> +    isa_unassign_ioport(isa->iobase, 16);
> +    isa_unassign_ioport(isa->iobase + 0x10, 2);
> +    isa_unassign_ioport(isa->iobase + 0x1f, 1);
> +
> +    qemu_free(s);
> +}
> +
> +static int isa_ne2000_initfn(ISADevice *dev)
> +{
> +    ISANE2000State *isa = DO_UPCAST(ISANE2000State, dev, dev);
> +    NE2000State *s = &isa->ne2000;
> +
> +    register_ioport_write(isa->iobase, 16, 1, ne2000_ioport_write, s);
> +    register_ioport_read(isa->iobase, 16, 1, ne2000_ioport_read, s);
> +
> +    register_ioport_write(isa->iobase + 0x10, 1, 1, ne2000_asic_ioport_write, s);
> +    register_ioport_read(isa->iobase + 0x10, 1, 1, ne2000_asic_ioport_read, s);
> +    register_ioport_write(isa->iobase + 0x10, 2, 2, ne2000_asic_ioport_write, s);
> +    register_ioport_read(isa->iobase + 0x10, 2, 2, ne2000_asic_ioport_read, s);
> +
> +    register_ioport_write(isa->iobase + 0x1f, 1, 1, ne2000_reset_ioport_write, s);
> +    register_ioport_read(isa->iobase + 0x1f, 1, 1, ne2000_reset_ioport_read, s);
> +
> +    isa_init_irq(dev, &s->irq, isa->isairq);
> +
> +    register_savevm("ne2000", -1, 2, ne2000_save, ne2000_load, s);
> +    return 0;
> +}
> +
> +void isa_ne2000_init(int base, int irq, NICInfo *nd)

Nitpick: in qdev parlance, this function is a create_simple, not an
init.

> +{
> +    ISADevice *dev;
> +    NE2000State *s;
> +
> +    qemu_check_nic_model(nd, "ne2k_isa");
> +
> +    dev = isa_create("ne2k_isa");
> +    qdev_prop_set_uint32(&dev->qdev, "iobase", base);
> +    qdev_prop_set_uint32(&dev->qdev, "irq",    irq);
> +    qdev_init(&dev->qdev);
> +

Shouldn't the rest be in isa_ne2000_initfn()?  Think of -device, which
doesn't go through this function...

> +    s = &DO_UPCAST(ISANE2000State, dev, dev)->ne2000;
> +    memcpy(s->macaddr, nd->macaddr, 6);
> +    ne2000_reset(s); /* needs macaddr */
> +    s->vc = nd->vc = qemu_new_vlan_client(nd->vlan, nd->model, nd->name,
> +                                          ne2000_can_receive, ne2000_receive,
> +                                          NULL, isa_ne2000_cleanup, s);

Shouldn't this use qdev_get_vlan_client(), like the other qdevified
NICs?

> +    qemu_format_nic_info_str(s->vc, s->macaddr);
> +}
> +
> +static ISADeviceInfo ne2000_isa_info = {
> +    .qdev.name  = "ne2k_isa",
> +    .qdev.size  = sizeof(ISANE2000State),
> +    .init       = isa_ne2000_initfn,
> +    .qdev.props = (Property[]) {
> +        DEFINE_PROP_HEX32("iobase", ISANE2000State, iobase, 0x300),
> +        DEFINE_PROP_UINT32("irq",   ISANE2000State, isairq, 9),
> +        DEFINE_PROP_END_OF_LIST(),
> +    },
> +};
> +
> +static void ne2000_isa_register_devices(void)
> +{
> +    isa_qdev_register(&ne2000_isa_info);
> +}
> +
> +device_init(ne2000_isa_register_devices)
> diff --git a/hw/ne2000.c b/hw/ne2000.c
> index bdfc9ed..cde64be 100644
> --- a/hw/ne2000.c
> +++ b/hw/ne2000.c
> @@ -25,6 +25,7 @@
>  #include "pci.h"
>  #include "pc.h"
>  #include "net.h"
> +#include "ne2000.h"
>  
>  /* debug NE2000 card */
>  //#define DEBUG_NE2000
> @@ -116,42 +117,19 @@
>  #define ENTSR_CDH 0x40	/* The collision detect "heartbeat" signal was lost. */
>  #define ENTSR_OWC 0x80  /* There was an out-of-window collision. */
>  
> -#define NE2000_PMEM_SIZE    (32*1024)
> -#define NE2000_PMEM_START   (16*1024)
> -#define NE2000_PMEM_END     (NE2000_PMEM_SIZE+NE2000_PMEM_START)
> -#define NE2000_MEM_SIZE     NE2000_PMEM_END
> -
> -typedef struct NE2000State {
> -    uint8_t cmd;
> -    uint32_t start;
> -    uint32_t stop;
> -    uint8_t boundary;
> -    uint8_t tsr;
> -    uint8_t tpsr;
> -    uint16_t tcnt;
> -    uint16_t rcnt;
> -    uint32_t rsar;
> -    uint8_t rsr;
> -    uint8_t rxcr;
> -    uint8_t isr;
> -    uint8_t dcfg;
> -    uint8_t imr;
> -    uint8_t phys[6]; /* mac address */
> -    uint8_t curpag;
> -    uint8_t mult[8]; /* multicast mask array */
> -    qemu_irq irq;
> -    int isa_io_base;
> -    VLANClientState *vc;
> -    uint8_t macaddr[6];
> -    uint8_t mem[NE2000_MEM_SIZE];
> -} NE2000State;
> -
>  typedef struct PCINE2000State {
>      PCIDevice dev;
>      NE2000State ne2000;
>  } PCINE2000State;
>  
> -static void ne2000_reset(NE2000State *s)
> +typedef struct ISANE2000State {
> +    ISADevice dev;
> +    uint32_t iobase;
> +    uint32_t isairq;
> +    NE2000State ne2000;
> +} ISANE2000State;

ISANE2000State doesn't belong here, and isn't used as far as I can see.

> +
> +void ne2000_reset(NE2000State *s)
>  {
>      int i;
>  
[...]