Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Stefano Stabellini]

Julien,

This seems to be an arm randconfig failure:

https://gitlab.com/xen-project/patchew/xen/-/pipelines/246632953
https://gitlab.com/xen-project/patchew/xen/-/jobs/985455044


On Mon, 25 Jan 2021, no-reply@patchew.org wrote:
> Hi,
> 
> Patchew automatically ran gitlab-ci pipeline with this patch (series) applied, but the job failed. Maybe there's a bug in the patches?
> 
> You can find the link to the pipeline near the end of the report below:
> 
> Type: series
> Message-id: 1611601709-28361-1-git-send-email-olekstysh@gmail.com
> Subject: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> sleep 10
> patchew gitlab-pipeline-check -p xen-project/patchew/xen
> === TEST SCRIPT END ===
> 
> warning: redirecting to https://gitlab.com/xen-project/patchew/xen.git/
> warning: redirecting to https://gitlab.com/xen-project/patchew/xen.git/
> From https://gitlab.com/xen-project/patchew/xen
>  * [new tag]               patchew/1611601709-28361-1-git-send-email-olekstysh@gmail.com -> patchew/1611601709-28361-1-git-send-email-olekstysh@gmail.com
> Switched to a new branch 'test'
> 51c66995c4 xen/arm: Add mapcache invalidation handling
> ac18dce945 xen/ioreq: Make x86's send_invalidate_req() common
> bff65c909c xen/arm: io: Harden sign extension check
> db7d33509b xen/arm: io: Abstract sign-extension
> 49d920436c xen/dm: Introduce xendevicemodel_set_irq_level DM op
> 9f750615ff xen/ioreq: Introduce domain_has_ioreq_server()
> 8bcf50987f xen/mm: Handle properly reference in set_foreign_p2m_entry() on Arm
> 54b6d5517d xen/arm: Call vcpu_ioreq_handle_completion() in check_for_vcpu_work()
> c54d6b6a4c arm/ioreq: Introduce arch specific bits for IOREQ/DM features
> 955655a87c xen/ioreq: Use guest_cmpxchg64() instead of cmpxchg()
> ff3da51e59 xen/ioreq: Remove "hvm" prefixes from involved function names
> f2d022b8d2 xen/mm: Make x86's XENMEM_resource_ioreq_server handling common
> dc137ff63c xen/ioreq: Move x86's io_completion/io_req fields to struct vcpu
> da63f74717 xen/ioreq: Make x86's IOREQ related dm-op handling common
> 53ed326f85 xen/ioreq: Move x86's ioreq_server to struct domain
> 6eb4a9b103 xen/ioreq: Make x86's hvm_ioreq_(page/vcpu/server) structs common
> 9198aac40e xen/ioreq: Make x86's hvm_mmio_first(last)_byte() common
> 64669ca3f1 xen/ioreq: Make x86's hvm_ioreq_needs_completion() common
> 64ed7b62fb xen/ioreq: Make x86's IOREQ feature common
> 8fc382a03b x86/ioreq: Provide out-of-line wrapper for the handle_mmio()
> 1aac704b38 x86/ioreq: Add IOREQ_STATUS_* #define-s and update code for moving
> ab818a53dc x86/ioreq: Prepare IOREQ feature for making it common
> 
> === OUTPUT BEGIN ===
> [2021-01-25 19:28:41] Looking up pipeline...
> [2021-01-25 19:28:42] Found pipeline 246632953:
> 
> https://gitlab.com/xen-project/patchew/xen/-/pipelines/246632953
> 
> [2021-01-25 19:28:42] Waiting for pipeline to finish...
> [2021-01-25 19:43:46] Still waiting...
> [2021-01-25 19:58:50] Still waiting...
> [2021-01-25 20:13:55] Still waiting...
> [2021-01-25 20:29:00] Still waiting...
> [2021-01-25 20:44:04] Still waiting...
> [2021-01-25 20:53:07] Pipeline failed
> [2021-01-25 20:53:08] Job 'qemu-smoke-x86-64-clang-pvh' in stage 'test' is skipped
> [2021-01-25 20:53:08] Job 'qemu-smoke-x86-64-gcc-pvh' in stage 'test' is skipped
> [2021-01-25 20:53:08] Job 'qemu-smoke-x86-64-clang' in stage 'test' is skipped
> [2021-01-25 20:53:08] Job 'qemu-smoke-x86-64-gcc' in stage 'test' is skipped
> [2021-01-25 20:53:08] Job 'qemu-smoke-arm64-gcc' in stage 'test' is skipped
> [2021-01-25 20:53:08] Job 'qemu-alpine-arm64-gcc' in stage 'test' is skipped
> [2021-01-25 20:53:08] Job 'build-each-commit-gcc' in stage 'test' is skipped
> [2021-01-25 20:53:08] Job 'debian-unstable-gcc-debug-arm64-randconfig' in stage 'build' is failed
> [2021-01-25 20:53:08] Job 'debian-unstable-gcc-arm64-randconfig' in stage 'build' is failed
> [2021-01-25 20:53:08] Job 'alpine-3.12-clang-debug' in stage 'build' is failed
> [2021-01-25 20:53:08] Job 'alpine-3.12-clang' in stage 'build' is failed
> [2021-01-25 20:53:08] Job 'alpine-3.12-gcc-debug' in stage 'build' is failed
> [2021-01-25 20:53:08] Job 'alpine-3.12-gcc' in stage 'build' is failed
> === OUTPUT END ===
> 
> Test command exited with code: 1

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Julien Grall]

On Mon, 25 Jan 2021 at 20:56, Stefano Stabellini <sstabellini@kernel.org> wrote:
>
> Julien,

Hi,

>
> This seems to be an arm randconfig failure:
>
> https://gitlab.com/xen-project/patchew/xen/-/pipelines/246632953
> https://gitlab.com/xen-project/patchew/xen/-/jobs/985455044

Thanks! The error is:

#'target_mem_ref' not supported by expression#'memory.c: In function
'do_memory_op':
memory.c:1210:18: error:  may be used uninitialized in this function
[-Werror=maybe-uninitialized]
 1210 |             rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1211 |                                        _mfn(mfn_list[i]));
      |                                        ~~~~~~~~~~~~~~~~~~

I found a few references online of the error message, but it is not
clear what it means. From a quick look at Oleksandr's branch, I also
can't spot anything unitialized. Any ideas?

Cheers,

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Oleksandr]

On 26.01.21 01:20, Julien Grall wrote:

Hi Julien, Stefano

> On Mon, 25 Jan 2021 at 20:56, Stefano Stabellini <sstabellini@kernel.org> wrote:
>> Julien,
> Hi,
>
>> This seems to be an arm randconfig failure:
>>
>> https://gitlab.com/xen-project/patchew/xen/-/pipelines/246632953
>> https://gitlab.com/xen-project/patchew/xen/-/jobs/985455044
> Thanks! The error is:
>
> #'target_mem_ref' not supported by expression#'memory.c: In function
> 'do_memory_op':
> memory.c:1210:18: error:  may be used uninitialized in this function
> [-Werror=maybe-uninitialized]
>   1210 |             rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
>        |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   1211 |                                        _mfn(mfn_list[i]));
>        |                                        ~~~~~~~~~~~~~~~~~~
>
> I found a few references online of the error message, but it is not
> clear what it means. From a quick look at Oleksandr's branch, I also
> can't spot anything unitialized. Any ideas?
It seems that error happens if *both* CONFIG_GRANT_TABLE and 
CONFIG_IOREQ_SERVER are disabled. Looks like that mfn_list is 
initialized either in acquire_grant_table() or in acquire_ioreq_server().
If these options disabled then corresponding helpers are just stubs, so 
indeed that mfn_list gets uninitialized. But, I am not sure why gcc 
complains about it as set_foreign_p2m_entry() is *not* going to be 
called in that case???



-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Oleksandr]

On 26.01.21 02:14, Oleksandr wrote:

Hello, all

>
> On 26.01.21 01:20, Julien Grall wrote:
>
> Hi Julien, Stefano
>
>> On Mon, 25 Jan 2021 at 20:56, Stefano Stabellini 
>> <sstabellini@kernel.org> wrote:
>>> Julien,
>> Hi,
>>
>>> This seems to be an arm randconfig failure:
>>>
>>> https://gitlab.com/xen-project/patchew/xen/-/pipelines/246632953
>>> https://gitlab.com/xen-project/patchew/xen/-/jobs/985455044
>> Thanks! The error is:
>>
>> #'target_mem_ref' not supported by expression#'memory.c: In function
>> 'do_memory_op':
>> memory.c:1210:18: error:  may be used uninitialized in this function
>> [-Werror=maybe-uninitialized]
>>   1210 |             rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
>>        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>   1211 | _mfn(mfn_list[i]));
>>        | ~~~~~~~~~~~~~~~~~~
>>
>> I found a few references online of the error message, but it is not
>> clear what it means. From a quick look at Oleksandr's branch, I also
>> can't spot anything unitialized. Any ideas?
> It seems that error happens if *both* CONFIG_GRANT_TABLE and 
> CONFIG_IOREQ_SERVER are disabled. Looks like that mfn_list is 
> initialized either in acquire_grant_table() or in acquire_ioreq_server().
> If these options disabled then corresponding helpers are just stubs, 
> so indeed that mfn_list gets uninitialized. But, I am not sure why gcc 
> complains about it as set_foreign_p2m_entry() is *not* going to be 
> called in that case???

This weird build error goes away if I simply add:

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 33296e6..d1bd57b 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1136,7 +1136,7 @@ static int acquire_resource(
       * moment since they are small, but if they need to grow in future
       * use-cases then per-CPU arrays or heap allocations may be required.
       */
-    xen_pfn_t mfn_list[32];
+    xen_pfn_t mfn_list[32] = {0};
      int rc;

      if ( !arch_acquire_resource_check(currd) )


Shall I make the corresponding patch?
But it is still unclear to me why the compiler doesn't recognize that 
*non-yet-uninitialized* mfn_list[] won't be used if both 
CONFIG_GRANT_TABLE and CONFIG_IOREQ_SERVER are not set...


-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Jan Beulich]

On 27.01.2021 11:13, Oleksandr wrote:
> On 26.01.21 02:14, Oleksandr wrote:
>> On 26.01.21 01:20, Julien Grall wrote:
>>> On Mon, 25 Jan 2021 at 20:56, Stefano Stabellini 
>>> <sstabellini@kernel.org> wrote:
>>>> This seems to be an arm randconfig failure:
>>>>
>>>> https://gitlab.com/xen-project/patchew/xen/-/pipelines/246632953
>>>> https://gitlab.com/xen-project/patchew/xen/-/jobs/985455044
>>> Thanks! The error is:
>>>
>>> #'target_mem_ref' not supported by expression#'memory.c: In function

Btw, I found the first part of this line pretty confusing, to a
degree that when seeing it initially I thought this must be some
odd tool producing the odd error. But perhaps this is just
unfortunate output ordering from different tools running in
parallel.

