Patchwork Re: Compile files only once: some planning

login
register
mail settings
Submitter Blue Swirl
Date March 24, 2010, 8:05 p.m.
Message ID <f43fc5581003241305h2947ea46o158343b6ceb80297@mail.gmail.com>
Download mbox | patch
Permalink /patch/48468/
State New
Headers show

Comments

Blue Swirl - March 24, 2010, 8:05 p.m.
On 3/24/10, Michael S. Tsirkin <mst@redhat.com> 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.

>  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.
Michael S. Tsirkin - March 24, 2010, 8:08 p.m.
On Wed, Mar 24, 2010 at 10:05:10PM +0200, Blue Swirl wrote:
> On 3/24/10, Michael S. Tsirkin <mst@redhat.com> 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 <blauwirbel@gmail.com>
> Date: Wed, 24 Mar 2010 19:54:05 +0000
> Subject: [PATCH] Compile rtl8139 and e1000 only once
> 
> WIP
> 
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>  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
>
Blue Swirl - March 24, 2010, 8:24 p.m.
On 3/24/10, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Mar 24, 2010 at 10:05:10PM +0200, Blue Swirl wrote:
>  > On 3/24/10, Michael S. Tsirkin <mst@redhat.com> 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 <blauwirbel@gmail.com>
>  > Date: Wed, 24 Mar 2010 19:54:05 +0000
>  > Subject: [PATCH] Compile rtl8139 and e1000 only once
>  >
>  > WIP
>  >
>  > Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>  > ---
>  >  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
>  >
>
>
Michael S. Tsirkin - March 24, 2010, 8:24 p.m.
On Wed, Mar 24, 2010 at 10:24:12PM +0200, Blue Swirl wrote:
> On 3/24/10, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Wed, Mar 24, 2010 at 10:05:10PM +0200, Blue Swirl wrote:
> >  > On 3/24/10, Michael S. Tsirkin <mst@redhat.com> 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.

Presumably it's there for a reason?

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

How about just making it 64 bit unconditionally?
How much do we save by using a 32 bit address value?
Anthony Liguori - March 24, 2010, 8:28 p.m.
On 03/24/2010 03:24 PM, Blue Swirl wrote:
> On 3/24/10, Michael S. Tsirkin<mst@redhat.com>  wrote:
>    
>> On Wed, Mar 24, 2010 at 10:05:10PM +0200, Blue Swirl wrote:
>>   >  On 3/24/10, Michael S. Tsirkin<mst@redhat.com>  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.
>    

My previous pci memory functions patches removed the byte swapping.

The problem is that PCI devices are going to operate in little endian 
mode (usually) whereas the CPU may be acting in big endian mode.  We 
need to do a byte swap somewhere but the better place to do it is in the 
PCI bus layer.

Regards,

Anthony Liguori
Blue Swirl - March 24, 2010, 8:32 p.m.
On 3/24/10, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Mar 24, 2010 at 10:24:12PM +0200, Blue Swirl wrote:
>  > On 3/24/10, Michael S. Tsirkin <mst@redhat.com> wrote:
>  > > On Wed, Mar 24, 2010 at 10:05:10PM +0200, Blue Swirl wrote:
>  > >  > On 3/24/10, Michael S. Tsirkin <mst@redhat.com> 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.
>
>
> Presumably it's there for a reason?
>
>
>  > >  > >  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.
>
>
> How about just making it 64 bit unconditionally?
>  How much do we save by using a 32 bit address value?

On a 32 bit host, probably a lot because of register pressure. And
it's not too much effort to keep the target_phys_addr_t type logic.
Aurelien Jarno - March 24, 2010, 9 p.m.
On Wed, Mar 24, 2010 at 10:24:12PM +0200, Blue Swirl wrote:
> On 3/24/10, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Wed, Mar 24, 2010 at 10:05:10PM +0200, Blue Swirl wrote:
> >  > On 3/24/10, Michael S. Tsirkin <mst@redhat.com> 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.
> 

The real fix is actually to add a layer handling bus byte swapping
depending on how bus are connected.

Currently it only works because all big endian boards QEMU emulates
need to byteswap bus access, and none of the little endian boards 
need to do that.
Paul Brook - March 24, 2010, 10:33 p.m.
> 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.

It should not be a property of the device. All devices have a native 
endianness (for PCI this is little-endian), and the intermediate 
busses/interconnects should determine whether byteswapping occurs.

