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

[Blue Swirl]

On 3/24/10, Michael S. Tsirkin <address@hidden> wrote:
> 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.

It shouldn't. Maybe the real fix is to remove the byte swapping.

>  > >  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?

target_phys_addr_t is 64 bit on a 64 bit host, on a 32 bit host it's
size will be either 64 or 32 bits depending on the target. So the
files are compiled once on 64 bit host, twice on 32 bit host if both
32 and 64 bits targets are selected.

>  > 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
>  >
>
>