>>> 'do_memory_op':
>>> memory.c:1210:18: error:  may be used uninitialized in this function
>>> [-Werror=maybe-uninitialized]
>>>   1210 |             rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
>>>        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>   1211 | _mfn(mfn_list[i]));
>>>        | ~~~~~~~~~~~~~~~~~~
>>>
>>> I found a few references online of the error message, but it is not
>>> clear what it means. From a quick look at Oleksandr's branch, I also
>>> can't spot anything unitialized. Any ideas?
>> It seems that error happens if *both* CONFIG_GRANT_TABLE and 
>> CONFIG_IOREQ_SERVER are disabled. Looks like that mfn_list is 
>> initialized either in acquire_grant_table() or in acquire_ioreq_server().
>> If these options disabled then corresponding helpers are just stubs, 
>> so indeed that mfn_list gets uninitialized. But, I am not sure why gcc 
>> complains about it as set_foreign_p2m_entry() is *not* going to be 
>> called in that case???
> 
> This weird build error goes away if I simply add:
> 
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 33296e6..d1bd57b 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1136,7 +1136,7 @@ static int acquire_resource(
>        * moment since they are small, but if they need to grow in future
>        * use-cases then per-CPU arrays or heap allocations may be required.
>        */
> -    xen_pfn_t mfn_list[32];
> +    xen_pfn_t mfn_list[32] = {0};
>       int rc;
> 
>       if ( !arch_acquire_resource_check(currd) )
> 
> 
> Shall I make the corresponding patch?

I'd prefer if we could find another solution, avoiding this
pointless writing of 256 bytes of zeros (and really to be on the
safe side I think it should rather be ~0 that gets put in there).
Could you check whether clearing the array along the lines of
this

    default:
        memset(mfn_list, ~0, sizeof(mfn_list));
        rc = -EOPNOTSUPP;
        break;

helps (avoiding the writes in all normal cases)? Of course this
wouldn't be a guarantee that another compiler (version) won't
warn yet again. But the only other alternative I can think of
without having the writes on the common path would be something
along the lines of older Linux'es uninitialized_var(). Maybe
someone else has a better idea ...

> But it is still unclear to me why the compiler doesn't recognize that 
> *non-yet-uninitialized* mfn_list[] won't be used if both 
> CONFIG_GRANT_TABLE and CONFIG_IOREQ_SERVER are not set...

The combination of conditions may be too complex for it to
figure. I suppose a warning like this can't be had without at
least one of false positives or false negatives.

Jan

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Oleksandr]

On 27.01.21 12:51, Jan Beulich wrote:

Hi Jan

> On 27.01.2021 11:13, Oleksandr wrote:
>> On 26.01.21 02:14, Oleksandr wrote:
>>> On 26.01.21 01:20, Julien Grall wrote:
>>>> On Mon, 25 Jan 2021 at 20:56, Stefano Stabellini
>>>> <sstabellini@kernel.org> wrote:
>>>>> This seems to be an arm randconfig failure:
>>>>>
>>>>> https://gitlab.com/xen-project/patchew/xen/-/pipelines/246632953
>>>>> https://gitlab.com/xen-project/patchew/xen/-/jobs/985455044
>>>> Thanks! The error is:
>>>>
>>>> #'target_mem_ref' not supported by expression#'memory.c: In function
> Btw, I found the first part of this line pretty confusing, to a
> degree that when seeing it initially I thought this must be some
> odd tool producing the odd error. But perhaps this is just
> unfortunate output ordering from different tools running in
> parallel.
>
>>>> 'do_memory_op':
>>>> memory.c:1210:18: error:  may be used uninitialized in this function
>>>> [-Werror=maybe-uninitialized]
>>>>    1210 |             rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
>>>>         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>    1211 | _mfn(mfn_list[i]));
>>>>         | ~~~~~~~~~~~~~~~~~~
>>>>
>>>> I found a few references online of the error message, but it is not
>>>> clear what it means. From a quick look at Oleksandr's branch, I also
>>>> can't spot anything unitialized. Any ideas?
>>> It seems that error happens if *both* CONFIG_GRANT_TABLE and
>>> CONFIG_IOREQ_SERVER are disabled. Looks like that mfn_list is
>>> initialized either in acquire_grant_table() or in acquire_ioreq_server().
>>> If these options disabled then corresponding helpers are just stubs,
>>> so indeed that mfn_list gets uninitialized. But, I am not sure why gcc
>>> complains about it as set_foreign_p2m_entry() is *not* going to be
>>> called in that case???
>> This weird build error goes away if I simply add:
>>
>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>> index 33296e6..d1bd57b 100644
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -1136,7 +1136,7 @@ static int acquire_resource(
>>         * moment since they are small, but if they need to grow in future
>>         * use-cases then per-CPU arrays or heap allocations may be required.
>>         */
>> -    xen_pfn_t mfn_list[32];
>> +    xen_pfn_t mfn_list[32] = {0};
>>        int rc;
>>
>>        if ( !arch_acquire_resource_check(currd) )
>>
>>
>> Shall I make the corresponding patch?
> I'd prefer if we could find another solution, avoiding this
> pointless writing of 256 bytes of zeros (and really to be on the
> safe side I think it should rather be ~0 that gets put in there).
> Could you check whether clearing the array along the lines of
> this
>
>      default:
>          memset(mfn_list, ~0, sizeof(mfn_list));
>          rc = -EOPNOTSUPP;
>          break;
>
> helps (avoiding the writes in all normal cases)?

Yes, this helps (at least in my environment):

aarch64-poky-linux-gcc v8.2


> Of course this
> wouldn't be a guarantee that another compiler (version) won't
> warn yet again. But the only other alternative I can think of
> without having the writes on the common path would be something
> along the lines of older Linux'es uninitialized_var(). Maybe
> someone else has a better idea ...
>
>> But it is still unclear to me why the compiler doesn't recognize that
>> *non-yet-uninitialized* mfn_list[] won't be used if both
>> CONFIG_GRANT_TABLE and CONFIG_IOREQ_SERVER are not set...
> The combination of conditions may be too complex for it to
> figure. I suppose a warning like this can't be had without at
> least one of false positives or false negatives.

I got it.

-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Jan Beulich]

On 27.01.2021 12:15, Oleksandr wrote:
> On 27.01.21 12:51, Jan Beulich wrote:
>> On 27.01.2021 11:13, Oleksandr wrote:
>>> On 26.01.21 02:14, Oleksandr wrote:
>>>> On 26.01.21 01:20, Julien Grall wrote:
>>>>> On Mon, 25 Jan 2021 at 20:56, Stefano Stabellini
>>>>> <sstabellini@kernel.org> wrote:
>>>>>> This seems to be an arm randconfig failure:
>>>>>>
>>>>>> https://gitlab.com/xen-project/patchew/xen/-/pipelines/246632953
>>>>>> https://gitlab.com/xen-project/patchew/xen/-/jobs/985455044
>>>>> Thanks! The error is:
>>>>>
>>>>> #'target_mem_ref' not supported by expression#'memory.c: In function
>> Btw, I found the first part of this line pretty confusing, to a
>> degree that when seeing it initially I thought this must be some
>> odd tool producing the odd error. But perhaps this is just
>> unfortunate output ordering from different tools running in
>> parallel.
>>
>>>>> 'do_memory_op':
>>>>> memory.c:1210:18: error:  may be used uninitialized in this function
>>>>> [-Werror=maybe-uninitialized]
>>>>>    1210 |             rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
>>>>>         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>    1211 | _mfn(mfn_list[i]));
>>>>>         | ~~~~~~~~~~~~~~~~~~
>>>>>
>>>>> I found a few references online of the error message, but it is not
>>>>> clear what it means. From a quick look at Oleksandr's branch, I also
>>>>> can't spot anything unitialized. Any ideas?
>>>> It seems that error happens if *both* CONFIG_GRANT_TABLE and
>>>> CONFIG_IOREQ_SERVER are disabled. Looks like that mfn_list is
>>>> initialized either in acquire_grant_table() or in acquire_ioreq_server().
>>>> If these options disabled then corresponding helpers are just stubs,
>>>> so indeed that mfn_list gets uninitialized. But, I am not sure why gcc
>>>> complains about it as set_foreign_p2m_entry() is *not* going to be
>>>> called in that case???
>>> This weird build error goes away if I simply add:
>>>
>>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>>> index 33296e6..d1bd57b 100644
>>> --- a/xen/common/memory.c
>>> +++ b/xen/common/memory.c
>>> @@ -1136,7 +1136,7 @@ static int acquire_resource(
>>>         * moment since they are small, but if they need to grow in future
>>>         * use-cases then per-CPU arrays or heap allocations may be required.
>>>         */
>>> -    xen_pfn_t mfn_list[32];
>>> +    xen_pfn_t mfn_list[32] = {0};
>>>        int rc;
>>>
>>>        if ( !arch_acquire_resource_check(currd) )
>>>
>>>
>>> Shall I make the corresponding patch?
>> I'd prefer if we could find another solution, avoiding this
>> pointless writing of 256 bytes of zeros (and really to be on the
>> safe side I think it should rather be ~0 that gets put in there).
>> Could you check whether clearing the array along the lines of
>> this
>>
>>      default:
>>          memset(mfn_list, ~0, sizeof(mfn_list));
>>          rc = -EOPNOTSUPP;
>>          break;
>>
>> helps (avoiding the writes in all normal cases)?
> 
> Yes, this helps (at least in my environment):
> 
> aarch64-poky-linux-gcc v8.2

Good. I'd be okay if you folded this in (plus a comment of
course), but others may have different views, not the least as
this is only papering over the issue (yet an issue that's not
ours, but the compiler's).

Jan

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Oleksandr]

On 27.01.21 13:22, Jan Beulich wrote:

Hi Jan

> On 27.01.2021 12:15, Oleksandr wrote:
>> On 27.01.21 12:51, Jan Beulich wrote:
>>> On 27.01.2021 11:13, Oleksandr wrote:
>>>> On 26.01.21 02:14, Oleksandr wrote:
>>>>> On 26.01.21 01:20, Julien Grall wrote:
>>>>>> On Mon, 25 Jan 2021 at 20:56, Stefano Stabellini
>>>>>> <sstabellini@kernel.org> wrote:
>>>>>>> This seems to be an arm randconfig failure:
>>>>>>>
>>>>>>> https://gitlab.com/xen-project/patchew/xen/-/pipelines/246632953
>>>>>>> https://gitlab.com/xen-project/patchew/xen/-/jobs/985455044
>>>>>> Thanks! The error is:
>>>>>>
>>>>>> #'target_mem_ref' not supported by expression#'memory.c: In function
>>> Btw, I found the first part of this line pretty confusing, to a
>>> degree that when seeing it initially I thought this must be some
>>> odd tool producing the odd error. But perhaps this is just
>>> unfortunate output ordering from different tools running in
>>> parallel.
>>>
>>>>>> 'do_memory_op':
>>>>>> memory.c:1210:18: error:  may be used uninitialized in this function
>>>>>> [-Werror=maybe-uninitialized]
>>>>>>     1210 |             rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
>>>>>>          | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>     1211 | _mfn(mfn_list[i]));
>>>>>>          | ~~~~~~~~~~~~~~~~~~
>>>>>>
>>>>>> I found a few references online of the error message, but it is not
>>>>>> clear what it means. From a quick look at Oleksandr's branch, I also
>>>>>> can't spot anything unitialized. Any ideas?
>>>>> It seems that error happens if *both* CONFIG_GRANT_TABLE and
>>>>> CONFIG_IOREQ_SERVER are disabled. Looks like that mfn_list is
>>>>> initialized either in acquire_grant_table() or in acquire_ioreq_server().
>>>>> If these options disabled then corresponding helpers are just stubs,
>>>>> so indeed that mfn_list gets uninitialized. But, I am not sure why gcc
>>>>> complains about it as set_foreign_p2m_entry() is *not* going to be
>>>>> called in that case???
>>>> This weird build error goes away if I simply add:
>>>>
>>>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>>>> index 33296e6..d1bd57b 100644
>>>> --- a/xen/common/memory.c
>>>> +++ b/xen/common/memory.c
>>>> @@ -1136,7 +1136,7 @@ static int acquire_resource(
>>>>          * moment since they are small, but if they need to grow in future
>>>>          * use-cases then per-CPU arrays or heap allocations may be required.
>>>>          */
>>>> -    xen_pfn_t mfn_list[32];
>>>> +    xen_pfn_t mfn_list[32] = {0};
>>>>         int rc;
>>>>
>>>>         if ( !arch_acquire_resource_check(currd) )
>>>>
>>>>
>>>> Shall I make the corresponding patch?
>>> I'd prefer if we could find another solution, avoiding this
>>> pointless writing of 256 bytes of zeros (and really to be on the
>>> safe side I think it should rather be ~0 that gets put in there).
>>> Could you check whether clearing the array along the lines of
>>> this
>>>
>>>       default:
>>>           memset(mfn_list, ~0, sizeof(mfn_list));
>>>           rc = -EOPNOTSUPP;
>>>           break;
>>>
>>> helps (avoiding the writes in all normal cases)?
>> Yes, this helps (at least in my environment):
>>
>> aarch64-poky-linux-gcc v8.2
> Good. I'd be okay if you folded this in (plus a comment of
> course), but others may have different views, not the least as
> this is only papering over the issue (yet an issue that's not
> ours, but the compiler's).

Great, I will wait a bit and if there are no objections I will fold this 
in.

-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Julien Grall]