Paul
Anthony Liguori - March 24, 2010, 10:50 p.m.
On 03/24/2010 05:33 PM, Paul Brook wrote:
>> 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.
>>      
> It should not be a property of the device. All devices have a native
> endianness (for PCI this is little-endian), and the intermediate
> busses/interconnects should determine whether byteswapping occurs.
>    

Right, the byte swapping needs to happen at the bus level which requires 
that the PCI regions use a different callback mechanism (and don't 
register directly with the cpu).

Regards,

Anthony Liguori

> Paul
>
>
>
Paul Brook - March 24, 2010, 11:05 p.m.
> On 03/24/2010 05:33 PM, Paul Brook wrote:
> >> 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.
> >
> > It should not be a property of the device. All devices have a native
> > endianness (for PCI this is little-endian), and the intermediate
> > busses/interconnects should determine whether byteswapping occurs.
> 
> Right, the byte swapping needs to happen at the bus level which requires
> that the PCI regions use a different callback mechanism (and don't
> register directly with the cpu).

Not necessarily a different callback mechanism, but probably a different 
registration mechanism.

Paul
Anthony Liguori - March 24, 2010, 11:07 p.m.
On 03/24/2010 06:05 PM, Paul Brook wrote:
>> On 03/24/2010 05:33 PM, Paul Brook wrote:
>>      
>>>> 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.
>>>>          
>>> It should not be a property of the device. All devices have a native
>>> endianness (for PCI this is little-endian), and the intermediate
>>> busses/interconnects should determine whether byteswapping occurs.
>>>        
>> Right, the byte swapping needs to happen at the bus level which requires
>> that the PCI regions use a different callback mechanism (and don't
>> register directly with the cpu).
>>      
> Not necessarily a different callback mechanism, but probably a different
> registration mechanism.
>    

Problem with the current scheme is that it's tied to 
target_phys_addr_t.  It's not a target_phys_addr_t and cannot be used 
with functions that take target_phys_addr_ts.

We can either introduce a generic address type or we can introduce bus 
specific addresses and have different callbacks.  The later has better 
type safety and since this isn't an obvious issue to most developers, 
that's probably an important feature.

Regards,

Anthony Liguori

> Paul
>
Michael S. Tsirkin - March 25, 2010, 8:21 a.m.
On Wed, Mar 24, 2010 at 06:07:35PM -0500, Anthony Liguori wrote:
> On 03/24/2010 06:05 PM, Paul Brook wrote:
>>> On 03/24/2010 05:33 PM, Paul Brook wrote:
>>>      
>>>>> 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.
>>>>>          
>>>> It should not be a property of the device. All devices have a native
>>>> endianness (for PCI this is little-endian), and the intermediate
>>>> busses/interconnects should determine whether byteswapping occurs.
>>>>        
>>> Right, the byte swapping needs to happen at the bus level which requires
>>> that the PCI regions use a different callback mechanism (and don't
>>> register directly with the cpu).
>>>      
>> Not necessarily a different callback mechanism, but probably a different
>> registration mechanism.
>>    
>
> Problem with the current scheme is that it's tied to target_phys_addr_t.  
> It's not a target_phys_addr_t and cannot be used with functions that take 
> target_phys_addr_ts.
>
> We can either introduce a generic address type or we can introduce bus  
> specific addresses and have different callbacks.  The later has better  
> type safety and since this isn't an obvious issue to most developers,  
> that's probably an important feature.
>
> Regards,
>
> Anthony Liguori
>
>> Paul
>>    


I'd prefer a generic address type since this makes it easier to
share code: for example virtio devices can use different busses,
it's common for pci host bridges to have common code for i/o
and memory, etc.
Paul Brook - March 25, 2010, 12:01 p.m.
> I'd prefer a generic address type since this makes it easier to
> share code: for example virtio devices can use different busses,

This is exactly why virtio devices should not be accessing memory. They should 
be doing everything via a virtqueue, which can interface with the host 
bindings appropriately.

Paul

Patch

From e0ab5cc41c68207be558ccb330f4fb83fba4ee6f Mon Sep 17 00:00:00 2001
From: Blue Swirl <blauwirbel@gmail.com>
Date: Wed, 24 Mar 2010 19:54:05 +0000
Subject: [PATCH] Compile rtl8139 and e1000 only once

WIP

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 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