diff mbox

[0/5] *** SUBJECT HERE ***

Message ID 1270052612-18351-1-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini March 31, 2010, 4:23 p.m. UTC
On 03/26/2010 08:02 PM, Blue Swirl wrote:
> Comments, anyone?

Sorry I'm late.

I don't really like the changes introduced here, because they make
devices very very tied to the boards.  Hopefully this could be changed
one day with qdev, and patches like this make this task more complicated.
One example is the openpic page size pointed out downthread.

What about something like this (doesn't change all the places affected
by your series, compile-tested only)?

Paolo

Paolo Bonzini (5):
  introduce a wrapper to access target properties without macros
  openpic: use target_get_page_size
  introduce to/from target endianness conversion functions
  openpic: use to/from target endianness conversion functions
  ide/macio: use to/from target endianness conversion functions

 Makefile.target |    2 +-
 arch_init.c     |   26 -----------
 arch_init.h     |    3 -
 bswap.h         |   28 ++++++++++++
 hw/ide/macio.c  |    4 +-
 hw/openpic.c    |   36 ++++++++-------
 target.c        |  132 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 target.h        |    9 ++++
 vl.c            |    1 +
 9 files changed, 192 insertions(+), 49 deletions(-)
 create mode 100644 target.c
 create mode 100644 target.h

From 2e4b169850121bc0209bb3412e42218153642d91 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Wed, 31 Mar 2010 16:00:15 +0200
Subject: [PATCH 1/5] introduce a wrapper to access target properties without macros

I moved also some functions from arch_init.c there.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 Makefile.target |    2 +-
 arch_init.c     |   26 -----------------------
 arch_init.h     |    3 --
 target.c        |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 target.h        |    9 ++++++++
 vl.c            |    1 +
 6 files changed, 73 insertions(+), 30 deletions(-)
 create mode 100644 target.c
 create mode 100644 target.h

Comments

Blue Swirl March 31, 2010, 5:08 p.m. UTC | #1
On 3/31/10, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 03/26/2010 08:02 PM, Blue Swirl wrote:
>  > Comments, anyone?
>
>  Sorry I'm late.
>
>  I don't really like the changes introduced here, because they make
>  devices very very tied to the boards.  Hopefully this could be changed
>  one day with qdev, and patches like this make this task more complicated.

The real solution is to insert a byte swapping bus where needed and
also make devices use some kind of memory API which goes through any
buses (including byte swapping buses) in between the device and
memory.

>  One example is the openpic page size pointed out downthread.
>
>  What about something like this (doesn't change all the places affected
>  by your series, compile-tested only)?

In general (with vmport and maybe virtio as the only exceptions), the
devices have no business knowing _any_ CPU properties, like page size
or endianness. If there really was a device that really cared about
CPU page size, the size should be also known by the board and should
be passed down from there via qdev property. Byte swapping should be
handled by the bus, bus controller or memory controller.

Your patch does not move things to right direction, instead the byte
swapping remains at the device, so I don't think the patch should be
applied.

Thank you for the comments.
Aurelien Jarno March 31, 2010, 7:08 p.m. UTC | #2
On Wed, Mar 31, 2010 at 08:08:52PM +0300, Blue Swirl wrote:
> On 3/31/10, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > On 03/26/2010 08:02 PM, Blue Swirl wrote:
> >  > Comments, anyone?
> >
> >  Sorry I'm late.
> >
> >  I don't really like the changes introduced here, because they make
> >  devices very very tied to the boards.  Hopefully this could be changed
> >  one day with qdev, and patches like this make this task more complicated.
> 
> The real solution is to insert a byte swapping bus where needed and
> also make devices use some kind of memory API which goes through any
> buses (including byte swapping buses) in between the device and
> memory.
> 
> >  One example is the openpic page size pointed out downthread.
> >
> >  What about something like this (doesn't change all the places affected
> >  by your series, compile-tested only)?
> 
> In general (with vmport and maybe virtio as the only exceptions), the
> devices have no business knowing _any_ CPU properties, like page size
> or endianness. If there really was a device that really cared about
> CPU page size, the size should be also known by the board and should
> be passed down from there via qdev property. Byte swapping should be
> handled by the bus, bus controller or memory controller.

If the device don't have to know about any CPU properties, it's probably
better to remove the wrong dependency on the CPU property, instead of
hardcoding values, like for example to 4096 for the page size.