On 27/01/2021 10:51, Jan Beulich wrote:
> On 27.01.2021 11:13, Oleksandr wrote:
>> On 26.01.21 02:14, Oleksandr wrote:
>>> On 26.01.21 01:20, Julien Grall wrote:
>>>> On Mon, 25 Jan 2021 at 20:56, Stefano Stabellini
>>>> <sstabellini@kernel.org> wrote:
>>>>> This seems to be an arm randconfig failure:
>>>>>
>>>>> https://gitlab.com/xen-project/patchew/xen/-/pipelines/246632953
>>>>> https://gitlab.com/xen-project/patchew/xen/-/jobs/985455044
>>>> Thanks! The error is:
>>>>
>>>> #'target_mem_ref' not supported by expression#'memory.c: In function
> 
> Btw, I found the first part of this line pretty confusing, to a
> degree that when seeing it initially I thought this must be some
> odd tool producing the odd error. But perhaps this is just
> unfortunate output ordering from different tools running in
> parallel.

This message is actually coming from GCC. There are quite a few reports 
online.

Although, I am not sure whether this was intended.

> 
>>>> 'do_memory_op':
>>>> memory.c:1210:18: error:  may be used uninitialized in this function
>>>> [-Werror=maybe-uninitialized]
>>>>    1210 |             rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
>>>>         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>    1211 | _mfn(mfn_list[i]));
>>>>         | ~~~~~~~~~~~~~~~~~~
>>>>
>>>> I found a few references online of the error message, but it is not
>>>> clear what it means. From a quick look at Oleksandr's branch, I also
>>>> can't spot anything unitialized. Any ideas?
>>> It seems that error happens if *both* CONFIG_GRANT_TABLE and
>>> CONFIG_IOREQ_SERVER are disabled. Looks like that mfn_list is
>>> initialized either in acquire_grant_table() or in acquire_ioreq_server().
>>> If these options disabled then corresponding helpers are just stubs,
>>> so indeed that mfn_list gets uninitialized. But, I am not sure why gcc
>>> complains about it as set_foreign_p2m_entry() is *not* going to be
>>> called in that case???
>>
>> This weird build error goes away if I simply add:
>>
>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>> index 33296e6..d1bd57b 100644
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -1136,7 +1136,7 @@ static int acquire_resource(
>>         * moment since they are small, but if they need to grow in future
>>         * use-cases then per-CPU arrays or heap allocations may be required.
>>         */
>> -    xen_pfn_t mfn_list[32];
>> +    xen_pfn_t mfn_list[32] = {0};
>>        int rc;
>>
>>        if ( !arch_acquire_resource_check(currd) )
>>
>>
>> Shall I make the corresponding patch?
> 
> I'd prefer if we could find another solution, avoiding this
> pointless writing of 256 bytes of zeros (and really to be on the
> safe side I think it should rather be ~0 that gets put in there).
> Could you check whether clearing the array along the lines of
> this
> 
>      default:
>          memset(mfn_list, ~0, sizeof(mfn_list));
>          rc = -EOPNOTSUPP;
>          break;
> 
> helps (avoiding the writes in all normal cases)? Of course this
> wouldn't be a guarantee that another compiler (version) won't
> warn yet again. But the only other alternative I can think of
> without having the writes on the common path would be something
> along the lines of older Linux'es uninitialized_var(). Maybe
> someone else has a better idea ...

So GCC is complaining specifically about mfn_list[0]. For instance, I 
wrote the following:

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 33296e65cb04..81b4645047e7 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1139,6 +1139,8 @@ static int acquire_resource(
      xen_pfn_t mfn_list[32];
      int rc;

+    mfn_list[0] = 0;
+
      if ( !arch_acquire_resource_check(currd) )
          return -EACCES;

It will silence GCC. But the follwing will not:

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 33296e65cb04..cf558a6b9fa4 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1139,6 +1139,8 @@ static int acquire_resource(
      xen_pfn_t mfn_list[32];
      int rc;

+    mfn_list[1] = 0;
+
      if ( !arch_acquire_resource_check(currd) )
          return -EACCES;

I also try to set mfn_list[0] to 0 in different part of the switch. It 
doesn't help, if I add it in the default. But it does, if I put in the 
grant table path.

So it looks like the grant table path is the issue.

I can confirm that your solution will silenece GCC, but I am a bit worry 
that this may only hide a different bug because the failure seems to be 
widespread on arm (gitlab reported the error with GCC 9.2.1 and I have 
managed to reproduced on GCC 7.5.0).

Given this is a randconfig where CONFIG_GRANT_TABLE is disabled (we 
technically don't grant table disabled), I would rather take a bit more 
time to understand the problem rather than rushing it.

Cheers,

-- 
Julien Grall

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Jan Beulich]

On 27.01.2021 16:29, Julien Grall wrote:
> 
> 
> On 27/01/2021 10:51, Jan Beulich wrote:
>> On 27.01.2021 11:13, Oleksandr wrote:
>>> On 26.01.21 02:14, Oleksandr wrote:
>>>> On 26.01.21 01:20, Julien Grall wrote:
>>>>> On Mon, 25 Jan 2021 at 20:56, Stefano Stabellini
>>>>> <sstabellini@kernel.org> wrote:
>>>>>> This seems to be an arm randconfig failure:
>>>>>>
>>>>>> https://gitlab.com/xen-project/patchew/xen/-/pipelines/246632953
>>>>>> https://gitlab.com/xen-project/patchew/xen/-/jobs/985455044
>>>>> Thanks! The error is:
>>>>>
>>>>> #'target_mem_ref' not supported by expression#'memory.c: In function
>>
>> Btw, I found the first part of this line pretty confusing, to a
>> degree that when seeing it initially I thought this must be some
>> odd tool producing the odd error. But perhaps this is just
>> unfortunate output ordering from different tools running in
>> parallel.
> 
> This message is actually coming from GCC. There are quite a few reports 
> online.
> 
> Although, I am not sure whether this was intended.
> 
>>
>>>>> 'do_memory_op':
>>>>> memory.c:1210:18: error:  may be used uninitialized in this function
>>>>> [-Werror=maybe-uninitialized]
>>>>>    1210 |             rc = set_foreign_p2m_entry(currd, d, gfn_list[i],
>>>>>         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>    1211 | _mfn(mfn_list[i]));
>>>>>         | ~~~~~~~~~~~~~~~~~~
>>>>>
>>>>> I found a few references online of the error message, but it is not
>>>>> clear what it means. From a quick look at Oleksandr's branch, I also
>>>>> can't spot anything unitialized. Any ideas?
>>>> It seems that error happens if *both* CONFIG_GRANT_TABLE and
>>>> CONFIG_IOREQ_SERVER are disabled. Looks like that mfn_list is
>>>> initialized either in acquire_grant_table() or in acquire_ioreq_server().
>>>> If these options disabled then corresponding helpers are just stubs,
>>>> so indeed that mfn_list gets uninitialized. But, I am not sure why gcc
>>>> complains about it as set_foreign_p2m_entry() is *not* going to be
>>>> called in that case???
>>>
>>> This weird build error goes away if I simply add:
>>>
>>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>>> index 33296e6..d1bd57b 100644
>>> --- a/xen/common/memory.c
>>> +++ b/xen/common/memory.c
>>> @@ -1136,7 +1136,7 @@ static int acquire_resource(
>>>         * moment since they are small, but if they need to grow in future
>>>         * use-cases then per-CPU arrays or heap allocations may be required.
>>>         */
>>> -    xen_pfn_t mfn_list[32];
>>> +    xen_pfn_t mfn_list[32] = {0};
>>>        int rc;
>>>
>>>        if ( !arch_acquire_resource_check(currd) )
>>>
>>>
>>> Shall I make the corresponding patch?
>>
>> I'd prefer if we could find another solution, avoiding this
>> pointless writing of 256 bytes of zeros (and really to be on the
>> safe side I think it should rather be ~0 that gets put in there).
>> Could you check whether clearing the array along the lines of
>> this
>>
>>      default:
>>          memset(mfn_list, ~0, sizeof(mfn_list));
>>          rc = -EOPNOTSUPP;
>>          break;
>>
>> helps (avoiding the writes in all normal cases)? Of course this
>> wouldn't be a guarantee that another compiler (version) won't
>> warn yet again. But the only other alternative I can think of
>> without having the writes on the common path would be something
>> along the lines of older Linux'es uninitialized_var(). Maybe
>> someone else has a better idea ...
> 
> So GCC is complaining specifically about mfn_list[0]. For instance, I 
> wrote the following:
> 
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 33296e65cb04..81b4645047e7 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1139,6 +1139,8 @@ static int acquire_resource(
>       xen_pfn_t mfn_list[32];
>       int rc;
> 
> +    mfn_list[0] = 0;
> +
>       if ( !arch_acquire_resource_check(currd) )
>           return -EACCES;
> 
> It will silence GCC. But the follwing will not:
> 
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 33296e65cb04..cf558a6b9fa4 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1139,6 +1139,8 @@ static int acquire_resource(
>       xen_pfn_t mfn_list[32];
>       int rc;
> 
> +    mfn_list[1] = 0;
> +
>       if ( !arch_acquire_resource_check(currd) )
>           return -EACCES;

Interesting.

> I also try to set mfn_list[0] to 0 in different part of the switch. It 
> doesn't help, if I add it in the default. But it does, if I put in the 
> grant table path.

Even more interesting. All pretty odd, so yes, ...

> So it looks like the grant table path is the issue.
> 
> I can confirm that your solution will silenece GCC, but I am a bit worry 
> that this may only hide a different bug because the failure seems to be 
> widespread on arm (gitlab reported the error with GCC 9.2.1 and I have 
> managed to reproduced on GCC 7.5.0).
> 
> Given this is a randconfig where CONFIG_GRANT_TABLE is disabled (we 
> technically don't grant table disabled), I would rather take a bit more 
> time to understand the problem rather than rushing it.

... this would certainly be fine with me.

Jan

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Oleksandr]

On 28.01.21 21:44, Oleksandr wrote:

Hi Andrew, Julien

>
> On 28.01.21 20:10, Andrew Cooper wrote:
>
> Hi Andrew
>
>> On 28/01/2021 17:44, Julien Grall wrote:
>>>
>>> On 28/01/2021 17:24, Stefano Stabellini wrote:
>>>> On Thu, 28 Jan 2021, Julien Grall wrote:
>>>>> On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
>>>>>    > Patch series [8] was rebased on recent "staging branch"
>>>>>> (5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is
>>>>>> unmapped) and
>>>>>> tested on
>>>>>> Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with virtio-mmio 
>>>>>> disk
>>>>>> backend [9]
>>>>>> running in driver domain and unmodified Linux Guest running on
>>>>>> existing
>>>>>> virtio-blk driver (frontend). No issues were observed. Guest domain
>>>>>> 'reboot/destroy'
>>>>>> use-cases work properly. Patch series was only build-tested on x86.
>>>>> I have done basic testing with a Linux guest on x86 and I didn't
>>>>> spot any
>>>>> regression.
>>>>>
>>>>> Unfortunately, I don't have a Windows VM in hand, so I can't confirm
>>>>> if there
>>>>> is no regression there. Can anyone give a try?
>>>>>
>>>>> On a separate topic, Andrew expressed on IRC some concern to 
>>>>> expose the
>>>>> bufioreq interface to other arch than x86. I will let him explain
>>>>> his view
>>>>> here.
>>>>>
>>>>> Given that IOREQ will be a tech preview on Arm (this implies the
>>>>> interface is
>>>>> not stable) on Arm, I think we can defer the discussion past the
>>>>> freeze.
>>>> Yes, I agree that we can defer the discussion.
>>>>
>>>>
>>>>> For now, I would suggest to check if a Device Model is trying to
>>>>> create an
>>>>> IOREQ server with bufioreq is enabled (see ioreq_server_create()).
>>>>> This would
>>>>> not alleviate Andrew's concern, however it would at allow us to
>>>>> judge whether
>>>>> the buffering concept is going to be used on Arm (I can see some use
>>>>> for the
>>>>> Virtio doorbell).
>>>>>
>>>>> What do others think?
>>>> Difficult to say. We don't have any uses today but who knows in the
>>>> future.
>>> I have some ideas, but Andrew so far objects on using the existing
>>> bufioreq interface for good reasons. It is using guest_cmpxchg() which
>>> is really expensive.
>> Worse.  Patch 5 needs to switch cmpxchg() to guest_cmpxchg() to avoid
>> reintroducing XSA-295, but doesn't.
>>
>>>> Maybe for now the important thing is to clarify that the bufioreq
>>>> interface won't be maintain for backward compatibility on ARM, and 
>>>> could
>>>> be removed without warnings, at least as long as the whole IOREQ 
>>>> feature
>>>> is a tech preview. >
>>>> In other words, it is not like we are forced to keep bufioreq around
>>>> forever on ARM.
>>> That's correct, we are not forced to use it. But if you only document
>>> it, then there is a fair chance this will split past the "Tech 
>>> Preview".
>>>
>>> Today, there is no caller which requires to send buffered I/O in the
>>> Xen Arm hypervisor. So a Device Model should not need to say "I want
>>> to allocate a page and event channel for the buffered I/O".
>>>
>>> The check I suggested serves two purposes:
>>>    1) Catch any future upstream user of bufioreq
>>>    2) Avoid an interface to be marked supported by mistake
>> "use bufioreq" is an input to create_ioreq_server.  The easiest fix in
>> the short term is "if ( !IS_ENABLED(CONFIG_X86) && bufioreq ) return
>> -EINVAL;"
>
> OK, will take into the account for V6 as a separate patch
What I would like to say is that the easiest fix works fine, it breaks 
virtio backend PoC by rejecting xendevicemodel_create_ioreq_server() call)))
No, buffered I/O is *not* used for the communication *at run-time*, a 
device model just requests bufioreq support, gets bufioreq page and 
event channel, and that's all without future use of them.
So I have just removed that unneeded at the moment initialization, what 
it more that it doesn't match with that check we would like to get in.



>
>
>
-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Julien Grall]

