[1/2] target-i386/seg_helper: refactor 4 helper functions

Message ID	1366602253-15974-1-git-send-email-lig.fnst@cn.fujitsu.com
State	New
Headers	show Return-Path: <qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org> From: liguang <lig.fnst@cn.fujitsu.com> To: qemu-devel@nongnu.org Date: Mon, 22 Apr 2013 11:44:12 +0800 Message-Id: <1366602253-15974-1-git-send-email-lig.fnst@cn.fujitsu.com> Cc: blauwirbel@gmail.com, pbonzini@redhat.com, afaerber@suse.de, lig.fnst@cn.fujitsu.com Subject: [Qemu-devel] [PATCH 1/2] target-i386/seg_helper: refactor 4 helper functions Precedence: list Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org

liguang April 22, 2013, 3:44 a.m. UTC

for helper_{lsl, lar, verr, verw}, there are
common parts, so move them outside, and then
call this new helper-helper function.

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 target-i386/seg_helper.c |  179 ++++++++++++++-------------------------------
 1 files changed, 56 insertions(+), 123 deletions(-)

liguang April 30, 2013, 3:21 a.m. UTC | #1

ping ...

在 2013-04-22一的 11:44 +0800，liguang写道：
> for helper_{lsl, lar, verr, verw}, there are
> common parts, so move them outside, and then
> call this new helper-helper function.
> 
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
>  target-i386/seg_helper.c |  179 ++++++++++++++-------------------------------
>  1 files changed, 56 insertions(+), 123 deletions(-)
> 
> diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
> index 906e4f3..419efd8 100644
> --- a/target-i386/seg_helper.c
> +++ b/target-i386/seg_helper.c
> @@ -2292,9 +2292,10 @@ void helper_sysexit(CPUX86State *env, int dflag)
>      EIP = EDX;
>  }
>  
> -target_ulong helper_lsl(CPUX86State *env, target_ulong selector1)
> +
> +static target_ulong misc_check_helper(CPUX86State *env, target_ulong selector1,
> +                                      int inst)
>  {
> -    unsigned int limit;
>      uint32_t e1, e2, eflags, selector;
>      int rpl, dpl, cpl, type;
>  
> @@ -2306,14 +2307,30 @@ target_ulong helper_lsl(CPUX86State *env, target_ulong selector1)
>      if (load_segment(env, &e1, &e2, selector) != 0) {
>          goto fail;
>      }
> +
> +    CC_SRC = eflags & ~CC_Z;
> +
>      rpl = selector & 3;
>      dpl = (e2 >> DESC_DPL_SHIFT) & 3;
>      cpl = env->hflags & HF_CPL_MASK;
> +
>      if (e2 & DESC_S_MASK) {
> -        if ((e2 & DESC_CS_MASK) && (e2 & DESC_C_MASK)) {
> -            /* conforming */
> -        } else {
> -            if (dpl < cpl || dpl < rpl) {
> +        if (e2 & DESC_CS_MASK) {
> +            switch (inst) {
> +            case 1:
> +                goto fail;
> +            case 2:
> +                if (!(e2 & (DESC_R_MASK | DESC_C_MASK))) {
> +                    goto fail;
> +                }
> +                break;
> +            case 3:
> +            case 4:
> +                if (!(e2 & DESC_C_MASK)) {
> +                    goto check_pl;
> +                }
> +                break;
> +            default:
>                  goto fail;
>              }
>          }
> @@ -2325,140 +2342,56 @@ target_ulong helper_lsl(CPUX86State *env, target_ulong selector1)
>          case 3:
>          case 9:
>          case 11:
> -            break;
> +            if (inst == 3) {
> +                break;
> +            }
> +        case 5:
> +        case 12:
> +            if (inst == 4) {
> +                break;
> +            }
>          default:
>              goto fail;
>          }
> -        if (dpl < cpl || dpl < rpl) {
> -        fail:
> -            CC_SRC = eflags & ~CC_Z;
> -            return 0;
> -        }
> +        goto check_pl;
> +    }
> +
> +    if (inst == 3) {
> +        e2 &= 0x00f0ff00;
>      }
> -    limit = get_seg_limit(e1, e2);
> +    if (inst == 4) {
> +        e2 = get_seg_limit(e1, e2);
> +    }
> +
>      CC_SRC = eflags | CC_Z;
> -    return limit;
> +
> +check_pl:
> +    if (dpl < cpl || dpl < rpl) {
> +        goto fail;
> +    }
> +
> +fail:
> +    return e2;
>  }
>  
> -target_ulong helper_lar(CPUX86State *env, target_ulong selector1)
> +target_ulong helper_lsl(CPUX86State *env, target_ulong selector1)
>  {
> -    uint32_t e1, e2, eflags, selector;
> -    int rpl, dpl, cpl, type;
> +    return misc_check_helper(env, selector1, 4);
> +}
>  
> -    selector = selector1 & 0xffff;
> -    eflags = cpu_cc_compute_all(env, CC_OP);
> -    if ((selector & 0xfffc) == 0) {
> -        goto fail;
> -    }
> -    if (load_segment(env, &e1, &e2, selector) != 0) {
> -        goto fail;
> -    }
> -    rpl = selector & 3;
> -    dpl = (e2 >> DESC_DPL_SHIFT) & 3;
> -    cpl = env->hflags & HF_CPL_MASK;
> -    if (e2 & DESC_S_MASK) {
> -        if ((e2 & DESC_CS_MASK) && (e2 & DESC_C_MASK)) {
> -            /* conforming */
> -        } else {
> -            if (dpl < cpl || dpl < rpl) {
> -                goto fail;
> -            }
> -        }
> -    } else {
> -        type = (e2 >> DESC_TYPE_SHIFT) & 0xf;
> -        switch (type) {
> -        case 1:
> -        case 2:
> -        case 3:
> -        case 4:
> -        case 5:
> -        case 9:
> -        case 11:
> -        case 12:
> -            break;
> -        default:
> -            goto fail;
> -        }
> -        if (dpl < cpl || dpl < rpl) {
> -        fail:
> -            CC_SRC = eflags & ~CC_Z;
> -            return 0;
> -        }
> -    }
> -    CC_SRC = eflags | CC_Z;
> -    return e2 & 0x00f0ff00;
> +target_ulong helper_lar(CPUX86State *env, target_ulong selector1)
> +{
> +    return misc_check_helper(env, selector1, 3);
>  }
>  
>  void helper_verr(CPUX86State *env, target_ulong selector1)
>  {
> -    uint32_t e1, e2, eflags, selector;
> -    int rpl, dpl, cpl;
> -
> -    selector = selector1 & 0xffff;
> -    eflags = cpu_cc_compute_all(env, CC_OP);
> -    if ((selector & 0xfffc) == 0) {
> -        goto fail;
> -    }
> -    if (load_segment(env, &e1, &e2, selector) != 0) {
> -        goto fail;
> -    }
> -    if (!(e2 & DESC_S_MASK)) {
> -        goto fail;
> -    }
> -    rpl = selector & 3;
> -    dpl = (e2 >> DESC_DPL_SHIFT) & 3;
> -    cpl = env->hflags & HF_CPL_MASK;
> -    if (e2 & DESC_CS_MASK) {
> -        if (!(e2 & DESC_R_MASK)) {
> -            goto fail;
> -        }
> -        if (!(e2 & DESC_C_MASK)) {
> -            if (dpl < cpl || dpl < rpl) {
> -                goto fail;
> -            }
> -        }
> -    } else {
> -        if (dpl < cpl || dpl < rpl) {
> -        fail:
> -            CC_SRC = eflags & ~CC_Z;
> -            return;
> -        }
> -    }
> -    CC_SRC = eflags | CC_Z;
> +    misc_check_helper(env, selector1, 2);
>  }
>  
>  void helper_verw(CPUX86State *env, target_ulong selector1)
>  {
> -    uint32_t e1, e2, eflags, selector;
> -    int rpl, dpl, cpl;
> -
> -    selector = selector1 & 0xffff;
> -    eflags = cpu_cc_compute_all(env, CC_OP);
> -    if ((selector & 0xfffc) == 0) {
> -        goto fail;
> -    }
> -    if (load_segment(env, &e1, &e2, selector) != 0) {
> -        goto fail;
> -    }
> -    if (!(e2 & DESC_S_MASK)) {
> -        goto fail;
> -    }
> -    rpl = selector & 3;
> -    dpl = (e2 >> DESC_DPL_SHIFT) & 3;
> -    cpl = env->hflags & HF_CPL_MASK;
> -    if (e2 & DESC_CS_MASK) {
> -        goto fail;
> -    } else {
> -        if (dpl < cpl || dpl < rpl) {
> -            goto fail;
> -        }
> -        if (!(e2 & DESC_W_MASK)) {
> -        fail:
> -            CC_SRC = eflags & ~CC_Z;
> -            return;
> -        }
> -    }
> -    CC_SRC = eflags | CC_Z;
> +    misc_check_helper(env, selector1, 1);
>  }
>  
>  #if defined(CONFIG_USER_ONLY)

Peter Maydell April 30, 2013, 7:16 a.m. UTC | #2

On 22 April 2013 04:44, liguang <lig.fnst@cn.fujitsu.com> wrote:
> for helper_{lsl, lar, verr, verw}, there are
> common parts, so move them outside, and then
> call this new helper-helper function.
>
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
>  target-i386/seg_helper.c |  179 ++++++++++++++-------------------------------
>  1 files changed, 56 insertions(+), 123 deletions(-)
>
> diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
> index 906e4f3..419efd8 100644
> --- a/target-i386/seg_helper.c
> +++ b/target-i386/seg_helper.c
> @@ -2292,9 +2292,10 @@ void helper_sysexit(CPUX86State *env, int dflag)
>      EIP = EDX;
>  }
>
> -target_ulong helper_lsl(CPUX86State *env, target_ulong selector1)
> +
> +static target_ulong misc_check_helper(CPUX86State *env, target_ulong selector1,
> +                                      int inst)