Note also that we are talking about devices that are usually part of the
CPU like openpic, that's why there are really tight with the CPU.
Paul Brook April 1, 2010, 12:48 p.m. UTC | #3
> >  One example is the openpic page size pointed out downthread.
> >
> >  What about something like this (doesn't change all the places affected
> >  by your series, compile-tested only)?
> 
> In general (with vmport and maybe virtio as the only exceptions), the
> devices have no business knowing _any_ CPU properties, like page size
> or endianness. If there really was a device that really cared about
> CPU page size, the size should be also known by the board and should
> be passed down from there via qdev property. Byte swapping should be
> handled by the bus, bus controller or memory controller.

I agree. Most of the uses of TARGET_PAGE_SIZE are simply an optimization - 
it's a convenient buffer size for DMA transfers. Other uses are probably 
incorrect.

None of virtio.c, virtio-{blk,net,serial} or virtio-{pci,syborg} should know 
about CPU properties. Virtio-s390 might because it's implementing magic CPU 
instructions.

Paul
diff mbox

Patch

diff --git a/Makefile.target b/Makefile.target
index dbffe63..48da70c 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -44,7 +44,7 @@  all: $(PROGS)
 
 #########################################################
 # cpu emulator library
-libobj-y = exec.o translate-all.o cpu-exec.o translate.o
+libobj-y = exec.o translate-all.o cpu-exec.o translate.o target.o
 libobj-y += tcg/tcg.o
 libobj-$(CONFIG_SOFTFLOAT) += fpu/softfloat.o
 libobj-$(CONFIG_NOSOFTFLOAT) += fpu/softfloat-native.o
diff --git a/arch_init.c b/arch_init.c
index cfc03ea..9e5150f 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -482,29 +482,3 @@  void cpudef_init(void)
 #endif
 }
 
-int audio_available(void)
-{
-#ifdef HAS_AUDIO
-    return 1;
-#else
-    return 0;
-#endif
-}
-
-int kvm_available(void)
-{
-#ifdef CONFIG_KVM
-    return 1;
-#else
-    return 0;
-#endif
-}
-
-int xen_available(void)
-{
-#ifdef CONFIG_XEN
-    return 1;
-#else
-    return 0;
-#endif
-}
diff --git a/arch_init.h b/arch_init.h
index 682890c..a2bb31c 100644
--- a/arch_init.h
+++ b/arch_init.h
@@ -26,8 +26,5 @@  int ram_load(QEMUFile *f, void *opaque, int version_id);
 void do_acpitable_option(const char *optarg);
 void do_smbios_option(const char *optarg);
 void cpudef_init(void);
-int audio_available(void);
-int kvm_available(void);
-int xen_available(void);
 
 #endif