On Thu, 28 Jan 2021 at 20:10, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 28/01/2021 18:16, Julien Grall wrote:
> > Hi Andrew,
> >
> > On 28/01/2021 18:10, Andrew Cooper wrote:
> >> On 28/01/2021 17:44, Julien Grall wrote:
> >>>
> >>>
> >>> On 28/01/2021 17:24, Stefano Stabellini wrote:
> >>>> On Thu, 28 Jan 2021, Julien Grall wrote:
> >>>>> On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
> >>>>>    > Patch series [8] was rebased on recent "staging branch"
> >>>>>> (5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is
> >>>>>> unmapped) and
> >>>>>> tested on
> >>>>>> Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with virtio-mmio
> >>>>>> disk
> >>>>>> backend [9]
> >>>>>> running in driver domain and unmodified Linux Guest running on
> >>>>>> existing
> >>>>>> virtio-blk driver (frontend). No issues were observed. Guest domain
> >>>>>> 'reboot/destroy'
> >>>>>> use-cases work properly. Patch series was only build-tested on x86.
> >>>>>
> >>>>> I have done basic testing with a Linux guest on x86 and I didn't
> >>>>> spot any
> >>>>> regression.
> >>>>>
> >>>>> Unfortunately, I don't have a Windows VM in hand, so I can't confirm
> >>>>> if there
> >>>>> is no regression there. Can anyone give a try?
> >>>>>
> >>>>> On a separate topic, Andrew expressed on IRC some concern to
> >>>>> expose the
> >>>>> bufioreq interface to other arch than x86. I will let him explain
> >>>>> his view
> >>>>> here.
> >>>>>
> >>>>> Given that IOREQ will be a tech preview on Arm (this implies the
> >>>>> interface is
> >>>>> not stable) on Arm, I think we can defer the discussion past the
> >>>>> freeze.
> >>>>
> >>>> Yes, I agree that we can defer the discussion.
> >>>>
> >>>>
> >>>>> For now, I would suggest to check if a Device Model is trying to
> >>>>> create an
> >>>>> IOREQ server with bufioreq is enabled (see ioreq_server_create()).
> >>>>> This would
> >>>>> not alleviate Andrew's concern, however it would at allow us to
> >>>>> judge whether
> >>>>> the buffering concept is going to be used on Arm (I can see some use
> >>>>> for the
> >>>>> Virtio doorbell).
> >>>>>
> >>>>> What do others think?
> >>>>
> >>>> Difficult to say. We don't have any uses today but who knows in the
> >>>> future.
> >>>
> >>> I have some ideas, but Andrew so far objects on using the existing
> >>> bufioreq interface for good reasons. It is using guest_cmpxchg() which
> >>> is really expensive.
> >>
> >> Worse.  Patch 5 needs to switch cmpxchg() to guest_cmpxchg() to avoid
> >> reintroducing XSA-295, but doesn't.
> >
> > The memory is only shared with the emulator (we don't allow the legacy
> > interface on Arm).
>
> Excellent.
>
> > So why do you think it is re-introducing XSA-295?
>
> Because the Xen security model considers "stubdomain can DoS Xen" a
> security issue.
>
> Yes - I know that right now, it will only be the hardware domain which
> can use acquire_resource on ARM, but eventually we'll fix the
> refcounting bug, and lifting the "translate && !hwdom" restriction would
> be the thing that actually reintroduces XSA-295.

By refcounting bugs, are you referring to mapping foreign pages in a
domain? If so, on Arm, we always increment the reference counter on
the foreign page during the map request. The reference will be
released when the emulator unmap it. Therefore, we don't need the
restriction "translate && !hwdom" (see patch #16 [1]).

In addition to that, patch #13 [2] is replacing the cmpxchg() with
guest_cmpxchg() to prevent XSA-295 from reappearing.

So unless I am missing something, the two security issues you
mentioned are already covered by this series.

Cheers,

[1]  https://lore.kernel.org/xen-devel/1611601709-28361-17-git-send-email-olekstysh@gmail.com/
[2] https://lore.kernel.org/xen-devel/1611601709-28361-14-git-send-email-olekstysh@gmail.com/

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Andrew Cooper]

On 28/01/2021 18:16, Julien Grall wrote:
> Hi Andrew,
>
> On 28/01/2021 18:10, Andrew Cooper wrote:
>> On 28/01/2021 17:44, Julien Grall wrote:
>>>
>>>
>>> On 28/01/2021 17:24, Stefano Stabellini wrote:
>>>> On Thu, 28 Jan 2021, Julien Grall wrote:
>>>>> On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
>>>>>    > Patch series [8] was rebased on recent "staging branch"
>>>>>> (5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is
>>>>>> unmapped) and
>>>>>> tested on
>>>>>> Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with virtio-mmio
>>>>>> disk
>>>>>> backend [9]
>>>>>> running in driver domain and unmodified Linux Guest running on
>>>>>> existing
>>>>>> virtio-blk driver (frontend). No issues were observed. Guest domain
>>>>>> 'reboot/destroy'
>>>>>> use-cases work properly. Patch series was only build-tested on x86.
>>>>>
>>>>> I have done basic testing with a Linux guest on x86 and I didn't
>>>>> spot any
>>>>> regression.
>>>>>
>>>>> Unfortunately, I don't have a Windows VM in hand, so I can't confirm
>>>>> if there
>>>>> is no regression there. Can anyone give a try?
>>>>>
>>>>> On a separate topic, Andrew expressed on IRC some concern to
>>>>> expose the
>>>>> bufioreq interface to other arch than x86. I will let him explain
>>>>> his view
>>>>> here.
>>>>>
>>>>> Given that IOREQ will be a tech preview on Arm (this implies the
>>>>> interface is
>>>>> not stable) on Arm, I think we can defer the discussion past the
>>>>> freeze.
>>>>
>>>> Yes, I agree that we can defer the discussion.
>>>>
>>>>
>>>>> For now, I would suggest to check if a Device Model is trying to
>>>>> create an
>>>>> IOREQ server with bufioreq is enabled (see ioreq_server_create()).
>>>>> This would
>>>>> not alleviate Andrew's concern, however it would at allow us to
>>>>> judge whether
>>>>> the buffering concept is going to be used on Arm (I can see some use
>>>>> for the
>>>>> Virtio doorbell).
>>>>>
>>>>> What do others think?
>>>>
>>>> Difficult to say. We don't have any uses today but who knows in the
>>>> future.
>>>
>>> I have some ideas, but Andrew so far objects on using the existing
>>> bufioreq interface for good reasons. It is using guest_cmpxchg() which
>>> is really expensive.
>>
>> Worse.  Patch 5 needs to switch cmpxchg() to guest_cmpxchg() to avoid
>> reintroducing XSA-295, but doesn't.
>
> The memory is only shared with the emulator (we don't allow the legacy
> interface on Arm).

Excellent.

> So why do you think it is re-introducing XSA-295?

Because the Xen security model considers "stubdomain can DoS Xen" a
security issue.

Yes - I know that right now, it will only be the hardware domain which
can use acquire_resource on ARM, but eventually we'll fix the
refcounting bug, and lifting the "translate && !hwdom" restriction would
be the thing that actually reintroduces XSA-295.

~Andrew

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Oleksandr]

On 28.01.21 20:10, Andrew Cooper wrote:

Hi Andrew

> On 28/01/2021 17:44, Julien Grall wrote:
>>
>> On 28/01/2021 17:24, Stefano Stabellini wrote:
>>> On Thu, 28 Jan 2021, Julien Grall wrote:
>>>> On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
>>>>    > Patch series [8] was rebased on recent "staging branch"
>>>>> (5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is
>>>>> unmapped) and
>>>>> tested on
>>>>> Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with virtio-mmio disk
>>>>> backend [9]
>>>>> running in driver domain and unmodified Linux Guest running on
>>>>> existing
>>>>> virtio-blk driver (frontend). No issues were observed. Guest domain
>>>>> 'reboot/destroy'
>>>>> use-cases work properly. Patch series was only build-tested on x86.
>>>> I have done basic testing with a Linux guest on x86 and I didn't
>>>> spot any
>>>> regression.
>>>>
>>>> Unfortunately, I don't have a Windows VM in hand, so I can't confirm
>>>> if there
>>>> is no regression there. Can anyone give a try?
>>>>
>>>> On a separate topic, Andrew expressed on IRC some concern to expose the
>>>> bufioreq interface to other arch than x86. I will let him explain
>>>> his view
>>>> here.
>>>>
>>>> Given that IOREQ will be a tech preview on Arm (this implies the
>>>> interface is
>>>> not stable) on Arm, I think we can defer the discussion past the
>>>> freeze.
>>> Yes, I agree that we can defer the discussion.
>>>
>>>
>>>> For now, I would suggest to check if a Device Model is trying to
>>>> create an
>>>> IOREQ server with bufioreq is enabled (see ioreq_server_create()).
>>>> This would
>>>> not alleviate Andrew's concern, however it would at allow us to
>>>> judge whether
>>>> the buffering concept is going to be used on Arm (I can see some use
>>>> for the
>>>> Virtio doorbell).
>>>>
>>>> What do others think?
>>> Difficult to say. We don't have any uses today but who knows in the
>>> future.
>> I have some ideas, but Andrew so far objects on using the existing
>> bufioreq interface for good reasons. It is using guest_cmpxchg() which
>> is really expensive.
> Worse.  Patch 5 needs to switch cmpxchg() to guest_cmpxchg() to avoid
> reintroducing XSA-295, but doesn't.
>
>>> Maybe for now the important thing is to clarify that the bufioreq
>>> interface won't be maintain for backward compatibility on ARM, and could
>>> be removed without warnings, at least as long as the whole IOREQ feature
>>> is a tech preview. >
>>> In other words, it is not like we are forced to keep bufioreq around
>>> forever on ARM.
>> That's correct, we are not forced to use it. But if you only document
>> it, then there is a fair chance this will split past the "Tech Preview".
>>
>> Today, there is no caller which requires to send buffered I/O in the
>> Xen Arm hypervisor. So a Device Model should not need to say "I want
>> to allocate a page and event channel for the buffered I/O".
>>
>> The check I suggested serves two purposes:
>>    1) Catch any future upstream user of bufioreq
>>    2) Avoid an interface to be marked supported by mistake
> "use bufioreq" is an input to create_ioreq_server.  The easiest fix in
> the short term is "if ( !IS_ENABLED(CONFIG_X86) && bufioreq ) return
> -EINVAL;"

