target-i386: Fix I/O bitmap checks for in/out

Commit 1b90d56e changed the implementation of in/out imm to not assign
the accessed port number to cpu_T[0] as it appeared unnecessary.
However, currently gen_check_io() makes use of cpu_T[0] to implement the
I/O bitmap checks, so it's in fact still used and the change broke the
check, leading to #GP in legitimate cases (and probably also allowing
access to ports that shouldn't be allowed).

This patch reintroduces the missing assignment for these cases.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 target-i386/translate.c | 2 ++
 1 file changed, 2 insertions(+)

Am 24.01.2014 um 11:19 hat Kevin Wolf geschrieben:
> Commit 1b90d56e changed the implementation of in/out imm to not assign
> the accessed port number to cpu_T[0] as it appeared unnecessary.
> However, currently gen_check_io() makes use of cpu_T[0] to implement the
> I/O bitmap checks, so it's in fact still used and the change broke the
> check, leading to #GP in legitimate cases (and probably also allowing
> access to ports that shouldn't be allowed).
> 
> This patch reintroduces the missing assignment for these cases.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Richard Henderson <rth@twiddle.net>

Ping?

/me considers sending a one-patch pull request for an area he's
absolutely not maintaining, but if this is the only way to get patches
applied to qemu...

Kevin

> ---
>  target-i386/translate.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index b0f2279..5dd2450 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -6284,6 +6284,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
>      case 0xe5:
>          ot = mo_b_d32(b, dflag);
>          val = cpu_ldub_code(env, s->pc++);
> +        tcg_gen_movi_tl(cpu_T[0], val);
>          gen_check_io(s, ot, pc_start - s->cs_base,
>                       SVM_IOIO_TYPE_MASK | svm_is_rep(prefixes));
>          if (use_icount)
> @@ -6300,6 +6301,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
>      case 0xe7:
>          ot = mo_b_d32(b, dflag);
>          val = cpu_ldub_code(env, s->pc++);
> +        tcg_gen_movi_tl(cpu_T[0], val);
>          gen_check_io(s, ot, pc_start - s->cs_base,
>                       svm_is_rep(prefixes));
>          gen_op_mov_v_reg(ot, cpu_T[1], R_EAX);
> -- 
> 1.8.1.4
>

On 17 February 2014 10:14, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 24.01.2014 um 11:19 hat Kevin Wolf geschrieben:
>> Commit 1b90d56e changed the implementation of in/out imm to not assign
>> the accessed port number to cpu_T[0] as it appeared unnecessary.
>> However, currently gen_check_io() makes use of cpu_T[0] to implement the
>> I/O bitmap checks, so it's in fact still used and the change broke the
>> check, leading to #GP in legitimate cases (and probably also allowing
>> access to ports that shouldn't be allowed).
>>
>> This patch reintroduces the missing assignment for these cases.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> Reviewed-by: Richard Henderson <rth@twiddle.net>
>
> Ping?
>
> /me considers sending a one-patch pull request for an area he's
> absolutely not maintaining, but if this is the only way to get patches
> applied to qemu...

I don't currently have a workflow for identifying and applying
patches which aren't in pull requests (apart from obvious
"fixes build breakage" patches, and even there it's depending
on my happening to notice them). In this case I'd expect rth
to put together a pull request, I guess.

Suggestions for better workflows welcome; we have had issues
with patches falling through the gaps between maintained
subsystems for a long time.

thanks
-- PMM

Am 17.02.2014 um 11:47 hat Peter Maydell geschrieben:
> On 17 February 2014 10:14, Kevin Wolf <kwolf@redhat.com> wrote:
> > Am 24.01.2014 um 11:19 hat Kevin Wolf geschrieben:
> >> Commit 1b90d56e changed the implementation of in/out imm to not assign
> >> the accessed port number to cpu_T[0] as it appeared unnecessary.
> >> However, currently gen_check_io() makes use of cpu_T[0] to implement the
> >> I/O bitmap checks, so it's in fact still used and the change broke the
> >> check, leading to #GP in legitimate cases (and probably also allowing
> >> access to ports that shouldn't be allowed).
> >>
> >> This patch reintroduces the missing assignment for these cases.
> >>
> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >> Reviewed-by: Richard Henderson <rth@twiddle.net>
> >
> > Ping?
> >
> > /me considers sending a one-patch pull request for an area he's
> > absolutely not maintaining, but if this is the only way to get patches
> > applied to qemu...
> 
> I don't currently have a workflow for identifying and applying
> patches which aren't in pull requests (apart from obvious
> "fixes build breakage" patches, and even there it's depending
> on my happening to notice them). In this case I'd expect rth
> to put together a pull request, I guess.

The problem is the "I guess" part, especially if Richard guesses
otherwise. target-i386 happens to be an officially unmaintained area.
This is the get_maintainer.pl output:

qemu-devel@nongnu.org (odd fixer:X86)
Richard Henderson <rth@twiddle.net> (commit_signer:123/126=98%)
Peter Maydell <peter.maydell@linaro.org> (commit_signer:51/126=40%)
Paolo Bonzini <pbonzini@redhat.com> (commit_signer:32/126=25%)
Blue Swirl <blauwirbel@gmail.com> (commit_signer:13/126=10%)

Richard, would you be willing to take up official maintainership to
solve at least this uncertainty?

> Suggestions for better workflows welcome; we have had issues
> with patches falling through the gaps between maintained
> subsystems for a long time.

Yes, we have a lot of code that doesn't fall in any subsystem with a
subtree maintainer. This is the really worrying part here. I'm pretty
sure I would get this specific patch merged the one or the other way
(after all, my pull requests are generally accepted), but if even I fail
to get it in using the "normal" way, it probably also means that
contributors outside of the core team have no chance at all getting any
patches in.

This is alarming and certainly can't be healthy.

I think Anthony did try to apply such patches that don't belong to any
submaintainer's area (even though often with considerable delays), but
I'm not sure how much time it cost him and how he managed to filter them.

Anthony, any hints?

Kevin