diff --git a/target.c b/target.c
new file mode 100644
index 0000000..fd8e3c6
--- /dev/null
+++ b/target.c
@@ -0,0 +1,62 @@ 
+/*
+ * Access to target properties
+ *
+ * Copyright (c) 2010 Paolo Bonzini
+ *
+ * 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 "config.h"
+#include "qemu-common.h"
+#include "cpu.h"
+#include "target.h"
+
+unsigned long
+get_target_page_size (void)
+{
+    return TARGET_PAGE_SIZE;
+}
+
+int audio_available(void)
+{
+#if defined HAS_AUDIO
+    return 1;
+#else
+    return 0;
+#endif
+}
+
+int kvm_available(void)
+{
+#ifdef CONFIG_KVM
+    return 1;
+#else
+    return 0;
+#endif
+}
+
+int xen_available(void)
+{
+#ifdef CONFIG_XEN
+    return 1;
+#else
+    return 0;
+#endif
+}
+
diff --git a/target.h b/target.h
new file mode 100644
index 0000000..05e1543
--- /dev/null
+++ b/target.h
@@ -0,0 +1,9 @@ 
+#ifndef TARGET_H
+#define TARGET_H
+
+unsigned long get_target_page_size(void);
+int audio_available(void);
+int kvm_available(void);
+int xen_available(void);
+
+#endif
diff --git a/vl.c b/vl.c
index 6768cf1..855da20 100644
--- a/vl.c
+++ b/vl.c
@@ -160,6 +160,7 @@  int main(int argc, char **argv)
 #include "qemu-queue.h"
 #include "cpus.h"
 #include "arch_init.h"
+#include "target.h"
 
 //#define DEBUG_NET
 //#define DEBUG_SLIRP
-- 
1.6.6.1


From 415afbdec10b06aac4fea216aa13703e156d1ed7 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Wed, 31 Mar 2010 16:09:03 +0200
Subject: [PATCH 2/5] openpic: use target_get_page_size

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/openpic.c |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/hw/openpic.c b/hw/openpic.c
index ac21993..379e7ea 100644
--- a/hw/openpic.c
+++ b/hw/openpic.c
@@ -36,6 +36,7 @@ 
 #include "ppc_mac.h"
 #include "pci.h"
 #include "openpic.h"
+#include "target.h"
 
 //#define DEBUG_OPENPIC
 
@@ -141,8 +142,6 @@  enum mpic_ide_bits {
 #error "Please select which OpenPic implementation is to be emulated"
 #endif
 
-#define OPENPIC_PAGE_SIZE 4096
-
 #define BF_WIDTH(_bits_) \
 (((_bits_) + (sizeof(uint32_t) * 8) - 1) / (sizeof(uint32_t) * 8))
 
@@ -243,6 +242,7 @@  typedef struct openpic_t {
     int irq_ipi0;
     int irq_tim0;
     int need_swap;
+    unsigned long page_size;
     void (*reset) (void *);
     void (*irq_raise) (struct openpic_t *, int, IRQ_src_t *);
 } openpic_t;
@@ -1233,6 +1233,7 @@  qemu_irq *openpic_init (PCIBus *bus, int *pmem_index, int nb_cpus,
         opp->dst[i].irqs = irqs[i];
     opp->irq_out = irq_out;
     opp->need_swap = 1;
+    opp->page_size = get_target_page_size();
 
     register_savevm("openpic", 0, 2, openpic_save, openpic_load, opp);
     qemu_register_reset(openpic_reset, opp);
@@ -1370,7 +1371,7 @@  static void mpic_src_ext_write (void *opaque, target_phys_addr_t addr,
     if (addr & 0xF)
         return;
 
-    addr -= MPIC_EXT_REG_START & (OPENPIC_PAGE_SIZE - 1);
+    addr -= MPIC_EXT_REG_START & (mpp->page_size - 1);
     if (addr < MPIC_EXT_REG_SIZE) {
         idx += (addr & 0xFFF0) >> 5;
         if (addr & 0x10) {
@@ -1394,7 +1395,7 @@  static uint32_t mpic_src_ext_read (void *opaque, target_phys_addr_t addr)
     if (addr & 0xF)
         return retval;
 
-    addr -= MPIC_EXT_REG_START & (OPENPIC_PAGE_SIZE - 1);
+    addr -= MPIC_EXT_REG_START & (mpp->page_size - 1);
     if (addr < MPIC_EXT_REG_SIZE) {
         idx += (addr & 0xFFF0) >> 5;
         if (addr & 0x10) {
@@ -1420,7 +1421,7 @@  static void mpic_src_int_write (void *opaque, target_phys_addr_t addr,
     if (addr & 0xF)
         return;
 
-    addr -= MPIC_INT_REG_START & (OPENPIC_PAGE_SIZE - 1);
+    addr -= MPIC_INT_REG_START & (mpp->page_size - 1);
     if (addr < MPIC_INT_REG_SIZE) {
         idx += (addr & 0xFFF0) >> 5;
         if (addr & 0x10) {
@@ -1444,7 +1445,7 @@  static uint32_t mpic_src_int_read (void *opaque, target_phys_addr_t addr)
     if (addr & 0xF)
         return retval;
 
-    addr -= MPIC_INT_REG_START & (OPENPIC_PAGE_SIZE - 1);
+    addr -= MPIC_INT_REG_START & (mpp->page_size - 1);
     if (addr < MPIC_INT_REG_SIZE) {
         idx += (addr & 0xFFF0) >> 5;
         if (addr & 0x10) {
@@ -1470,7 +1471,7 @@  static void mpic_src_msg_write (void *opaque, target_phys_addr_t addr,
     if (addr & 0xF)
         return;
 
-    addr -= MPIC_MSG_REG_START & (OPENPIC_PAGE_SIZE - 1);
+    addr -= MPIC_MSG_REG_START & (mpp->page_size - 1);
     if (addr < MPIC_MSG_REG_SIZE) {
         idx += (addr & 0xFFF0) >> 5;
         if (addr & 0x10) {
@@ -1494,7 +1495,7 @@  static uint32_t mpic_src_msg_read (void *opaque, target_phys_addr_t addr)
     if (addr & 0xF)
         return retval;
 
-    addr -= MPIC_MSG_REG_START & (OPENPIC_PAGE_SIZE - 1);
+    addr -= MPIC_MSG_REG_START & (mpp->page_size - 1);
     if (addr < MPIC_MSG_REG_SIZE) {
         idx += (addr & 0xFFF0) >> 5;
         if (addr & 0x10) {
@@ -1520,7 +1521,7 @@  static void mpic_src_msi_write (void *opaque, target_phys_addr_t addr,
     if (addr & 0xF)
         return;
 
-    addr -= MPIC_MSI_REG_START & (OPENPIC_PAGE_SIZE - 1);
+    addr -= MPIC_MSI_REG_START & (mpp->page_size - 1);
     if (addr < MPIC_MSI_REG_SIZE) {
         idx += (addr & 0xFFF0) >> 5;
         if (addr & 0x10) {
@@ -1543,7 +1544,7 @@  static uint32_t mpic_src_msi_read (void *opaque, target_phys_addr_t addr)
     if (addr & 0xF)
         return retval;
 
-    addr -= MPIC_MSI_REG_START & (OPENPIC_PAGE_SIZE - 1);
+    addr -= MPIC_MSI_REG_START & (mpp->page_size - 1);
     if (addr < MPIC_MSI_REG_SIZE) {
         idx += (addr & 0xFFF0) >> 5;
         if (addr & 0x10) {
@@ -1688,6 +1689,7 @@  qemu_irq *mpic_init (target_phys_addr_t base, int nb_cpus,
         mpp->dst[i].irqs = irqs[i];
     mpp->irq_out = irq_out;
     mpp->need_swap = 0;    /* MPIC has the same endian as target */