This function really needs a better (more specific) name...

thanks
-- PMM

liguang April 30, 2013, 7:27 a.m. UTC | #3

在 2013-04-30二的 08:16 +0100，Peter Maydell写道：
> On 22 April 2013 04:44, liguang <lig.fnst@cn.fujitsu.com> wrote:
> > for helper_{lsl, lar, verr, verw}, there are
> > common parts, so move them outside, and then
> > call this new helper-helper function.
> >
> > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> > ---
> >  target-i386/seg_helper.c |  179 ++++++++++++++-------------------------------
> >  1 files changed, 56 insertions(+), 123 deletions(-)
> >
> > diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
> > index 906e4f3..419efd8 100644
> > --- a/target-i386/seg_helper.c
> > +++ b/target-i386/seg_helper.c
> > @@ -2292,9 +2292,10 @@ void helper_sysexit(CPUX86State *env, int dflag)
> >      EIP = EDX;
> >  }
> >
> > -target_ulong helper_lsl(CPUX86State *env, target_ulong selector1)
> > +
> > +static target_ulong misc_check_helper(CPUX86State *env, target_ulong selector1,
> > +                                      int inst)
> 
> This function really needs a better (more specific) name...
> 

Sorry, I'm hesitating to ask what will be the proper name?
this function is just a helper of other 4 functions,
seems its function can hardly be described by a name.

