Re: [PATCH V1 09/16] arm/ioreq: Introduce arch specific bits for IOREQ/DM features

[Oleksandr <olekstysh@xxxxxxxxx>]

On 26.09.20 16:21, Julien Grall wrote:
> Hi Oleksandr,
Hi Julien.


> On 24/09/2020 19:22, Oleksandr wrote:
> > On 24.09.20 20:25, Julien Grall wrote:
> > > On 23/09/2020 21:16, Oleksandr wrote:
> > > > On 23.09.20 21:03, Julien Grall wrote:
> > > > > On 10/09/2020 21:22, Oleksandr Tyshchenko wrote:
> > > > > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > > Could you please clarify how this patch could be split in smaller one?
> > > This patch is going to be reduced a fair bit if you make some of the 
> > > structure common. The next steps would be to move anything that is 
> > > not directly related to IOREQ out.
> >
> > Thank you for the clarification.
> > Yes, however, I believed everything in this patch is directly related 
> > to IOREQ...
> >
> > > From a quick look, there are few things that can be moved in 
> > > separate patches:
> > >    - The addition of the ASSERT_UNREACHABLE()
> > Did you mean the addition of the ASSERT_UNREACHABLE() to 
> > arch_handle_hvm_io_completion/handle_pio can moved to separate patches?
> > Sorry, I don't quite understand, for what benefit?
> Sorry I didn't realize there was multiple ASSERT_UNREACHABLE() in the 
> code. I was referring to the one in the follow chunk:
> @@ -1955,9 +1959,14 @@ static void do_trap_stage2_abort_guest(struct 
> cpu_user_regs *regs,
>              case IO_HANDLED:
>                  advance_pc(regs, hsr);
>                  return;
> +            case IO_RETRY:
> +                /* finish later */
> +                return;
>              case IO_UNHANDLED:
>                  /* IO unhandled, try another way to handle it. */
>                  break;
> +            default:
> +                ASSERT_UNREACHABLE();
>              }
>          }
>
> While I understand the reason this was added, to me this doesn't seem 
> to be directly related to this patch.
> In fact, the switch case will be done on an enum. So without the 
> default, the compiler will be able to notice if we are adding a new 
> field. With this new approach, you would only notice at runtime 
> (assuming the path is exercised).
> So what do we gain?
Hmm, now I am in doubt whether we really need to put 
ASSERT_UNREACHABLE() here. Also we would notice it at the runtime for 
debug builds only.

> [...]
>
> > > I think Jan made some suggestion today. Let me know if you require 
> > > more input.
> >
> > Yes. I am considering this now. I provided my thoughts on that a 
> > little bit earlier. Could you please clarify there.
> I have replied to it.
Thank you.


--
Regards,

Oleksandr Tyshchenko