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