OK, will take into the account for V6 as a separate patch



-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Julien Grall]

On 28/01/2021 18:16, Julien Grall wrote:
> Hi Andrew,
> 
> On 28/01/2021 18:10, Andrew Cooper wrote:
>> On 28/01/2021 17:44, Julien Grall wrote:
>>>
>>>
>>> On 28/01/2021 17:24, Stefano Stabellini wrote:
>>>> On Thu, 28 Jan 2021, Julien Grall wrote:
>>>>> On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
>>>>>    > Patch series [8] was rebased on recent "staging branch"
>>>>>> (5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is
>>>>>> unmapped) and
>>>>>> tested on
>>>>>> Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with virtio-mmio disk
>>>>>> backend [9]
>>>>>> running in driver domain and unmodified Linux Guest running on
>>>>>> existing
>>>>>> virtio-blk driver (frontend). No issues were observed. Guest domain
>>>>>> 'reboot/destroy'
>>>>>> use-cases work properly. Patch series was only build-tested on x86.
>>>>>
>>>>> I have done basic testing with a Linux guest on x86 and I didn't
>>>>> spot any
>>>>> regression.
>>>>>
>>>>> Unfortunately, I don't have a Windows VM in hand, so I can't confirm
>>>>> if there
>>>>> is no regression there. Can anyone give a try?
>>>>>
>>>>> On a separate topic, Andrew expressed on IRC some concern to expose 
>>>>> the
>>>>> bufioreq interface to other arch than x86. I will let him explain
>>>>> his view
>>>>> here.
>>>>>
>>>>> Given that IOREQ will be a tech preview on Arm (this implies the
>>>>> interface is
>>>>> not stable) on Arm, I think we can defer the discussion past the
>>>>> freeze.
>>>>
>>>> Yes, I agree that we can defer the discussion.
>>>>
>>>>
>>>>> For now, I would suggest to check if a Device Model is trying to
>>>>> create an
>>>>> IOREQ server with bufioreq is enabled (see ioreq_server_create()).
>>>>> This would
>>>>> not alleviate Andrew's concern, however it would at allow us to
>>>>> judge whether
>>>>> the buffering concept is going to be used on Arm (I can see some use
>>>>> for the
>>>>> Virtio doorbell).
>>>>>
>>>>> What do others think?
>>>>
>>>> Difficult to say. We don't have any uses today but who knows in the
>>>> future.
>>>
>>> I have some ideas, but Andrew so far objects on using the existing
>>> bufioreq interface for good reasons. It is using guest_cmpxchg() which
>>> is really expensive.
>>
>> Worse.  Patch 5 needs to switch cmpxchg() to guest_cmpxchg() to avoid
>> reintroducing XSA-295, but doesn't.
> 
> The memory is only shared with the emulator (we don't allow the legacy 
> interface on Arm). So why do you think it is re-introducing XSA-295?

Just for clarification, the swithc to guest_cmpxchg() is done as part of 
patch #13.

Cheers,

-- 
Julien Grall

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Julien Grall]

Hi Andrew,

On 28/01/2021 18:10, Andrew Cooper wrote:
> On 28/01/2021 17:44, Julien Grall wrote:
>>
>>
>> On 28/01/2021 17:24, Stefano Stabellini wrote:
>>> On Thu, 28 Jan 2021, Julien Grall wrote:
>>>> On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
>>>>    > Patch series [8] was rebased on recent "staging branch"
>>>>> (5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is
>>>>> unmapped) and
>>>>> tested on
>>>>> Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with virtio-mmio disk
>>>>> backend [9]
>>>>> running in driver domain and unmodified Linux Guest running on
>>>>> existing
>>>>> virtio-blk driver (frontend). No issues were observed. Guest domain
>>>>> 'reboot/destroy'
>>>>> use-cases work properly. Patch series was only build-tested on x86.
>>>>
>>>> I have done basic testing with a Linux guest on x86 and I didn't
>>>> spot any
>>>> regression.
>>>>
>>>> Unfortunately, I don't have a Windows VM in hand, so I can't confirm
>>>> if there
>>>> is no regression there. Can anyone give a try?
>>>>
>>>> On a separate topic, Andrew expressed on IRC some concern to expose the
>>>> bufioreq interface to other arch than x86. I will let him explain
>>>> his view
>>>> here.
>>>>
>>>> Given that IOREQ will be a tech preview on Arm (this implies the
>>>> interface is
>>>> not stable) on Arm, I think we can defer the discussion past the
>>>> freeze.
>>>
>>> Yes, I agree that we can defer the discussion.
>>>
>>>
>>>> For now, I would suggest to check if a Device Model is trying to
>>>> create an
>>>> IOREQ server with bufioreq is enabled (see ioreq_server_create()).
>>>> This would
>>>> not alleviate Andrew's concern, however it would at allow us to
>>>> judge whether
>>>> the buffering concept is going to be used on Arm (I can see some use
>>>> for the
>>>> Virtio doorbell).
>>>>
>>>> What do others think?
>>>
>>> Difficult to say. We don't have any uses today but who knows in the
>>> future.
>>
>> I have some ideas, but Andrew so far objects on using the existing
>> bufioreq interface for good reasons. It is using guest_cmpxchg() which
>> is really expensive.
> 
> Worse.  Patch 5 needs to switch cmpxchg() to guest_cmpxchg() to avoid
> reintroducing XSA-295, but doesn't.

The memory is only shared with the emulator (we don't allow the legacy 
interface on Arm). So why do you think it is re-introducing XSA-295?

Cheers,

-- 
Julien Grall

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Andrew Cooper]

On 28/01/2021 17:44, Julien Grall wrote:
>
>
> On 28/01/2021 17:24, Stefano Stabellini wrote:
>> On Thu, 28 Jan 2021, Julien Grall wrote:
>>> On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
>>>   > Patch series [8] was rebased on recent "staging branch"
>>>> (5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is
>>>> unmapped) and
>>>> tested on
>>>> Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with virtio-mmio disk
>>>> backend [9]
>>>> running in driver domain and unmodified Linux Guest running on
>>>> existing
>>>> virtio-blk driver (frontend). No issues were observed. Guest domain
>>>> 'reboot/destroy'
>>>> use-cases work properly. Patch series was only build-tested on x86.
>>>
>>> I have done basic testing with a Linux guest on x86 and I didn't
>>> spot any
>>> regression.
>>>
>>> Unfortunately, I don't have a Windows VM in hand, so I can't confirm
>>> if there
>>> is no regression there. Can anyone give a try?
>>>
>>> On a separate topic, Andrew expressed on IRC some concern to expose the
>>> bufioreq interface to other arch than x86. I will let him explain
>>> his view
>>> here.
>>>
>>> Given that IOREQ will be a tech preview on Arm (this implies the
>>> interface is
>>> not stable) on Arm, I think we can defer the discussion past the
>>> freeze.
>>
>> Yes, I agree that we can defer the discussion.
>>
>>
>>> For now, I would suggest to check if a Device Model is trying to
>>> create an
>>> IOREQ server with bufioreq is enabled (see ioreq_server_create()).
>>> This would
>>> not alleviate Andrew's concern, however it would at allow us to
>>> judge whether
>>> the buffering concept is going to be used on Arm (I can see some use
>>> for the
>>> Virtio doorbell).
>>>
>>> What do others think?
>>
>> Difficult to say. We don't have any uses today but who knows in the
>> future.
>
> I have some ideas, but Andrew so far objects on using the existing
> bufioreq interface for good reasons. It is using guest_cmpxchg() which
> is really expensive.

Worse.  Patch 5 needs to switch cmpxchg() to guest_cmpxchg() to avoid
reintroducing XSA-295, but doesn't.

>
>>
>> Maybe for now the important thing is to clarify that the bufioreq
>> interface won't be maintain for backward compatibility on ARM, and could
>> be removed without warnings, at least as long as the whole IOREQ feature
>> is a tech preview. >
>> In other words, it is not like we are forced to keep bufioreq around
>> forever on ARM.
>
> That's correct, we are not forced to use it. But if you only document
> it, then there is a fair chance this will split past the "Tech Preview".
>
> Today, there is no caller which requires to send buffered I/O in the
> Xen Arm hypervisor. So a Device Model should not need to say "I want
> to allocate a page and event channel for the buffered I/O".
>
> The check I suggested serves two purposes:
>   1) Catch any future upstream user of bufioreq
>   2) Avoid an interface to be marked supported by mistake

"use bufioreq" is an input to create_ioreq_server.  The easiest fix in
the short term is "if ( !IS_ENABLED(CONFIG_X86) && bufioreq ) return
-EINVAL;"

The key problem with the bufioreq interface is that it is a ring with a
non-power-of-2 number of entries.  See c/s b7007bc6f9a45 if the
implications of a non-power-of-2 number of entries aren't immediately clear.

~Andrew

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Oleksandr]

On 28.01.21 17:14, Julien Grall wrote:

Hi Julien