liguang May 23, 2013, 8:35 a.m. UTC | #4

ping ... again.

在 2013-04-22一的 11:44 +0800，liguang写道：
> for helper_{lsl, lar, verr, verw}, there are
> common parts, so move them outside, and then
> call this new helper-helper function.
> 
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
>  target-i386/seg_helper.c |  179 ++++++++++++++-------------------------------
>  1 files changed, 56 insertions(+), 123 deletions(-)
> 
> diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
> index 906e4f3..419efd8 100644
> --- a/target-i386/seg_helper.c
> +++ b/target-i386/seg_helper.c
> @@ -2292,9 +2292,10 @@ void helper_sysexit(CPUX86State *env, int dflag)
>      EIP = EDX;
>  }
>  
> -target_ulong helper_lsl(CPUX86State *env, target_ulong selector1)
> +
> +static target_ulong misc_check_helper(CPUX86State *env, target_ulong selector1,
> +                                      int inst)
>  {
> -    unsigned int limit;
>      uint32_t e1, e2, eflags, selector;
>      int rpl, dpl, cpl, type;
>  
> @@ -2306,14 +2307,30 @@ target_ulong helper_lsl(CPUX86State *env, target_ulong selector1)
>      if (load_segment(env, &e1, &e2, selector) != 0) {
>          goto fail;
>      }
> +
> +    CC_SRC = eflags & ~CC_Z;
> +
>      rpl = selector & 3;
>      dpl = (e2 >> DESC_DPL_SHIFT) & 3;
>      cpl = env->hflags & HF_CPL_MASK;
> +
>      if (e2 & DESC_S_MASK) {
> -        if ((e2 & DESC_CS_MASK) && (e2 & DESC_C_MASK)) {
> -            /* conforming */
> -        } else {
> -            if (dpl < cpl || dpl < rpl) {
> +        if (e2 & DESC_CS_MASK) {
> +            switch (inst) {
> +            case 1:
> +                goto fail;
> +            case 2:
> +                if (!(e2 & (DESC_R_MASK | DESC_C_MASK))) {
> +                    goto fail;
> +                }
> +                break;
> +            case 3:
> +            case 4:
> +                if (!(e2 & DESC_C_MASK)) {
> +                    goto check_pl;
> +                }
> +                break;
> +            default:
>                  goto fail;
>              }
>          }
> @@ -2325,140 +2342,56 @@ target_ulong helper_lsl(CPUX86State *env, target_ulong selector1)
>          case 3:
>          case 9:
>          case 11:
> -            break;
> +            if (inst == 3) {
> +                break;
> +            }
> +        case 5:
> +        case 12:
> +            if (inst == 4) {
> +                break;
> +            }
>          default:
>              goto fail;
>          }
> -        if (dpl < cpl || dpl < rpl) {
> -        fail:
> -            CC_SRC = eflags & ~CC_Z;
> -            return 0;
> -        }
> +        goto check_pl;
> +    }
> +
> +    if (inst == 3) {
> +        e2 &= 0x00f0ff00;
>      }
> -    limit = get_seg_limit(e1, e2);
> +    if (inst == 4) {
> +        e2 = get_seg_limit(e1, e2);
> +    }
> +
>      CC_SRC = eflags | CC_Z;
> -    return limit;
> +
> +check_pl:
> +    if (dpl < cpl || dpl < rpl) {
> +        goto fail;
> +    }
> +
> +fail:
> +    return e2;
>  }
>  
> -target_ulong helper_lar(CPUX86State *env, target_ulong selector1)
> +target_ulong helper_lsl(CPUX86State *env, target_ulong selector1)
>  {
> -    uint32_t e1, e2, eflags, selector;
> -    int rpl, dpl, cpl, type;
> +    return misc_check_helper(env, selector1, 4);
> +}
>  
> -    selector = selector1 & 0xffff;
> -    eflags = cpu_cc_compute_all(env, CC_OP);
> -    if ((selector & 0xfffc) == 0) {
> -        goto fail;
> -    }
> -    if (load_segment(env, &e1, &e2, selector) != 0) {
> -        goto fail;
> -    }
> -    rpl = selector & 3;
> -    dpl = (e2 >> DESC_DPL_SHIFT) & 3;
> -    cpl = env->hflags & HF_CPL_MASK;
> -    if (e2 & DESC_S_MASK) {
> -        if ((e2 & DESC_CS_MASK) && (e2 & DESC_C_MASK)) {
> -            /* conforming */
> -        } else {
> -            if (dpl < cpl || dpl < rpl) {
> -                goto fail;
> -            }
> -        }
> -    } else {
> -        type = (e2 >> DESC_TYPE_SHIFT) & 0xf;
> -        switch (type) {
> -        case 1:
> -        case 2:
> -        case 3:
> -        case 4:
> -        case 5:
> -        case 9:
> -        case 11:
> -        case 12:
> -            break;
> -        default:
> -            goto fail;
> -        }
> -        if (dpl < cpl || dpl < rpl) {
> -        fail:
> -            CC_SRC = eflags & ~CC_Z;
> -            return 0;
> -        }
> -    }
> -    CC_SRC = eflags | CC_Z;
> -    return e2 & 0x00f0ff00;
> +target_ulong helper_lar(CPUX86State *env, target_ulong selector1)
> +{
> +    return misc_check_helper(env, selector1, 3);
>  }
>  
>  void helper_verr(CPUX86State *env, target_ulong selector1)
>  {
> -    uint32_t e1, e2, eflags, selector;
> -    int rpl, dpl, cpl;
> -
> -    selector = selector1 & 0xffff;
> -    eflags = cpu_cc_compute_all(env, CC_OP);
> -    if ((selector & 0xfffc) == 0) {
> -        goto fail;
> -    }
> -    if (load_segment(env, &e1, &e2, selector) != 0) {
> -        goto fail;
> -    }
> -    if (!(e2 & DESC_S_MASK)) {
> -        goto fail;
> -    }
> -    rpl = selector & 3;
> -    dpl = (e2 >> DESC_DPL_SHIFT) & 3;
> -    cpl = env->hflags & HF_CPL_MASK;
> -    if (e2 & DESC_CS_MASK) {
> -        if (!(e2 & DESC_R_MASK)) {
> -            goto fail;
> -        }
> -        if (!(e2 & DESC_C_MASK)) {
> -            if (dpl < cpl || dpl < rpl) {
> -                goto fail;
> -            }
> -        }
> -    } else {
> -        if (dpl < cpl || dpl < rpl) {
> -        fail:
> -            CC_SRC = eflags & ~CC_Z;
> -            return;
> -        }
> -    }
> -    CC_SRC = eflags | CC_Z;
> +    misc_check_helper(env, selector1, 2);
>  }
>  
>  void helper_verw(CPUX86State *env, target_ulong selector1)
>  {
> -    uint32_t e1, e2, eflags, selector;
> -    int rpl, dpl, cpl;
> -
> -    selector = selector1 & 0xffff;
> -    eflags = cpu_cc_compute_all(env, CC_OP);
> -    if ((selector & 0xfffc) == 0) {
> -        goto fail;
> -    }
> -    if (load_segment(env, &e1, &e2, selector) != 0) {
> -        goto fail;
> -    }
> -    if (!(e2 & DESC_S_MASK)) {
> -        goto fail;
> -    }
> -    rpl = selector & 3;
> -    dpl = (e2 >> DESC_DPL_SHIFT) & 3;
> -    cpl = env->hflags & HF_CPL_MASK;
> -    if (e2 & DESC_CS_MASK) {
> -        goto fail;
> -    } else {
> -        if (dpl < cpl || dpl < rpl) {
> -            goto fail;
> -        }
> -        if (!(e2 & DESC_W_MASK)) {
> -        fail:
> -            CC_SRC = eflags & ~CC_Z;
> -            return;
> -        }
> -    }
> -    CC_SRC = eflags | CC_Z;
> +    misc_check_helper(env, selector1, 1);
>  }
>  
>  #if defined(CONFIG_USER_ONLY)

Peter Maydell May 23, 2013, 9:24 a.m. UTC | #5

On 23 May 2013 09:35, li guang <lig.fnst@cn.fujitsu.com> wrote:
> ping ... again.

misc_check_helper is still a terrible function name.

-- PMM

liguang May 24, 2013, 12:17 a.m. UTC | #6

在 2013-05-23四的 10:24 +0100，Peter Maydell写道：
> On 23 May 2013 09:35, li guang <lig.fnst@cn.fujitsu.com> wrote:
> > ping ... again.
> 
> misc_check_helper is still a terrible function name.
> 

Oh, sorry, I ping the obsolete one,
I've sent a v2 for your comment:
v2: change misc_check_helper to privilege_check
http://lists.gnu.org/archive/html/qemu-devel/2013-05/msg00554.html

I'll ping it.

Thanks!

[1/2] target-i386/seg_helper: refactor 4 helper functions

Commit Message

Comments

Patch