+    mpp->page_size = get_target_page_size();
 
     mpp->irq_raise = mpic_irq_raise;
     mpp->reset = mpic_reset;
-- 
1.6.6.1


From 329e0cdf6dc67f6334fda3f9099c70e032dae759 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Wed, 31 Mar 2010 15:55:55 +0200
Subject: [PATCH 3/5] introduce to/from target endianness conversion functions

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 bswap.h  |   28 ++++++++++++++++++++++++
 target.c |   70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+), 0 deletions(-)

diff --git a/bswap.h b/bswap.h
index aace9b7..602e630 100644
--- a/bswap.h
+++ b/bswap.h
@@ -220,4 +220,32 @@  static inline uint32_t qemu_bswap_len(uint32_t value, int len)
     return bswap32(value) >> (32 - 8 * len);
 }
 
+
+/* conversions to target format.  */
+extern uint16_t be_to_target16(uint16_t);
+extern uint32_t be_to_target32(uint32_t);
+extern uint64_t be_to_target64(uint64_t);
+extern uint16_t le_to_target16(uint16_t);
+extern uint32_t le_to_target32(uint32_t);
+extern uint64_t le_to_target64(uint64_t);
+extern void be_to_target16s(uint16_t *);
+extern void be_to_target32s(uint32_t *);
+extern void be_to_target64s(uint64_t *);
+extern void le_to_target16s(uint16_t *);
+extern void le_to_target32s(uint32_t *);
+extern void le_to_target64s(uint64_t *);
+
+#define target_to_be16 be_to_target16
+#define target_to_be32 be_to_target32
+#define target_to_be64 be_to_target64
+#define target_to_le16 le_to_target16
+#define target_to_le32 le_to_target32
+#define target_to_le64 le_to_target64
+#define target_to_be16s be_to_target16s
+#define target_to_be32s be_to_target32s
+#define target_to_be64s be_to_target64s
+#define target_to_le16s le_to_target16s
+#define target_to_le32s le_to_target32s
+#define target_to_le64s le_to_target64s
+
 #endif /* BSWAP_H */
diff --git a/target.c b/target.c
index fd8e3c6..9db8b86 100644
--- a/target.c
+++ b/target.c
@@ -26,6 +26,7 @@ 
 #include "qemu-common.h"
 #include "cpu.h"
 #include "target.h"
+#include "bswap.h"
 
 unsigned long
 get_target_page_size (void)
@@ -33,6 +34,75 @@  get_target_page_size (void)
     return TARGET_PAGE_SIZE;
 }
 
