[Qemu-devel] Re: Compile files only once: some planning

[Michael S. Tsirkin]

On Wed, Mar 24, 2010 at 10:05:10PM +0200, Blue Swirl wrote:
> On 3/24/10, Michael S. Tsirkin <address@hidden> wrote:
> > On Tue, Mar 23, 2010 at 11:43:51PM +0200, Blue Swirl wrote:
> >  > rtl8139.c, e1000.c: need to convert ldl/stl to 
> > cpu_physical_memory_read/write.
> >
> >
> > I don't see how it would help. These still get target_phys_addr_t which
> >  is per-target. Further, a ton of devices do
> >  cpu_register_physical_memory/qemu_register_coalesced_mmio.
> >  These are also per target.
> 
> I don't know what I was eating yesterday: there are no references to
> ldl/stl in either rtl8139 or e1000. In fact, the conversion is simple
> for the device itself, just add a property "be". The attached patch
> performs this part.
> 
> But now there is a bigger problem, how to pass the property to the
> device. It's not fair to require the user to remember to set it.

I still don't fully understand how come e1000 cares about
target endianness.

> >  A simple solution would be to change all of cpu_XX functions to
> >  get a 64 bit address. This is a lot of churn, if we do this
> >  anyway we should also pass length to callbacks, this way
> >  rwhandler will get very small or go away completely.
> 
> It's not too much effort to keep the target_phys_addr_t type.

I don't understand - target_phys_addr_t is different for different
targets to we will need to recompile the code for each target.
What am I missing?


