[Qemu-devel] Correction needed for R5900 instruction decoding

[Qemu-devel] Correction needed for R5900 instruction decoding

[Aleksandar Markovic]

Hi, Fridrik,

I did some closer code inspection of R5900 in last few days, and I noticed some sub-optimal implementation in the area where R5900-specific opcodes overlap with the rest-of-MIPS-CPUs opcodes.

The right implementation should be based on the principle that all such cases are covered with if statements involving INSN_R5900 flag, like this:

        if (ctx->insn_flags & INSN_R5900) {
            <R5900-specific handling>
        } else {
            <rest-of-MIPS-handling>
        }

You followed that principle for OPC_SPECIAL2 and OPC_SPECIAL3, but for some other opcodes not. For example, there are lines:

    if (reg == 0 && (opc == OPC_MFHI || opc == TX79_MMI_MFHI1 ||
                     opc == OPC_MFLO || opc == TX79_MMI_MFLO1)) {

or

     switch (opc) {
     case OPC_MFHI:
     case TX79_MMI_MFHI1:

Such implementation makes it difficult to discern R5900 and non-R5900 cases. Potentialy allows bugs to sneak in and affect non-R5900 support.

The correction is not that difficult, I gather. Worse comme to worst, you can remove R5900 MFLO1 and MFHI1 altogether, they are not that essential at this moment, but do try correcting the decoding stuff as I described. Can you please make these changes in next few days or so (given that 3.1 release is getting closer and closer), and send them to the list?

It is my bad that I didn't spot this during review, but in any case, I think this should be fixed in 3.1 to make sure that non-R5900 functionalities are intact.

Thanks,
Aleksandar

Re: [Qemu-devel] Correction needed for R5900 instruction decoding

[Philippe Mathieu-Daudé]

Cc'ing Fredrik.

On 1/11/18 12:06, Aleksandar Markovic wrote:
> Hi, Fridrik,
> 
> I did some closer code inspection of R5900 in last few days, and I noticed some sub-optimal implementation in the area where R5900-specific opcodes overlap with the rest-of-MIPS-CPUs opcodes.
> 
> The right implementation should be based on the principle that all such cases are covered with if statements involving INSN_R5900 flag, like this:
> 
>          if (ctx->insn_flags & INSN_R5900) {
>              <R5900-specific handling>
>          } else {
>              <rest-of-MIPS-handling>
>          }
> 
> You followed that principle for OPC_SPECIAL2 and OPC_SPECIAL3, but for some other opcodes not. For example, there are lines:
> 
>      if (reg == 0 && (opc == OPC_MFHI || opc == TX79_MMI_MFHI1 ||
>                       opc == OPC_MFLO || opc == TX79_MMI_MFLO1)) {
> 
> or
> 
>       switch (opc) {
>       case OPC_MFHI:
>       case TX79_MMI_MFHI1:
> 
> Such implementation makes it difficult to discern R5900 and non-R5900 cases. Potentialy allows bugs to sneak in and affect non-R5900 support.
> 
> The correction is not that difficult, I gather. Worse comme to worst, you can remove R5900 MFLO1 and MFHI1 altogether, they are not that essential at this moment, but do try correcting the decoding stuff as I described. Can you please make these changes in next few days or so (given that 3.1 release is getting closer and closer), and send them to the list?
> 
> It is my bad that I didn't spot this during review, but in any case, I think this should be fixed in 3.1 to make sure that non-R5900 functionalities are intact.
> 
> Thanks,
> Aleksandar
> 
> 

Re: [Qemu-devel] Correction needed for R5900 instruction decoding

[Emilio G. Cota]

Cc'ing Fredrik, who I think was the intended recipient of the below.

		E.

On Thu, Nov 01, 2018 at 11:06:30 +0000, Aleksandar Markovic wrote:
> Hi, Fridrik,
> 
> I did some closer code inspection of R5900 in last few days, and I noticed some sub-optimal implementation in the area where R5900-specific opcodes overlap with the rest-of-MIPS-CPUs opcodes.
> 
> The right implementation should be based on the principle that all such cases are covered with if statements involving INSN_R5900 flag, like this:
> 
>         if (ctx->insn_flags & INSN_R5900) {
>             <R5900-specific handling>
>         } else {
>             <rest-of-MIPS-handling>
>         }
> 
> You followed that principle for OPC_SPECIAL2 and OPC_SPECIAL3, but for some other opcodes not. For example, there are lines:
> 
>     if (reg == 0 && (opc == OPC_MFHI || opc == TX79_MMI_MFHI1 ||
>                      opc == OPC_MFLO || opc == TX79_MMI_MFLO1)) {
> 
> or
> 
>      switch (opc) {
>      case OPC_MFHI:
>      case TX79_MMI_MFHI1:
> 
> Such implementation makes it difficult to discern R5900 and non-R5900 cases. Potentialy allows bugs to sneak in and affect non-R5900 support.
> 
> The correction is not that difficult, I gather. Worse comme to worst, you can remove R5900 MFLO1 and MFHI1 altogether, they are not that essential at this moment, but do try correcting the decoding stuff as I described. Can you please make these changes in next few days or so (given that 3.1 release is getting closer and closer), and send them to the list?
> 
> It is my bad that I didn't spot this during review, but in any case, I think this should be fixed in 3.1 to make sure that non-R5900 functionalities are intact.
> 
> Thanks,
> Aleksandar
> 
> 

Re: [Qemu-devel] Correction needed for R5900 instruction decoding

[Fredrik Noring]

[ Philippe and Emilio -- thank you for cc-ing me. Good catch, since I'm
not subscribed to the QEMU mailing list. Changes to the R5900 emulation
are certainly of interest. ]

Hi Aleksandar, Philippe,

On Thu, Nov 01, 2018 at 03:31:54PM +0100, Philippe Mathieu-Daudé wrote:
> Cc'ing Fredrik.
> 
> On 1/11/18 12:06, Aleksandar Markovic wrote:
> > Hi, Fridrik,
> > 
> > I did some closer code inspection of R5900 in last few days, and I
> > noticed some sub-optimal implementation in the area where R5900-specific
> > opcodes overlap with the rest-of-MIPS-CPUs opcodes.
> > 
> > The right implementation should be based on the principle that all such
> > cases are covered with if statements involving INSN_R5900 flag, like
> > this:
> > 
> >          if (ctx->insn_flags & INSN_R5900) {
> >              <R5900-specific handling>
> >          } else {
> >              <rest-of-MIPS-handling>
> >          }
> > 
> > You followed that principle for OPC_SPECIAL2 and OPC_SPECIAL3, but for
> > some other opcodes not. For example, there are lines:
> > 
> >      if (reg == 0 && (opc == OPC_MFHI || opc == TX79_MMI_MFHI1 ||
> >                       opc == OPC_MFLO || opc == TX79_MMI_MFLO1)) {
> > 
> > or
> > 
> >       switch (opc) {
> >       case OPC_MFHI:
> >       case TX79_MMI_MFHI1:
> > 
> > Such implementation makes it difficult to discern R5900 and non-R5900
> > cases. Potentialy allows bugs to sneak in and affect non-R5900 support.

MFLO1, MFHI1, MTLO1 and MTHI1 for the TX79 and the R5900 are already
decoded in the ISA specific decode_tx79_mmi function, and thereby follow
your first suggested pattern. They do however reuse the gen_HILO function,
but it is a simpel matter to post a patch to make a new gen_tx79_HILO1
variant that is almost identical to the original gen_HILO.

The only other case is gen_muldiv that is used for DIV1 and DIVU1. The
same argument applies and a TX79 specific variant would be similar to the
original, but I can certainly post a variant for that one too.

> > The correction is not that difficult, I gather. Worse comme to worst,
> > you can remove R5900 MFLO1 and MFHI1 altogether, they are not that
> > essential at this moment, but do try correcting the decoding stuff as I
> > described. Can you please make these changes in next few days or so
> > (given that 3.1 release is getting closer and closer), and send them to
> > the list?

MFLO1 and MFHI1 are essential for MULT1, MULTU1, DIV1 and DIVU1 as well as
MADD1 and MADDU1 in the patch series I posted 25 October "[PATCH 00/11]
target/mips: Amend R5900 support". I will post updated patches shortly!

> > It is my bad that I didn't spot this during review, but in any case, I
> > think this should be fixed in 3.1 to make sure that non-R5900
> > functionalities are intact.

It is a common pattern in target/mips/translate.c to cover several ISAs
in the same gen_* and decode_* functions, especially when there are only
minor differences between them.

Fredrik

Re: [Qemu-devel] Correction needed for R5900 instruction decoding

[Aleksandar Markovic]

Fredrik, please do not include handling of any opcode other than those currently in the tree.

There are good and bad patterns in the code, and not every pattern is OK to follow.

Thanks,
Aleksandar

________________________________________
From: Fredrik Noring <noring@nocrew.org>
Sent: Thursday, November 1, 2018 6:23:53 PM
To: Philippe Mathieu-Daudé; Emilio G. Cota; Aleksandar Markovic
Cc: Stefan Markovic; Petar Jovanovic; Aleksandar Rikalo; Maciej W. Rozycki; Jürgen Urban; qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] Correction needed for R5900 instruction decoding

[ Philippe and Emilio -- thank you for cc-ing me. Good catch, since I'm
not subscribed to the QEMU mailing list. Changes to the R5900 emulation
are certainly of interest. ]

Hi Aleksandar, Philippe,

On Thu, Nov 01, 2018 at 03:31:54PM +0100, Philippe Mathieu-Daudé wrote:
> Cc'ing Fredrik.
>
> On 1/11/18 12:06, Aleksandar Markovic wrote:
> > Hi, Fridrik,
> >
> > I did some closer code inspection of R5900 in last few days, and I
> > noticed some sub-optimal implementation in the area where R5900-specific
> > opcodes overlap with the rest-of-MIPS-CPUs opcodes.
> >
> > The right implementation should be based on the principle that all such
> > cases are covered with if statements involving INSN_R5900 flag, like
> > this:
> >
> >          if (ctx->insn_flags & INSN_R5900) {
> >              <R5900-specific handling>
> >          } else {
> >              <rest-of-MIPS-handling>
> >          }
> >
> > You followed that principle for OPC_SPECIAL2 and OPC_SPECIAL3, but for
> > some other opcodes not. For example, there are lines:
> >
> >      if (reg == 0 && (opc == OPC_MFHI || opc == TX79_MMI_MFHI1 ||
> >                       opc == OPC_MFLO || opc == TX79_MMI_MFLO1)) {
> >
> > or
> >
> >       switch (opc) {
> >       case OPC_MFHI:
> >       case TX79_MMI_MFHI1:
> >
> > Such implementation makes it difficult to discern R5900 and non-R5900
> > cases. Potentialy allows bugs to sneak in and affect non-R5900 support.

MFLO1, MFHI1, MTLO1 and MTHI1 for the TX79 and the R5900 are already
decoded in the ISA specific decode_tx79_mmi function, and thereby follow
your first suggested pattern. They do however reuse the gen_HILO function,
but it is a simpel matter to post a patch to make a new gen_tx79_HILO1
variant that is almost identical to the original gen_HILO.

The only other case is gen_muldiv that is used for DIV1 and DIVU1. The
same argument applies and a TX79 specific variant would be similar to the
original, but I can certainly post a variant for that one too.

> > The correction is not that difficult, I gather. Worse comme to worst,
> > you can remove R5900 MFLO1 and MFHI1 altogether, they are not that
> > essential at this moment, but do try correcting the decoding stuff as I
> > described. Can you please make these changes in next few days or so
> > (given that 3.1 release is getting closer and closer), and send them to
> > the list?

MFLO1 and MFHI1 are essential for MULT1, MULTU1, DIV1 and DIVU1 as well as
MADD1 and MADDU1 in the patch series I posted 25 October "[PATCH 00/11]
target/mips: Amend R5900 support". I will post updated patches shortly!

> > It is my bad that I didn't spot this during review, but in any case, I
> > think this should be fixed in 3.1 to make sure that non-R5900
> > functionalities are intact.

It is a common pattern in target/mips/translate.c to cover several ISAs
in the same gen_* and decode_* functions, especially when there are only
minor differences between them.

Fredrik

Re: [Qemu-devel] Correction needed for R5900 instruction decoding

[Aleksandar Markovic]

> ... in the patch series I posted 25 October "[PATCH 00/11]
> target/mips: Amend R5900 support". I will post updated patches shortly!

Fridrik,

It is now code freeze before 3.1, the code base is being stabilized, and only important fixes are allowed to be integrated - so, in that light, a separate patch, or a small series, that addresses only concerns from the original mail of this thread is needed. Such series should not contain any additional features (like your v2 of the series "Amend..." does), and its patch titles should look like "Fix decoding mechanism of ..." or such.

Could you please provide those appropriate changes in that format?

Thanks,
Aleksandar

________________________________________
From: Fredrik Noring <noring@nocrew.org>
Sent: Thursday, November 1, 2018 6:23:53 PM
To: Philippe Mathieu-Daudé; Emilio G. Cota; Aleksandar Markovic
Cc: Stefan Markovic; Petar Jovanovic; Aleksandar Rikalo; Maciej W. Rozycki; Jürgen Urban; qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] Correction needed for R5900 instruction decoding

[ Philippe and Emilio -- thank you for cc-ing me. Good catch, since I'm
not subscribed to the QEMU mailing list. Changes to the R5900 emulation
are certainly of interest. ]

Hi Aleksandar, Philippe,

On Thu, Nov 01, 2018 at 03:31:54PM +0100, Philippe Mathieu-Daudé wrote:
> Cc'ing Fredrik.
>
> On 1/11/18 12:06, Aleksandar Markovic wrote:
> > Hi, Fridrik,
> >
> > I did some closer code inspection of R5900 in last few days, and I
> > noticed some sub-optimal implementation in the area where R5900-specific
> > opcodes overlap with the rest-of-MIPS-CPUs opcodes.
> >
> > The right implementation should be based on the principle that all such
> > cases are covered with if statements involving INSN_R5900 flag, like
> > this:
> >
> >          if (ctx->insn_flags & INSN_R5900) {
> >              <R5900-specific handling>
> >          } else {
> >              <rest-of-MIPS-handling>
> >          }
> >
> > You followed that principle for OPC_SPECIAL2 and OPC_SPECIAL3, but for
> > some other opcodes not. For example, there are lines:
> >
> >      if (reg == 0 && (opc == OPC_MFHI || opc == TX79_MMI_MFHI1 ||
> >                       opc == OPC_MFLO || opc == TX79_MMI_MFLO1)) {
> >
> > or
> >
> >       switch (opc) {
> >       case OPC_MFHI:
> >       case TX79_MMI_MFHI1:
> >
> > Such implementation makes it difficult to discern R5900 and non-R5900
> > cases. Potentialy allows bugs to sneak in and affect non-R5900 support.

MFLO1, MFHI1, MTLO1 and MTHI1 for the TX79 and the R5900 are already
decoded in the ISA specific decode_tx79_mmi function, and thereby follow
your first suggested pattern. They do however reuse the gen_HILO function,
but it is a simpel matter to post a patch to make a new gen_tx79_HILO1
variant that is almost identical to the original gen_HILO.

The only other case is gen_muldiv that is used for DIV1 and DIVU1. The
same argument applies and a TX79 specific variant would be similar to the
original, but I can certainly post a variant for that one too.

> > The correction is not that difficult, I gather. Worse comme to worst,
> > you can remove R5900 MFLO1 and MFHI1 altogether, they are not that
> > essential at this moment, but do try correcting the decoding stuff as I
> > described. Can you please make these changes in next few days or so
> > (given that 3.1 release is getting closer and closer), and send them to
> > the list?

MFLO1 and MFHI1 are essential for MULT1, MULTU1, DIV1 and DIVU1 as well as
MADD1 and MADDU1 in the patch series I posted 25 October "[PATCH 00/11]
target/mips: Amend R5900 support". I will post updated patches shortly!

> > It is my bad that I didn't spot this during review, but in any case, I
> > think this should be fixed in 3.1 to make sure that non-R5900
> > functionalities are intact.

It is a common pattern in target/mips/translate.c to cover several ISAs
in the same gen_* and decode_* functions, especially when there are only
minor differences between them.

Fredrik

Re: [Qemu-devel] Correction needed for R5900 instruction decoding

[Fredrik Noring]

Hi Aleksandar,

> It is now code freeze before 3.1, the code base is being stabilized, and
> only important fixes are allowed to be integrated - so, in that light, a
> separate patch, or a small series, that addresses only concerns from the
> original mail of this thread is needed. Such series should not contain any
> additional features (like your v2 of the series "Amend..." does), and its
> patch titles should look like "Fix decoding mechanism of ..." or such.
> 
> Could you please provide those appropriate changes in that format?

I certainly could, but why not simply apply patch 1 and 2 in the posted
v2 series and leave the rest for later? The only difference in a repost
would be the cover letter. The two patches would be identical reposts
that apply since yesterday.

The patch series is ordered with "crucial patches as the first ones",
as you wanted it according to:

http://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg04946.html

Fredrik

Re: [Qemu-devel] Correction needed for R5900 instruction decoding

[Aleksandar Markovic]

Hi, Fredrik.

> From: Fredrik Noring <noring@nocrew.org>
> Subject: Re: [Qemu-devel] Correction needed for R5900 instruction decoding
> 
> Hi Aleksandar,
> 
> > It is now code freeze before 3.1, the code base is being stabilized, and
> > only important fixes are allowed to be integrated - so, in that light, a
> > separate patch, or a small series, that addresses only concerns from the
> > original mail of this thread is needed. Such series should not contain any
> > additional features (like your v2 of the series "Amend..." does), and its
> > patch titles should look like "Fix decoding mechanism of ..." or such.
> >
> > Could you please provide those appropriate changes in that format?
> 
> I certainly could, but why not simply apply patch 1 and 2 in the posted
> v2 series and leave the rest for later?

How do you know patches 1 and 2 will and should be applied? You jump to conclusions. Also, a basic rule while analyzing problems and their solutions is to avoid and omit irrelevant parts.

> The only difference in a repost would be the cover letter.

Not true, I asked for different titles, and I may ask for something else - a lot of things can change. For example, I think a separate decode_opc_special_legacy() for R5900 is needed, I don't see that in your series.

Thanks,
Aleksandar

> The two patches would be identical reposts that apply since yesterday.
> 
> The patch series is ordered with "crucial patches as the first ones",
> as you wanted it according to:
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg04946.html
> 
> Fredrik

Re: [Qemu-devel] Correction needed for R5900 instruction decoding

[Peter Maydell]

On 2 November 2018 at 15:03, Aleksandar Markovic <amarkovic@wavecomp.com> wrote:
> Hi, Fredrik.
>
>> From: Fredrik Noring <noring@nocrew.org>
>> Subject: Re: [Qemu-devel] Correction needed for R5900 instruction decoding
>>
>> Hi Aleksandar,
>>
>> > It is now code freeze before 3.1, the code base is being stabilized, and
>> > only important fixes are allowed to be integrated - so, in that light, a
>> > separate patch, or a small series, that addresses only concerns from the
>> > original mail of this thread is needed. Such series should not contain any
>> > additional features (like your v2 of the series "Amend..." does), and its
>> > patch titles should look like "Fix decoding mechanism of ..." or such.
>> >
>> > Could you please provide those appropriate changes in that format?
>>
>> I certainly could, but why not simply apply patch 1 and 2 in the posted
>> v2 series and leave the rest for later?
>
> How do you know patches 1 and 2 will and should be applied? You jump
> to conclusions. Also, a basic rule while analyzing problems and their
> solutions is to avoid and omit irrelevant parts.

Hey guys, can we try to keep the tone of the conversation friendly here?

I think what Fred is suggesting is that the minimal set of fixing
patches would be just patch 1 and 2 from that set, and so you could
if you wanted apply those two patches to get the desired effect.

>From the other side of things, as a submaintainer around release
time there's often a lot of work to do and it's easy to confuse
different patchsets or forget the status of them, so it's useful
to have a patch series which is exactly the set of patches that
the submitter thinks are suitable to go into the release, and it's
less work to apply those than to fish out a subset of patches
from a series.

So overall, I think my suggestion would be that the best move
from here would be for Fred to send a patchset with the changes
for 3.1 and only those changes. Could you do that, please?

thanks
-- PMM

Re: [Qemu-devel] Correction needed for R5900 instruction decoding

[Fredrik Noring]

Hi Peter,

> From the other side of things, as a submaintainer around release
> time there's often a lot of work to do and it's easy to confuse
> different patchsets or forget the status of them, so it's useful
> to have a patch series which is exactly the set of patches that
> the submitter thinks are suitable to go into the release, and it's
> less work to apply those than to fish out a subset of patches
> from a series.

Understood. Aleksandar previously indicated that he wanted an amendment
series with changes ordered by importance, which is why the two patches
were part of that series (as the first ones).

> So overall, I think my suggestion would be that the best move
> from here would be for Fred to send a patchset with the changes
> for 3.1 and only those changes. Could you do that, please?

Yes, I will post a separate series for review immediately.

Fredrik

Re: [Qemu-devel] Correction needed for R5900 instruction decoding

[Philippe Mathieu-Daudé]

Hi Aleksandar,

On 1/11/18 12:06, Aleksandar Markovic wrote:
> Hi, Fridrik,
> 
> I did some closer code inspection of R5900 in last few days, and I noticed some sub-optimal implementation in the area where R5900-specific opcodes overlap with the rest-of-MIPS-CPUs opcodes.
> 
> The right implementation should be based on the principle that all such cases are covered with if statements involving INSN_R5900 flag, like this:
> 
>          if (ctx->insn_flags & INSN_R5900) {
>              <R5900-specific handling>
>          } else {
>              <rest-of-MIPS-handling>
>          }
> 
> You followed that principle for OPC_SPECIAL2 and OPC_SPECIAL3, but for some other opcodes not. For example, there are lines:
> 
>      if (reg == 0 && (opc == OPC_MFHI || opc == TX79_MMI_MFHI1 ||
>                       opc == OPC_MFLO || opc == TX79_MMI_MFLO1)) {
> 
> or
> 
>       switch (opc) {
>       case OPC_MFHI:
>       case TX79_MMI_MFHI1:
> 
> Such implementation makes it difficult to discern R5900 and non-R5900 cases. Potentialy allows bugs to sneak in and affect non-R5900 support.
> 
> The correction is not that difficult, I gather. Worse comme to worst, you can remove R5900 MFLO1 and MFHI1 altogether, they are not that essential at this moment, but do try correcting the decoding stuff as I described. Can you please make these changes in next few days or so (given that 3.1 release is getting closer and closer), and send them to the list?
> 
> It is my bad that I didn't spot this during review, but in any case, I think this should be fixed in 3.1 to make sure that non-R5900 functionalities are intact.

Don't be too bad on yourself, we are human thus not perfect :) This is 
why having more that one (or not always the same) person reviewing is 
helpful.

You can share the blame with all the person subscribed to the list who 
did not look at the patch ;)

Regards,

Phil.

> 
> Thanks,
> Aleksandar
> 
>