+#ifdef TARGET_WORDS_BIGENDIAN
+#define neutral(size)	be_to_target##size
+#define swapped(size)	le_to_target##size
+#define neutral_s(size)	be_to_target##size##s
+#define swapped_s(size)	le_to_target##size##s
+#else
+#define neutral(size)	le_to_target##size
+#define swapped(size)	be_to_target##size
+#define neutral_s(size)	le_to_target##size##s
+#define swapped_s(size)	be_to_target##size##s
+#endif
+
+uint16_t neutral(16) (uint16_t x)
+{
+    return x;
+} 
+
+uint32_t neutral(32) (uint32_t x)
+{
+    return x;
+} 
+
+uint64_t neutral(64) (uint64_t x)
+{
+    return x;
+} 
+
+void neutral_s(16) (uint16_t *x)
+{
+} 
+
+void neutral_s(32) (uint32_t *x)
+{
+} 
+
+void neutral_s(64) (uint64_t *x)
+{
+} 
+
+uint16_t swapped(16) (uint16_t x)
+{
+    return bswap16(x);
+} 
+
+uint32_t swapped(32) (uint32_t x)
+{
+    return bswap32(x);
+} 
+
+uint64_t swapped(64) (uint64_t x)
+{
+    return bswap64(x);
+} 
+
+void swapped_s(16) (uint16_t *x)
+{
+    bswap16s(x);
+} 
+
+void swapped_s(32) (uint32_t *x)
+{
+    bswap32s(x);
+} 
+
+void swapped_s(64) (uint64_t *x)
+{
+    bswap64s(x);
+} 
+
 int audio_available(void)
 {
 #if defined HAS_AUDIO
-- 
1.6.6.1


From 8a7a1e0cc770d3f2bf113475a91fc6a3772735c3 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Wed, 31 Mar 2010 15:56:13 +0200
Subject: [PATCH 4/5] openpic: use to/from target endianness conversion functions

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/openpic.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/openpic.c b/hw/openpic.c
index 379e7ea..ebe9f5e 100644
--- a/hw/openpic.c
+++ b/hw/openpic.c
@@ -241,7 +241,7 @@  typedef struct openpic_t {
     int max_irq;
     int irq_ipi0;
     int irq_tim0;
-    int need_swap;
+    int big_endian;
     unsigned long page_size;
     void (*reset) (void *);
     void (*irq_raise) (struct openpic_t *, int, IRQ_src_t *);
@@ -249,10 +249,10 @@  typedef struct openpic_t {
 
 static inline uint32_t openpic_swap32(openpic_t *opp, uint32_t val)
 {
-    if (opp->need_swap)
-        return bswap32(val);
-
-    return val;
+    if (opp->big_endian)
+        return be_to_target32(val);
+    else
+        return le_to_target32(val);
 }
 
 static inline void IRQ_setbit (IRQ_queue_t *q, int n_IRQ)
@@ -1232,7 +1232,7 @@  qemu_irq *openpic_init (PCIBus *bus, int *pmem_index, int nb_cpus,
     for (i = 0; i < nb_cpus; i++)
         opp->dst[i].irqs = irqs[i];
     opp->irq_out = irq_out;
-    opp->need_swap = 1;
+    opp->big_endian = 0;
     opp->page_size = get_target_page_size();
 
     register_savevm("openpic", 0, 2, openpic_save, openpic_load, opp);
@@ -1688,7 +1688,7 @@  qemu_irq *mpic_init (target_phys_addr_t base, int nb_cpus,
     for (i = 0; i < nb_cpus; i++)
         mpp->dst[i].irqs = irqs[i];
     mpp->irq_out = irq_out;
-    mpp->need_swap = 0;    /* MPIC has the same endian as target */
+    mpp->big_endian = 1;
     mpp->page_size = get_target_page_size();
 
     mpp->irq_raise = mpic_irq_raise;
-- 
1.6.6.1


From ddf213677cfc6f1475c2ef357751ba91fdd37a42 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Wed, 31 Mar 2010 16:18:10 +0200
Subject: [PATCH 5/5] ide/macio: use to/from target endianness conversion functions

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/macio.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 639f3f6..e9a70bd 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -225,7 +225,7 @@  static void pmac_ide_writew (void *opaque,
     MACIOIDEState *d = opaque;
 
     addr = (addr & 0xFFF) >> 4;
-    val = bswap16(val);
+    val = le_to_target16(val);
     if (addr == 0) {
         ide_data_writew(&d->bus, 0, val);
     }
@@ -242,7 +242,7 @@  static uint32_t pmac_ide_readw (void *opaque,target_phys_addr_t addr)
     } else {
         retval = 0xFFFF;
     }
-    retval = bswap16(retval);
+    retval = le_to_target16(retval);
     return retval;
 }