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

From: Julien Grall <julien@xen.org>
To: Oleksandr <olekstysh@gmail.com>
Cc: xen-devel@lists.xenproject.org,
	Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Julien Grall <julien.grall@arm.com>
Subject: Re: [PATCH V1 09/16] arm/ioreq: Introduce arch specific bits for IOREQ/DM features
Date: Sat, 26 Sep 2020 14:21:07 +0100	[thread overview]
Message-ID: <aa284c2a-c632-a446-2f14-03b22b402919@xen.org> (raw)
In-Reply-To: <fcb40929-9487-1d20-3990-09c79cab8df8@gmail.com>

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_tyshchenko@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