> Hi,
>
> On 28/01/2021 14:37, Oleksandr wrote:
>> On 27.01.21 19:45, Oleksandr wrote:
>>>
>>> On 27.01.21 19:42, Julien Grall wrote:
>>>
>>> Hi
>>>
>>>>
>>>>
>>>> On 27/01/2021 17:37, Oleksandr wrote:
>>>>>
>>>>> On 27.01.21 19:33, Julien Grall wrote:
>>>>>
>>>>> Hi Julien
>>>>>
>>>>>>
>>>>>>
>>>>>> On 27/01/2021 16:50, Oleksandr wrote:
>>>>>>>
>>>>>>> On 27.01.21 18:43, Julien Grall wrote:
>>>>>>>> Hi Oleksandr,
>>>>>>>
>>>>>>> Hi Julien
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
>>>>>>>>> ***
>>>>>>>>>
>>>>>>>>> Patch series [8] was rebased on recent "staging branch"
>>>>>>>>> (5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is 
>>>>>>>>> unmapped) and tested on
>>>>>>>>> Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with 
>>>>>>>>> virtio-mmio disk backend [9]
>>>>>>>>> running in driver domain and unmodified Linux Guest running on 
>>>>>>>>> existing
>>>>>>>>> virtio-blk driver (frontend). No issues were observed. Guest 
>>>>>>>>> domain 'reboot/destroy'
>>>>>>>>> use-cases work properly. Patch series was only build-tested on 
>>>>>>>>> x86.
>>>>>>>>>
>>>>>>>>> Please note, build-test passed for the following modes:
>>>>>>>>> 1. x86: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y (default)
>>>>>>>>> 2. x86: #CONFIG_HVM is not set / #CONFIG_IOREQ_SERVER is not set
>>>>>>>>> 3. Arm64: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
>>>>>>>>> 4. Arm64: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set 
>>>>>>>>> (default)
>>>>>>>>> 5. Arm32: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
>>>>>>>>> 6. Arm32: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set 
>>>>>>>>> (default)
>>>>>>>>
>>>>>>>> I thought I woudl give a try to test the code, but I can't find 
>>>>>>>> a way to enable CONFIG_IOREQ_SERVER from the UI.
>>>>>>>>
>>>>>>>> Looking at the Kconfig, it looks like CONFIG_IOREQ_SERVER 
>>>>>>>> doesn't have a prompt and is not selected by Arm.
>>>>>>>>
>>>>>>>> Can you provide details how this can be built on Arm?
>>>>>>>
>>>>>>> Please apply the attached patch to select IOREQ on Arm.
>>>>>>
>>>>>> This is roughly what I wrote. I think a user should be able to 
>>>>>> select IOREQ via the menuconfig without any additional patch on 
>>>>>> top of your series.
>>>>>>
>>>>>> Can you include a patch that would enable that?
>>>>>
>>>>> Yes, do you prefer a separate patch or required changes could be 
>>>>> folded in patch #14?
>>>>
>>>> I would do a separate patch as IOREQ only really work after the 
>>>> full series applies.
>>>
>>>
>>> Makes sense, I will do it for V6
>>
>>
>> Could we please negotiate *the last posting time* for me to able to 
>> prepare and push V6 not later than it?
>
> The feature freeze is going to be happen tomorrow evening. I can't 
> give you as specific time, but it is probably best if you respin v6 by 
> tomorrow morning. This should give us some slack for any last minutes 
> issues (if any).

I got it, will try


-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Julien Grall]

On 28/01/2021 17:24, Stefano Stabellini wrote:
> On Thu, 28 Jan 2021, Julien Grall wrote:
>> On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
>>   > Patch series [8] was rebased on recent "staging branch"
>>> (5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is unmapped) and
>>> tested on
>>> Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with virtio-mmio disk
>>> backend [9]
>>> running in driver domain and unmodified Linux Guest running on existing
>>> virtio-blk driver (frontend). No issues were observed. Guest domain
>>> 'reboot/destroy'
>>> use-cases work properly. Patch series was only build-tested on x86.
>>
>> I have done basic testing with a Linux guest on x86 and I didn't spot any
>> regression.
>>
>> Unfortunately, I don't have a Windows VM in hand, so I can't confirm if there
>> is no regression there. Can anyone give a try?
>>
>> On a separate topic, Andrew expressed on IRC some concern to expose the
>> bufioreq interface to other arch than x86. I will let him explain his view
>> here.
>>
>> Given that IOREQ will be a tech preview on Arm (this implies the interface is
>> not stable) on Arm, I think we can defer the discussion past the freeze.
> 
> Yes, I agree that we can defer the discussion.
> 
> 
>> For now, I would suggest to check if a Device Model is trying to create an
>> IOREQ server with bufioreq is enabled (see ioreq_server_create()). This would
>> not alleviate Andrew's concern, however it would at allow us to judge whether
>> the buffering concept is going to be used on Arm (I can see some use for the
>> Virtio doorbell).
>>
>> What do others think?
> 
> Difficult to say. We don't have any uses today but who knows in the
> future.

I have some ideas, but Andrew so far objects on using the existing 
bufioreq interface for good reasons. It is using guest_cmpxchg() which 
is really expensive.

> 
> Maybe for now the important thing is to clarify that the bufioreq
> interface won't be maintain for backward compatibility on ARM, and could
> be removed without warnings, at least as long as the whole IOREQ feature
> is a tech preview. >
> In other words, it is not like we are forced to keep bufioreq around
> forever on ARM.

That's correct, we are not forced to use it. But if you only document 
it, then there is a fair chance this will split past the "Tech Preview".

Today, there is no caller which requires to send buffered I/O in the Xen 
Arm hypervisor. So a Device Model should not need to say "I want to 
allocate a page and event channel for the buffered I/O".

The check I suggested serves two purposes:
   1) Catch any future upstream user of bufioreq
   2) Avoid an interface to be marked supported by mistake

Cheers,

-- 
Julien Grall

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Stefano Stabellini]

On Thu, 28 Jan 2021, Julien Grall wrote:
> On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
>  > Patch series [8] was rebased on recent "staging branch"
> > (5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is unmapped) and
> > tested on
> > Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with virtio-mmio disk
> > backend [9]
> > running in driver domain and unmodified Linux Guest running on existing
> > virtio-blk driver (frontend). No issues were observed. Guest domain
> > 'reboot/destroy'
> > use-cases work properly. Patch series was only build-tested on x86.
> 
> I have done basic testing with a Linux guest on x86 and I didn't spot any
> regression.
> 
> Unfortunately, I don't have a Windows VM in hand, so I can't confirm if there
> is no regression there. Can anyone give a try?
> 
> On a separate topic, Andrew expressed on IRC some concern to expose the
> bufioreq interface to other arch than x86. I will let him explain his view
> here.
> 
> Given that IOREQ will be a tech preview on Arm (this implies the interface is
> not stable) on Arm, I think we can defer the discussion past the freeze.

Yes, I agree that we can defer the discussion.


> For now, I would suggest to check if a Device Model is trying to create an
> IOREQ server with bufioreq is enabled (see ioreq_server_create()). This would
> not alleviate Andrew's concern, however it would at allow us to judge whether
> the buffering concept is going to be used on Arm (I can see some use for the
> Virtio doorbell).
> 
> What do others think?

Difficult to say. We don't have any uses today but who knows in the
future.

Maybe for now the important thing is to clarify that the bufioreq
interface won't be maintain for backward compatibility on ARM, and could
be removed without warnings, at least as long as the whole IOREQ feature
is a tech preview.

In other words, it is not like we are forced to keep bufioreq around
forever on ARM.

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Julien Grall]

Hi,

On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
  > Patch series [8] was rebased on recent "staging branch"
> (5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is unmapped) and tested on
> Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with virtio-mmio disk backend [9]
> running in driver domain and unmodified Linux Guest running on existing
> virtio-blk driver (frontend). No issues were observed. Guest domain 'reboot/destroy'
> use-cases work properly. Patch series was only build-tested on x86.

I have done basic testing with a Linux guest on x86 and I didn't spot 
any regression.

Unfortunately, I don't have a Windows VM in hand, so I can't confirm if 
there is no regression there. Can anyone give a try?

On a separate topic, Andrew expressed on IRC some concern to expose the 
bufioreq interface to other arch than x86. I will let him explain his 
view here.

Given that IOREQ will be a tech preview on Arm (this implies the 
interface is not stable) on Arm, I think we can defer the discussion 
past the freeze.

For now, I would suggest to check if a Device Model is trying to create 
an IOREQ server with bufioreq is enabled (see ioreq_server_create()). 
This would not alleviate Andrew's concern, however it would at allow us 
to judge whether the buffering concept is going to be used on Arm (I can 
see some use for the Virtio doorbell).

What do others think?

Cheers,

-- 
Julien Grall

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Julien Grall]

Hi,

On 28/01/2021 14:37, Oleksandr wrote:
> On 27.01.21 19:45, Oleksandr wrote:
>>
>> On 27.01.21 19:42, Julien Grall wrote:
>>
>> Hi
>>
>>>
>>>
>>> On 27/01/2021 17:37, Oleksandr wrote:
>>>>
>>>> On 27.01.21 19:33, Julien Grall wrote:
>>>>
>>>> Hi Julien
>>>>
>>>>>
>>>>>
>>>>> On 27/01/2021 16:50, Oleksandr wrote:
>>>>>>
>>>>>> On 27.01.21 18:43, Julien Grall wrote:
>>>>>>> Hi Oleksandr,
>>>>>>
>>>>>> Hi Julien
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
>>>>>>>> ***
>>>>>>>>
>>>>>>>> Patch series [8] was rebased on recent "staging branch"
>>>>>>>> (5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is 
>>>>>>>> unmapped) and tested on
>>>>>>>> Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with virtio-mmio 
>>>>>>>> disk backend [9]
>>>>>>>> running in driver domain and unmodified Linux Guest running on 
>>>>>>>> existing
>>>>>>>> virtio-blk driver (frontend). No issues were observed. Guest 
>>>>>>>> domain 'reboot/destroy'
>>>>>>>> use-cases work properly. Patch series was only build-tested on x86.
>>>>>>>>
>>>>>>>> Please note, build-test passed for the following modes:
>>>>>>>> 1. x86: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y (default)
>>>>>>>> 2. x86: #CONFIG_HVM is not set / #CONFIG_IOREQ_SERVER is not set
>>>>>>>> 3. Arm64: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
>>>>>>>> 4. Arm64: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set (default)
>>>>>>>> 5. Arm32: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
>>>>>>>> 6. Arm32: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set (default)
>>>>>>>
>>>>>>> I thought I woudl give a try to test the code, but I can't find a 
>>>>>>> way to enable CONFIG_IOREQ_SERVER from the UI.
>>>>>>>
>>>>>>> Looking at the Kconfig, it looks like CONFIG_IOREQ_SERVER doesn't 
>>>>>>> have a prompt and is not selected by Arm.
>>>>>>>
>>>>>>> Can you provide details how this can be built on Arm?
>>>>>>
>>>>>> Please apply the attached patch to select IOREQ on Arm.
>>>>>
>>>>> This is roughly what I wrote. I think a user should be able to 
>>>>> select IOREQ via the menuconfig without any additional patch on top 
>>>>> of your series.
>>>>>
>>>>> Can you include a patch that would enable that?
>>>>
>>>> Yes, do you prefer a separate patch or required changes could be 
>>>> folded in patch #14?
>>>
>>> I would do a separate patch as IOREQ only really work after the full 
>>> series applies.
>>
>>
>> Makes sense, I will do it for V6
> 
> 
> Could we please negotiate *the last posting time* for me to able to 
> prepare and push V6 not later than it?

The feature freeze is going to be happen tomorrow evening. I can't give 
you as specific time, but it is probably best if you respin v6 by 
tomorrow morning. This should give us some slack for any last minutes 
issues (if any).

Cheers,

-- 
Julien Grall

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Oleksandr]

Hi Julien, all


On 27.01.21 19:45, Oleksandr wrote:
>
> On 27.01.21 19:42, Julien Grall wrote:
>
> Hi
>
>>
>>
>> On 27/01/2021 17:37, Oleksandr wrote:
>>>
>>> On 27.01.21 19:33, Julien Grall wrote:
>>>
>>> Hi Julien
>>>
>>>>
>>>>
>>>> On 27/01/2021 16:50, Oleksandr wrote:
>>>>>
>>>>> On 27.01.21 18:43, Julien Grall wrote:
>>>>>> Hi Oleksandr,
>>>>>
>>>>> Hi Julien
>>>>>
>>>>>
>>>>>>
>>>>>> On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
>>>>>>> ***
>>>>>>>
>>>>>>> Patch series [8] was rebased on recent "staging branch"
>>>>>>> (5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is 
>>>>>>> unmapped) and tested on
>>>>>>> Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with virtio-mmio 
>>>>>>> disk backend [9]
>>>>>>> running in driver domain and unmodified Linux Guest running on 
>>>>>>> existing
>>>>>>> virtio-blk driver (frontend). No issues were observed. Guest 
>>>>>>> domain 'reboot/destroy'
>>>>>>> use-cases work properly. Patch series was only build-tested on x86.
>>>>>>>
>>>>>>> Please note, build-test passed for the following modes:
>>>>>>> 1. x86: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y (default)
>>>>>>> 2. x86: #CONFIG_HVM is not set / #CONFIG_IOREQ_SERVER is not set
>>>>>>> 3. Arm64: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
>>>>>>> 4. Arm64: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set (default)
>>>>>>> 5. Arm32: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
>>>>>>> 6. Arm32: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set (default)
>>>>>>
>>>>>> I thought I woudl give a try to test the code, but I can't find a 
>>>>>> way to enable CONFIG_IOREQ_SERVER from the UI.
>>>>>>
>>>>>> Looking at the Kconfig, it looks like CONFIG_IOREQ_SERVER doesn't 
>>>>>> have a prompt and is not selected by Arm.
>>>>>>
>>>>>> Can you provide details how this can be built on Arm?
>>>>>
>>>>> Please apply the attached patch to select IOREQ on Arm.
>>>>
>>>> This is roughly what I wrote. I think a user should be able to 
>>>> select IOREQ via the menuconfig without any additional patch on top 
>>>> of your series.
>>>>
>>>> Can you include a patch that would enable that?
>>>
>>> Yes, do you prefer a separate patch or required changes could be 
>>> folded in patch #14?
>>
>> I would do a separate patch as IOREQ only really work after the full 
>> series applies.
>
>
> Makes sense, I will do it for V6


