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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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