Hi Oleksandr,
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_tyshche...@epam.com>
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?
[...]
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.
Cheers,
--
Julien Grall