Could we please negotiate *the last posting time* for me to able to 
prepare and push V6 not later than it?


>
>
>
-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Oleksandr]

On 27.01.21 19:42, Julien Grall wrote:

Hi

>
>
> On 27/01/2021 17:37, Oleksandr wrote:
>>
>> On 27.01.21 19:33, Julien Grall wrote:
>>
>> Hi Julien
>>
>>>
>>>
>>> On 27/01/2021 16:50, Oleksandr wrote:
>>>>
>>>> On 27.01.21 18:43, Julien Grall wrote:
>>>>> Hi Oleksandr,
>>>>
>>>> Hi Julien
>>>>
>>>>
>>>>>
>>>>> On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
>>>>>> ***
>>>>>>
>>>>>> Patch series [8] was rebased on recent "staging branch"
>>>>>> (5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is 
>>>>>> unmapped) and tested on
>>>>>> Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with virtio-mmio 
>>>>>> disk backend [9]
>>>>>> running in driver domain and unmodified Linux Guest running on 
>>>>>> existing
>>>>>> virtio-blk driver (frontend). No issues were observed. Guest 
>>>>>> domain 'reboot/destroy'
>>>>>> use-cases work properly. Patch series was only build-tested on x86.
>>>>>>
>>>>>> Please note, build-test passed for the following modes:
>>>>>> 1. x86: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y (default)
>>>>>> 2. x86: #CONFIG_HVM is not set / #CONFIG_IOREQ_SERVER is not set
>>>>>> 3. Arm64: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
>>>>>> 4. Arm64: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set (default)
>>>>>> 5. Arm32: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
>>>>>> 6. Arm32: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set (default)
>>>>>
>>>>> I thought I woudl give a try to test the code, but I can't find a 
>>>>> way to enable CONFIG_IOREQ_SERVER from the UI.
>>>>>
>>>>> Looking at the Kconfig, it looks like CONFIG_IOREQ_SERVER doesn't 
>>>>> have a prompt and is not selected by Arm.
>>>>>
>>>>> Can you provide details how this can be built on Arm?
>>>>
>>>> Please apply the attached patch to select IOREQ on Arm.
>>>
>>> This is roughly what I wrote. I think a user should be able to 
>>> select IOREQ via the menuconfig without any additional patch on top 
>>> of your series.
>>>
>>> Can you include a patch that would enable that?
>>
>> Yes, do you prefer a separate patch or required changes could be 
>> folded in patch #14?
>
> I would do a separate patch as IOREQ only really work after the full 
> series applies.


Makes sense, I will do it for V6


-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Julien Grall]

On 27/01/2021 17:37, Oleksandr wrote:
> 
> On 27.01.21 19:33, Julien Grall wrote:
> 
> Hi Julien
> 
>>
>>
>> On 27/01/2021 16:50, Oleksandr wrote:
>>>
>>> On 27.01.21 18:43, Julien Grall wrote:
>>>> Hi Oleksandr,
>>>
>>> Hi Julien
>>>
>>>
>>>>
>>>> On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
>>>>> ***
>>>>>
>>>>> Patch series [8] was rebased on recent "staging branch"
>>>>> (5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is 
>>>>> unmapped) and tested on
>>>>> Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with virtio-mmio 
>>>>> disk backend [9]
>>>>> running in driver domain and unmodified Linux Guest running on 
>>>>> existing
>>>>> virtio-blk driver (frontend). No issues were observed. Guest domain 
>>>>> 'reboot/destroy'
>>>>> use-cases work properly. Patch series was only build-tested on x86.
>>>>>
>>>>> Please note, build-test passed for the following modes:
>>>>> 1. x86: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y (default)
>>>>> 2. x86: #CONFIG_HVM is not set / #CONFIG_IOREQ_SERVER is not set
>>>>> 3. Arm64: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
>>>>> 4. Arm64: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set (default)
>>>>> 5. Arm32: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
>>>>> 6. Arm32: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set (default)
>>>>
>>>> I thought I woudl give a try to test the code, but I can't find a 
>>>> way to enable CONFIG_IOREQ_SERVER from the UI.
>>>>
>>>> Looking at the Kconfig, it looks like CONFIG_IOREQ_SERVER doesn't 
>>>> have a prompt and is not selected by Arm.
>>>>
>>>> Can you provide details how this can be built on Arm?
>>>
>>> Please apply the attached patch to select IOREQ on Arm.
>>
>> This is roughly what I wrote. I think a user should be able to select 
>> IOREQ via the menuconfig without any additional patch on top of your 
>> series.
>>
>> Can you include a patch that would enable that?
> 
> Yes, do you prefer a separate patch or required changes could be folded 
> in patch #14?

I would do a separate patch as IOREQ only really work after the full 
series applies.

Cheers,

-- 
Julien Grall

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Oleksandr]

On 27.01.21 19:33, Julien Grall wrote:

Hi Julien

>
>
> On 27/01/2021 16:50, Oleksandr wrote:
>>
>> On 27.01.21 18:43, Julien Grall wrote:
>>> Hi Oleksandr,
>>
>> Hi Julien
>>
>>
>>>
>>> On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
>>>> ***
>>>>
>>>> Patch series [8] was rebased on recent "staging branch"
>>>> (5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is 
>>>> unmapped) and tested on
>>>> Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with virtio-mmio 
>>>> disk backend [9]
>>>> running in driver domain and unmodified Linux Guest running on 
>>>> existing
>>>> virtio-blk driver (frontend). No issues were observed. Guest domain 
>>>> 'reboot/destroy'
>>>> use-cases work properly. Patch series was only build-tested on x86.
>>>>
>>>> Please note, build-test passed for the following modes:
>>>> 1. x86: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y (default)
>>>> 2. x86: #CONFIG_HVM is not set / #CONFIG_IOREQ_SERVER is not set
>>>> 3. Arm64: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
>>>> 4. Arm64: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set (default)
>>>> 5. Arm32: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
>>>> 6. Arm32: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set (default)
>>>
>>> I thought I woudl give a try to test the code, but I can't find a 
>>> way to enable CONFIG_IOREQ_SERVER from the UI.
>>>
>>> Looking at the Kconfig, it looks like CONFIG_IOREQ_SERVER doesn't 
>>> have a prompt and is not selected by Arm.
>>>
>>> Can you provide details how this can be built on Arm?
>>
>> Please apply the attached patch to select IOREQ on Arm.
>
> This is roughly what I wrote. I think a user should be able to select 
> IOREQ via the menuconfig without any additional patch on top of your 
> series.
>
> Can you include a patch that would enable that?

Yes, do you prefer a separate patch or required changes could be folded 
in patch #14?


>
>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Julien Grall]

On 27/01/2021 16:50, Oleksandr wrote:
> 
> On 27.01.21 18:43, Julien Grall wrote:
>> Hi Oleksandr,
> 
> Hi Julien
> 
> 
>>
>> On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
>>> ***
>>>
>>> Patch series [8] was rebased on recent "staging branch"
>>> (5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is unmapped) 
>>> and tested on
>>> Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with virtio-mmio disk 
>>> backend [9]
>>> running in driver domain and unmodified Linux Guest running on existing
>>> virtio-blk driver (frontend). No issues were observed. Guest domain 
>>> 'reboot/destroy'
>>> use-cases work properly. Patch series was only build-tested on x86.
>>>
>>> Please note, build-test passed for the following modes:
>>> 1. x86: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y (default)
>>> 2. x86: #CONFIG_HVM is not set / #CONFIG_IOREQ_SERVER is not set
>>> 3. Arm64: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
>>> 4. Arm64: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set (default)
>>> 5. Arm32: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
>>> 6. Arm32: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set (default)
>>
>> I thought I woudl give a try to test the code, but I can't find a way 
>> to enable CONFIG_IOREQ_SERVER from the UI.
>>
>> Looking at the Kconfig, it looks like CONFIG_IOREQ_SERVER doesn't have 
>> a prompt and is not selected by Arm.
>>
>> Can you provide details how this can be built on Arm?
> 
> Please apply the attached patch to select IOREQ on Arm.

This is roughly what I wrote. I think a user should be able to select 
IOREQ via the menuconfig without any additional patch on top of your series.

Can you include a patch that would enable that?

Cheers,

-- 
Julien Grall

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Oleksandr]