> From e0ab5cc41c68207be558ccb330f4fb83fba4ee6f Mon Sep 17 00:00:00 2001
> From: Blue Swirl <address@hidden>
> Date: Wed, 24 Mar 2010 19:54:05 +0000
> Subject: [PATCH] Compile rtl8139 and e1000 only once
> 
> WIP
> 
> Signed-off-by: Blue Swirl <address@hidden>
> ---
>  Makefile.objs   |    2 +
>  Makefile.target |    4 --
>  hw/e1000.c      |  108 ++++++++++++++++++++++++++++++++++++++++++------------
>  hw/rtl8139.c    |   82 +++++++++++++++++++++++++++++++-----------
>  4 files changed, 147 insertions(+), 49 deletions(-)
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 281f7a6..54895f8 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -155,6 +155,8 @@ hw-obj-y += msix.o
>  hw-obj-y += ne2000.o
>  hw-obj-y += eepro100.o
>  hw-obj-y += pcnet.o
> +hw-obj-y += rtl8139.o
> +hw-obj-y += e1000.o
>  
>  hw-obj-$(CONFIG_SMC91C111) += smc91c111.o
>  hw-obj-$(CONFIG_LAN9118) += lan9118.o
> diff --git a/Makefile.target b/Makefile.target
> index eb4d010..1a86fc4 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -176,10 +176,6 @@ QEMU_CFLAGS += $(VNC_SASL_CFLAGS)
>  # xen backend driver support
>  obj-$(CONFIG_XEN) += xen_machine_pv.o xen_domainbuild.o
>  
> -# PCI network cards
> -obj-y += rtl8139.o
> -obj-y += e1000.o
> -
>  # Hardware support
>  obj-i386-y = ide/core.o
>  obj-i386-y += pckbd.o dma.o
> diff --git a/hw/e1000.c b/hw/e1000.c
> index fd3059a..0f72db8 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -121,6 +121,7 @@ typedef struct E1000State_st {
>          uint16_t reading;
>          uint32_t old_eecd;
>      } eecd_state;
> +    uint32_t be;
>  } E1000State;
>  
>  #define      defreg(x)       x = (E1000_##x>>2)
> @@ -825,14 +826,11 @@ static void (*macreg_writeops[])(E1000State *, int, 
> uint32_t) = {
>  enum { NWRITEOPS = ARRAY_SIZE(macreg_writeops) };
>  
>  static void
> -e1000_mmio_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
> +e1000_mmio_writel_le(void *opaque, target_phys_addr_t addr, uint32_t val)
>  {
>      E1000State *s = opaque;
>      unsigned int index = (addr & 0x1ffff) >> 2;
>  
> -#ifdef TARGET_WORDS_BIGENDIAN
> -    val = bswap32(val);
> -#endif
>      if (index < NWRITEOPS && macreg_writeops[index])
>          macreg_writeops[index](s, index, val);
>      else if (index < NREADOPS && macreg_readops[index])
> @@ -841,25 +839,47 @@ e1000_mmio_writel(void *opaque, target_phys_addr_t 
> addr, uint32_t val)
>          DBGOUT(UNKNOWN, "MMIO unknown write addr=0x%08x,val=0x%08x\n",
>                 index<<2, val);
>  }
> +static void
> +e1000_mmio_writel_be(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    val = bswap32(val);
> +    e1000_mmio_writel_le(opaque, addr, val);
> +}
>  
>  static void
> -e1000_mmio_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
> +e1000_mmio_writew_le(void *opaque, target_phys_addr_t addr, uint32_t val)
>  {
>      // emulate hw without byte enables: no RMW
> -    e1000_mmio_writel(opaque, addr & ~3,
> -                      (val & 0xffff) << (8*(addr & 3)));
> +    e1000_mmio_writel_le(opaque, addr & ~3,
> +                         (val & 0xffff) << (8*(addr & 3)));
>  }
>  
>  static void
> -e1000_mmio_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
> +e1000_mmio_writew_be(void *opaque, target_phys_addr_t addr, uint32_t val)
>  {
>      // emulate hw without byte enables: no RMW
> -    e1000_mmio_writel(opaque, addr & ~3,
> -                      (val & 0xff) << (8*(addr & 3)));
> +    e1000_mmio_writel_be(opaque, addr & ~3,
> +                         (val & 0xffff) << (8*(addr & 3)));
> +}
> +
> +static void
> +e1000_mmio_writeb_be(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    // emulate hw without byte enables: no RMW
> +    e1000_mmio_writel_be(opaque, addr & ~3,
> +                         (val & 0xff) << (8*(addr & 3)));
> +}
> +
> +static void
> +e1000_mmio_writeb_le(void *opaque, target_phys_addr_t addr, uint32_t val)
> +{
> +    // emulate hw without byte enables: no RMW
> +    e1000_mmio_writel_le(opaque, addr & ~3,
> +                         (val & 0xff) << (8*(addr & 3)));
>  }
>  
>  static uint32_t
> -e1000_mmio_readl(void *opaque, target_phys_addr_t addr)
> +e1000_mmio_readl_le(void *opaque, target_phys_addr_t addr)
>  {
>      E1000State *s = opaque;
>      unsigned int index = (addr & 0x1ffff) >> 2;
> @@ -867,9 +887,6 @@ e1000_mmio_readl(void *opaque, target_phys_addr_t addr)
>      if (index < NREADOPS && macreg_readops[index])
>      {
>          uint32_t val = macreg_readops[index](s, index);
> -#ifdef TARGET_WORDS_BIGENDIAN
> -        val = bswap32(val);
> -#endif
>          return val;
>      }
>      DBGOUT(UNKNOWN, "MMIO unknown read addr=0x%08x\n", index<<2);
> @@ -877,16 +894,38 @@ e1000_mmio_readl(void *opaque, target_phys_addr_t addr)
>  }
>  
>  static uint32_t
> -e1000_mmio_readb(void *opaque, target_phys_addr_t addr)
> +e1000_mmio_readl_be(void *opaque, target_phys_addr_t addr)
>  {
> -    return ((e1000_mmio_readl(opaque, addr & ~3)) >>
> +    uint32_t val = e1000_mmio_readl_le(opaque, addr);
> +    val = bswap32(val);
> +    return val;
> +}
> +
> +static uint32_t
> +e1000_mmio_readb_be(void *opaque, target_phys_addr_t addr)
> +{
> +    return ((e1000_mmio_readl_be(opaque, addr & ~3)) >>
>              (8 * (addr & 3))) & 0xff;
>  }
>  
>  static uint32_t
> -e1000_mmio_readw(void *opaque, target_phys_addr_t addr)
> +e1000_mmio_readb_le(void *opaque, target_phys_addr_t addr)
> +{
> +    return ((e1000_mmio_readl_le(opaque, addr & ~3)) >>
> +            (8 * (addr & 3))) & 0xff;
> +}
> +
> +static uint32_t
> +e1000_mmio_readw_le(void *opaque, target_phys_addr_t addr)
> +{
> +    return ((e1000_mmio_readl_le(opaque, addr & ~3)) >>
> +            (8 * (addr & 3))) & 0xffff;
> +}
> +
> +static uint32_t
> +e1000_mmio_readw_be(void *opaque, target_phys_addr_t addr)
>  {
> -    return ((e1000_mmio_readl(opaque, addr & ~3)) >>
> +    return ((e1000_mmio_readl_be(opaque, addr & ~3)) >>
>              (8 * (addr & 3))) & 0xffff;
>  }
>  
> @@ -1008,13 +1047,28 @@ static const uint32_t mac_reg_init[] = {
>  };
>  
>  /* PCI interface */
> +static CPUWriteMemoryFunc * const e1000_mmio_write_be[] = {
> +    e1000_mmio_writeb_be,
> +    e1000_mmio_writew_be,
> +    e1000_mmio_writel_be
> +};
>  
> -static CPUWriteMemoryFunc * const e1000_mmio_write[] = {
> -    e1000_mmio_writeb,       e1000_mmio_writew,      e1000_mmio_writel
> +static CPUReadMemoryFunc * const e1000_mmio_read_be[] = {
> +    e1000_mmio_readb_be,
> +    e1000_mmio_readw_be,
> +    e1000_mmio_readl_be
>  };
>  
> -static CPUReadMemoryFunc * const e1000_mmio_read[] = {
> -    e1000_mmio_readb,        e1000_mmio_readw,       e1000_mmio_readl
> +static CPUWriteMemoryFunc * const e1000_mmio_write_le[] = {
> +    e1000_mmio_writeb_le,
> +    e1000_mmio_writew_le,
> +    e1000_mmio_writel_le
> +};
> +
> +static CPUReadMemoryFunc * const e1000_mmio_read_le[] = {
> +    e1000_mmio_readb_le,
> +    e1000_mmio_readw_le,
> +    e1000_mmio_readl_le
>  };
>  
>  static void
> @@ -1102,8 +1156,13 @@ static int pci_e1000_init(PCIDevice *pci_dev)
>      /* TODO: RST# value should be 0 if programmable, PCI spec 6.2.4 */
>      pci_conf[PCI_INTERRUPT_PIN] = 1; // interrupt pin 0
>  
> -    d->mmio_index = cpu_register_io_memory(e1000_mmio_read,
> -            e1000_mmio_write, d);
> +    if (d->be) {
> +        d->mmio_index = cpu_register_io_memory(e1000_mmio_read_be,
> +                                               e1000_mmio_write_be, d);
> +    } else {
> +        d->mmio_index = cpu_register_io_memory(e1000_mmio_read_le,
> +                                               e1000_mmio_write_le, d);
> +    }
>  
>      pci_register_bar((PCIDevice *)d, 0, PNPMMIO_SIZE,
>                             PCI_BASE_ADDRESS_SPACE_MEMORY, e1000_mmio_map);
> @@ -1146,6 +1205,7 @@ static PCIDeviceInfo e1000_info = {
>      .romfile    = "pxe-e1000.bin",
>      .qdev.props = (Property[]) {
>          DEFINE_NIC_PROPERTIES(E1000State, conf),
> +        DEFINE_PROP_UINT32("be", E1000State, be, 0),
>          DEFINE_PROP_END_OF_LIST(),
>      }
>  };
> diff --git a/hw/rtl8139.c b/hw/rtl8139.c
> index 72e2242..ef5f1fd 100644
> --- a/hw/rtl8139.c
> +++ b/hw/rtl8139.c
> @@ -493,7 +493,7 @@ typedef struct RTL8139State {
>      /* PCI interrupt timer */
>      QEMUTimer *timer;
>      int64_t TimerExpire;
> -
> +    uint32_t be;
>  } RTL8139State;
>  
>  static void rtl8139_set_next_tctr_time(RTL8139State *s, int64_t 
> current_time);
> @@ -3123,19 +3123,29 @@ static void rtl8139_mmio_writeb(void *opaque, 
> target_phys_addr_t addr, uint32_t
>      rtl8139_io_writeb(opaque, addr & 0xFF, val);
>  }
>  
> -static void rtl8139_mmio_writew(void *opaque, target_phys_addr_t addr, 
> uint32_t val)
> +static void rtl8139_mmio_writew_be(void *opaque, target_phys_addr_t addr,
> +                                   uint32_t val)
>  {
> -#ifdef TARGET_WORDS_BIGENDIAN
>      val = bswap16(val);
> -#endif
>      rtl8139_io_writew(opaque, addr & 0xFF, val);
>  }
>  
> -static void rtl8139_mmio_writel(void *opaque, target_phys_addr_t addr, 
> uint32_t val)
> +static void rtl8139_mmio_writew_le(void *opaque, target_phys_addr_t addr,
> +                                   uint32_t val)
> +{
> +    rtl8139_io_writew(opaque, addr & 0xFF, val);
> +}
> +
> +static void rtl8139_mmio_writel_be(void *opaque, target_phys_addr_t addr,
> +                                   uint32_t val)
>  {
> -#ifdef TARGET_WORDS_BIGENDIAN
>      val = bswap32(val);
> -#endif
> +    rtl8139_io_writel(opaque, addr & 0xFF, val);
> +}
> +
> +static void rtl8139_mmio_writel_le(void *opaque, target_phys_addr_t addr,
> +                                   uint32_t val)
> +{
>      rtl8139_io_writel(opaque, addr & 0xFF, val);
>  }
>  
> @@ -3144,21 +3154,31 @@ static uint32_t rtl8139_mmio_readb(void *opaque, 
> target_phys_addr_t addr)
>      return rtl8139_io_readb(opaque, addr & 0xFF);
>  }
>  
> -static uint32_t rtl8139_mmio_readw(void *opaque, target_phys_addr_t addr)
> +static uint32_t rtl8139_mmio_readw_be(void *opaque, target_phys_addr_t addr)
>  {
>      uint32_t val = rtl8139_io_readw(opaque, addr & 0xFF);
> -#ifdef TARGET_WORDS_BIGENDIAN
>      val = bswap16(val);
> -#endif
>      return val;
>  }
>  
> -static uint32_t rtl8139_mmio_readl(void *opaque, target_phys_addr_t addr)
> +static uint32_t rtl8139_mmio_readw_le(void *opaque, target_phys_addr_t addr)
> +{
> +    uint32_t val = rtl8139_io_readw(opaque, addr & 0xFF);
> +
> +    return val;
> +}
> +
> +static uint32_t rtl8139_mmio_readl_be(void *opaque, target_phys_addr_t addr)
>  {
>      uint32_t val = rtl8139_io_readl(opaque, addr & 0xFF);
> -#ifdef TARGET_WORDS_BIGENDIAN
>      val = bswap32(val);
> -#endif
> +    return val;
> +}
> +
> +static uint32_t rtl8139_mmio_readl_le(void *opaque, target_phys_addr_t addr)
> +{
> +    uint32_t val = rtl8139_io_readl(opaque, addr & 0xFF);
> +
>      return val;
>  }
>  
> @@ -3292,16 +3312,28 @@ static void rtl8139_ioport_map(PCIDevice *pci_dev, 
> int region_num,
>      register_ioport_read( addr, 0x100, 4, rtl8139_ioport_readl,  s);
>  }
>  
> -static CPUReadMemoryFunc * const rtl8139_mmio_read[3] = {
> +static CPUReadMemoryFunc * const rtl8139_mmio_read_be[3] = {
>      rtl8139_mmio_readb,
> -    rtl8139_mmio_readw,
> -    rtl8139_mmio_readl,
> +    rtl8139_mmio_readw_be,
> +    rtl8139_mmio_readl_be,
>  };
>  
> -static CPUWriteMemoryFunc * const rtl8139_mmio_write[3] = {
> +static CPUWriteMemoryFunc * const rtl8139_mmio_write_be[3] = {
>      rtl8139_mmio_writeb,
> -    rtl8139_mmio_writew,
> -    rtl8139_mmio_writel,
> +    rtl8139_mmio_writew_be,
> +    rtl8139_mmio_writel_be,
> +};
> +
> +static CPUReadMemoryFunc * const rtl8139_mmio_read_le[3] = {
> +    rtl8139_mmio_readb,
> +    rtl8139_mmio_readw_le,
> +    rtl8139_mmio_readl_le,
> +};
> +
> +static CPUWriteMemoryFunc * const rtl8139_mmio_write_le[3] = {
> +    rtl8139_mmio_writeb,
> +    rtl8139_mmio_writew_le,
> +    rtl8139_mmio_writel_le,
>  };
>  
>  static void rtl8139_timer(void *opaque)
> @@ -3369,8 +3401,15 @@ static int pci_rtl8139_init(PCIDevice *dev)
>      pci_conf[PCI_CAPABILITY_LIST] = 0xdc;
>  
>      /* I/O handler for memory-mapped I/O */
> -    s->rtl8139_mmio_io_addr =
> -        cpu_register_io_memory(rtl8139_mmio_read, rtl8139_mmio_write, s);
> +    if (s->be) {
> +        s->rtl8139_mmio_io_addr =
> +            cpu_register_io_memory(rtl8139_mmio_read_be, 
> rtl8139_mmio_write_be,
> +                                   s);
> +    } else {
> +        s->rtl8139_mmio_io_addr =
> +            cpu_register_io_memory(rtl8139_mmio_read_le, 
> rtl8139_mmio_write_le,
> +                                   s);
> +    }
>  
>      pci_register_bar(&s->dev, 0, 0x100,
>                             PCI_BASE_ADDRESS_SPACE_IO,  rtl8139_ioport_map);
> @@ -3404,6 +3443,7 @@ static PCIDeviceInfo rtl8139_info = {
>      .romfile    = "pxe-rtl8139.bin",
>      .qdev.props = (Property[]) {
>          DEFINE_NIC_PROPERTIES(RTL8139State, conf),
> +        DEFINE_PROP_UINT32("be", RTL8139State, be, 0),
>          DEFINE_PROP_END_OF_LIST(),
>      }
>  };
> -- 
> 1.5.6.5
>