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

[Oleksandr <olekstysh@xxxxxxxxx>]

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