[-- Attachment #1: Type: text/plain, Size: 1425 bytes --]


On 27.01.21 18:43, Julien Grall wrote:
> Hi Oleksandr,

Hi Julien


>
> On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
>> ***
>>
>> Patch series [8] was rebased on recent "staging branch"
>> (5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is unmapped) 
>> and tested on
>> Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with virtio-mmio disk 
>> backend [9]
>> running in driver domain and unmodified Linux Guest running on existing
>> virtio-blk driver (frontend). No issues were observed. Guest domain 
>> 'reboot/destroy'
>> use-cases work properly. Patch series was only build-tested on x86.
>>
>> Please note, build-test passed for the following modes:
>> 1. x86: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y (default)
>> 2. x86: #CONFIG_HVM is not set / #CONFIG_IOREQ_SERVER is not set
>> 3. Arm64: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
>> 4. Arm64: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set (default)
>> 5. Arm32: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
>> 6. Arm32: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set (default)
>
> I thought I woudl give a try to test the code, but I can't find a way 
> to enable CONFIG_IOREQ_SERVER from the UI.
>
> Looking at the Kconfig, it looks like CONFIG_IOREQ_SERVER doesn't have 
> a prompt and is not selected by Arm.
>
> Can you provide details how this can be built on Arm?

Please apply the attached patch to select IOREQ on Arm.



-- 
Regards,

Oleksandr Tyshchenko


[-- Attachment #2: 0001-NOT-FOR-MERGE-enable-ioreq.patch --]
[-- Type: text/x-patch, Size: 656 bytes --]

From c0ef23f7f0788783f9fccbf1e4e4935711de67ea Mon Sep 17 00:00:00 2001
From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Date: Thu, 8 Oct 2020 22:30:24 +0300
Subject: [PATCH] [NOT FOR MERGE] enable ioreq

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 xen/arch/arm/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index c3eb13e..5cf1e84 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -21,6 +21,7 @@ config ARM
 	select HAS_PASSTHROUGH
 	select HAS_PDX
 	select IOMMU_FORCE_PT_SHARE
+	select IOREQ_SERVER
 
 config ARCH_DEFCONFIG
 	string
-- 
2.7.4

Re: [PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Julien Grall]

Hi Oleksandr,

On 25/01/2021 19:08, Oleksandr Tyshchenko wrote:
> ***
> 
> Patch series [8] was rebased on recent "staging branch"
> (5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is unmapped) and tested on
> Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with virtio-mmio disk backend [9]
> running in driver domain and unmodified Linux Guest running on existing
> virtio-blk driver (frontend). No issues were observed. Guest domain 'reboot/destroy'
> use-cases work properly. Patch series was only build-tested on x86.
> 
> Please note, build-test passed for the following modes:
> 1. x86: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y (default)
> 2. x86: #CONFIG_HVM is not set / #CONFIG_IOREQ_SERVER is not set
> 3. Arm64: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
> 4. Arm64: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set  (default)
> 5. Arm32: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
> 6. Arm32: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set  (default)

I thought I woudl give a try to test the code, but I can't find a way to 
enable CONFIG_IOREQ_SERVER from the UI.

Looking at the Kconfig, it looks like CONFIG_IOREQ_SERVER doesn't have a 
prompt and is not selected by Arm.

Can you provide details how this can be built on Arm?

Cheers,

-- 
Julien Grall

[PATCH V5 00/22] IOREQ feature (+ virtio-mmio) on Arm

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Hello all.

The purpose of this patch series is to add IOREQ/DM support to Xen on Arm.
You can find an initial discussion at [1] and RFC-V4 series at [2]-[6].
Xen on Arm requires some implementation to forward guest MMIO access to a device
model in order to implement virtio-mmio backend or even mediator outside of hypervisor.
As Xen on x86 already contains required support this series tries to make it common
and introduce Arm specific bits plus some new functionality. Patch series is based on
Julien's PoC "xen/arm: Add support for Guest IO forwarding to a device emulator".

***

IMPORTANT NOTE:

Current patch series doesn't contain VirtIO related changes for the toolstack
(but they are still available at the GitHub repo [8]):
- libxl: Introduce basic virtio-mmio support on Arm
- [RFC] libxl: Add support for virtio-disk configuration
I decided to skip these patches for now since they require some rework (not Xen 4.15 materials),
I will resume pushing them once we get *common* IOREQ in.  

***

According to the initial/subsequent discussions there are a few open
questions/concerns regarding security, performance in VirtIO solution:
1. virtio-mmio vs virtio-pci, SPI vs MSI, or even a composition of virtio-mmio + MSI, 
   different use-cases require different transport...
2. virtio backend is able to access all guest memory, some kind of protection
   is needed: 'virtio-iommu in Xen' vs 'pre-shared-memory & memcpys in guest', etc
   (for these Alex have provided some input at [7])
3. interface between toolstack and 'out-of-qemu' virtio backend, avoid using
   Xenstore in virtio backend if possible. Also, there is a desire to make VirtIO
   backend hypervisor-agnostic.
4. a lot of 'foreing mapping' could lead to the memory exhaustion at the host side,
   as we are stealing the page from host memory in order to map the guest page.
   Julien has some idea regarding that.
5. Julien also has some ideas how to optimize the IOREQ code:
   5.1 vcpu_ioreq_handle_completion (former handle_hvm_io_completion) which is called in
       an hotpath on Arm (everytime we are re-entering to the guest):
       Ideally, vcpu_ioreq_handle_completion should be a NOP (at max a few instructions)
       if there is nothing to do (if we don't have I/O forwarded to an IOREQ server).
       Maybe we want to introduce a per-vCPU flag indicating if an I/O has been
       forwarded to an IOREQ server. This would allow us to bypass most of the function
       if there is nothing to do.
   5.2 The current way to handle MMIO is the following:
       - Pause the vCPU
       - Forward the access to the backend domain
       - Schedule the backend domain
       - Wait for the access to be handled
       - Unpause the vCPU
       The sequence is going to be fairly expensive on Xen.
       It might be possible to optimize the ACK and avoid to wait for the backend
       to handle the access.

Looks like all of them are valid and worth considering, but the first thing
which we need on Arm is a mechanism to forward guest IO to a device emulator,
so let's focus on it in the first place.

There are a lot of changes since RFC series, almost all TODOs were resolved on Arm,
Arm code was improved and hardened, common IOREQ/DM code became really arch-agnostic
(without HVM-ism), the "legacy" mechanism of mapping magic pages for the IOREQ servers
was left x86 specific, etc. But one TODO still remains which is "PIO handling" on Arm.
The "PIO handling" TODO is expected to left unaddressed for the current series.
It is not an big issue for now while Xen doesn't have support for vPCI on Arm.
On Arm64 they are only used for PCI IO Bar and we would probably want to expose
them to emulator as PIO access to make a DM completely arch-agnostic. So "PIO handling"
should be implemented when we add support for vPCI.

There are patches on review this series depends on:
https://patchwork.kernel.org/patch/11816689
https://patchwork.kernel.org/patch/11803383

Please note, that IOREQ feature is disabled by default on Arm within current series.

***

Patch series [8] was rebased on recent "staging branch"
(5e31789 tools/ocaml/libs/xb: Do not crash after xenbus is unmapped) and tested on
Renesas Salvator-X board + H3 ES3.0 SoC (Arm64) with virtio-mmio disk backend [9]
running in driver domain and unmodified Linux Guest running on existing
virtio-blk driver (frontend). No issues were observed. Guest domain 'reboot/destroy'
use-cases work properly. Patch series was only build-tested on x86.

Please note, build-test passed for the following modes:
1. x86: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y (default)
2. x86: #CONFIG_HVM is not set / #CONFIG_IOREQ_SERVER is not set
3. Arm64: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
4. Arm64: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set  (default)
5. Arm32: CONFIG_HVM=y / CONFIG_IOREQ_SERVER=y
6. Arm32: CONFIG_HVM=y / #CONFIG_IOREQ_SERVER is not set  (default)

***

Any feedback/help would be highly appreciated.

[1] https://lists.xenproject.org/archives/html/xen-devel/2020-07/msg00825.html
[2] https://lists.xenproject.org/archives/html/xen-devel/2020-08/msg00071.html
[3] https://lists.xenproject.org/archives/html/xen-devel/2020-09/msg00732.html
[4] https://lists.xenproject.org/archives/html/xen-devel/2020-10/msg01077.html
[5] https://lists.xenproject.org/archives/html/xen-devel/2020-11/msg02188.html
[6] https://lists.xenproject.org/archives/html/xen-devel/2021-01/msg00749.html
[7] https://lists.xenproject.org/archives/html/xen-devel/2020-11/msg02212.html
[8] https://github.com/otyshchenko1/xen/commits/ioreq_4.14_ml6
[9] https://github.com/xen-troops/virtio-disk/commits/ioreq_ml1

Julien Grall (3):
  xen/ioreq: Make x86's IOREQ related dm-op handling common
  xen/mm: Make x86's XENMEM_resource_ioreq_server handling common
  arm/ioreq: Introduce arch specific bits for IOREQ/DM features

Oleksandr Tyshchenko (19):
  x86/ioreq: Prepare IOREQ feature for making it common
  x86/ioreq: Add IOREQ_STATUS_* #define-s and update code for moving
  x86/ioreq: Provide out-of-line wrapper for the handle_mmio()
  xen/ioreq: Make x86's IOREQ feature common
  xen/ioreq: Make x86's hvm_ioreq_needs_completion() common
  xen/ioreq: Make x86's hvm_mmio_first(last)_byte() common
  xen/ioreq: Make x86's hvm_ioreq_(page/vcpu/server) structs common
  xen/ioreq: Move x86's ioreq_server to struct domain
  xen/ioreq: Move x86's io_completion/io_req fields to struct vcpu
  xen/ioreq: Remove "hvm" prefixes from involved function names
  xen/ioreq: Use guest_cmpxchg64() instead of cmpxchg()
  xen/arm: Call vcpu_ioreq_handle_completion() in check_for_vcpu_work()
  xen/mm: Handle properly reference in set_foreign_p2m_entry() on Arm
  xen/ioreq: Introduce domain_has_ioreq_server()
  xen/dm: Introduce xendevicemodel_set_irq_level DM op
  xen/arm: io: Abstract sign-extension
  xen/arm: io: Harden sign extension check
  xen/ioreq: Make x86's send_invalidate_req() common
  xen/arm: Add mapcache invalidation handling

 MAINTAINERS                                  |    9 +-
 tools/include/xendevicemodel.h               |    4 +
 tools/libs/devicemodel/core.c                |   18 +
 tools/libs/devicemodel/libxendevicemodel.map |    1 +
 xen/arch/arm/Makefile                        |    2 +
 xen/arch/arm/dm.c                            |  149 +++
 xen/arch/arm/domain.c                        |    9 +
 xen/arch/arm/io.c                            |   30 +-
 xen/arch/arm/ioreq.c                         |  200 ++++
 xen/arch/arm/p2m.c                           |   51 +-
 xen/arch/arm/traps.c                         |   55 +-
 xen/arch/x86/Kconfig                         |    2 +-
 xen/arch/x86/hvm/dm.c                        |  134 +--
 xen/arch/x86/hvm/emulate.c                   |  220 ++--
 xen/arch/x86/hvm/hvm.c                       |   14 +-
 xen/arch/x86/hvm/hypercall.c                 |    9 +-
 xen/arch/x86/hvm/intercept.c                 |    5 +-
 xen/arch/x86/hvm/io.c                        |   52 +-
 xen/arch/x86/hvm/ioreq.c                     | 1368 ++----------------------
 xen/arch/x86/hvm/stdvga.c                    |   12 +-
 xen/arch/x86/hvm/svm/nestedsvm.c             |    2 +-
 xen/arch/x86/hvm/vmx/realmode.c              |    8 +-
 xen/arch/x86/hvm/vmx/vvmx.c                  |    5 +-
 xen/arch/x86/mm.c                            |   46 +-
 xen/arch/x86/mm/p2m.c                        |   17 +-
 xen/arch/x86/mm/shadow/common.c              |    2 +-
 xen/common/Kconfig                           |    6 +-
 xen/common/Makefile                          |    2 +
 xen/common/dm.c                              |   55 +
 xen/common/ioreq.c                           | 1426 ++++++++++++++++++++++++++
 xen/common/memory.c                          |   72 +-
 xen/include/asm-arm/domain.h                 |    2 +
 xen/include/asm-arm/ioreq.h                  |   70 ++
 xen/include/asm-arm/mmio.h                   |    1 +
 xen/include/asm-arm/p2m.h                    |   19 +-
 xen/include/asm-arm/traps.h                  |   25 +
 xen/include/asm-x86/hvm/domain.h             |   43 -
 xen/include/asm-x86/hvm/emulate.h            |    2 +-
 xen/include/asm-x86/hvm/io.h                 |   17 -
 xen/include/asm-x86/hvm/ioreq.h              |   39 +-
 xen/include/asm-x86/hvm/vcpu.h               |   18 -
 xen/include/asm-x86/ioreq.h                  |   37 +
 xen/include/asm-x86/mm.h                     |    4 -
 xen/include/asm-x86/p2m.h                    |   22 +-
 xen/include/public/hvm/dm_op.h               |   16 +
 xen/include/xen/dm.h                         |   43 +
 xen/include/xen/ioreq.h                      |  140 +++
 xen/include/xen/mm.h                         |    9 -
 xen/include/xen/p2m-common.h                 |    4 +
 xen/include/xen/sched.h                      |   34 +
 xen/include/xsm/dummy.h                      |    4 +-
 xen/include/xsm/xsm.h                        |    6 +-
 xen/xsm/dummy.c                              |    2 +-
 xen/xsm/flask/hooks.c                        |    5 +-
 54 files changed, 2712 insertions(+), 1835 deletions(-)
 create mode 100644 xen/arch/arm/dm.c
 create mode 100644 xen/arch/arm/ioreq.c
 create mode 100644 xen/common/dm.c
 create mode 100644 xen/common/ioreq.c
 create mode 100644 xen/include/asm-arm/ioreq.h
 create mode 100644 xen/include/asm-x86/ioreq.h
 create mode 100644 xen/include/xen/dm.h
 create mode 100644 xen/include/xen/ioreq.h

-- 
2.7.4