Re: [PATCH V4 14/24] 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,
	Julien Grall <julien.grall@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Subject: Re: [PATCH V4 14/24] arm/ioreq: Introduce arch specific bits for IOREQ/DM features
Date: Mon, 18 Jan 2021 19:17:48 +0000	[thread overview]
Message-ID: <b79b07eb-02db-16bb-2d8b-a55b0c355dbb@xen.org> (raw)
In-Reply-To: <e2dcc876-291f-1244-933c-179f97a84e07@gmail.com>

Hi Oleksandr,

On 17/01/2021 18:52, Oleksandr wrote:
> 
> On 17.01.21 20:07, Julien Grall wrote:
>>
>>
>> On 17/01/2021 17:11, Oleksandr wrote:
>>>
>>> On 15.01.21 22:26, Julien Grall wrote:
>>>
>>> Hi Julien
>>
>> Hi Oleksandr,
> 
> 
> Hi Julien
> 
> 
> 
>>
>>>>
>>>>> +
>>>>>       PROGRESS(xen):
>>>>>           ret = relinquish_memory(d, &d->xenpage_list);
>>>>>           if ( ret )
>>>>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>>>>> index ae7ef96..9814481 100644
>>>>> --- a/xen/arch/arm/io.c
>>>>> +++ b/xen/arch/arm/io.c
>>>>> @@ -16,6 +16,7 @@
>>>>>    * GNU General Public License for more details.
>>>>>    */
>>>>>   +#include <xen/ioreq.h>
>>>>>   #include <xen/lib.h>
>>>>>   #include <xen/spinlock.h>
>>>>>   #include <xen/sched.h>
>>>>> @@ -23,6 +24,7 @@
>>>>>   #include <asm/cpuerrata.h>
>>>>>   #include <asm/current.h>
>>>>>   #include <asm/mmio.h>
>>>>> +#include <asm/hvm/ioreq.h>
>>>>
>>>> Shouldn't this have been included by "xen/ioreq.h"?
>>> Well, for V1 asm/hvm/ioreq.h was included by xen/ioreq.h. But, it 
>>> turned out that there was nothing inside common header required arch 
>>> one to be included and
>>> I was asked to include arch header where it was indeed needed 
>>> (several *.c files).
>>
>> Fair enough.
>>
>> [...]
>>
>>>>
>>>> If you return IO_HANDLED here, then it means the we will take care 
>>>> of previous I/O but the current one is going to be ignored. 
>>> Which current one? As I understand, if try_fwd_ioserv() gets called 
>>> with vio->req.state == STATE_IORESP_READY then this is a second round 
>>> after emulator completes the emulation (the first round was when
>>> we returned IO_RETRY down the function and claimed that we would need 
>>> a completion), so we are still dealing with previous I/O.
>>> vcpu_ioreq_handle_completion() -> arch_ioreq_complete_mmio() -> 
>>> try_handle_mmio() -> try_fwd_ioserv() -> handle_ioserv()
>>> And after we return IO_HANDLED here, handle_ioserv() will be called 
>>> to complete the handling of this previous I/O emulation.
>>> Or I really missed something?
>>
>> Hmmm... I somehow thought try_fw_ioserv() would only be called the 
>> first time. Do you have a branch with your code applied? This would 
>> help to follow the different paths.
> Yes, I mentioned about it in cover letter.
> 
> Please see
> https://github.com/otyshchenko1/xen/commits/ioreq_4.14_ml5
> why 5 - because I started counting from the RFC)

Oh, I looked at the cover letter and didn't find it. Hence why I asked. 
I should have looked more carefully. Thanks!

I have looked closer at the question and I am not sure to understand why 
arch_ioreq_complete_mmio() is going to call try_handle_mmio().

This looks pretty innefficient to me because we already now the IO was 
handled by the IOREQ server.

I realize that x86 is calling handle_mmio() again. However, I don't 
think we need the same on Arm because the instruction for accessing 
device memory are a lot simpler (you can only read or store at most a 
64-bit value).

So I would like to keep our emulation simple and not rely on 
try_ioserv_fw() to always return true when call from completion (AFAICT 
it is not possible to return false then).

I will answer to the rest separately.

Cheers,

-- 
Julien Grall