On 21/02/19 12:45, Joao Martins wrote: > On 2/20/19 9:09 PM, Paolo Bonzini wrote: >> On 20/02/19 21:15, Joao Martins wrote: >>> 2. PV Driver support (patches 17 - 39) >>> >>> We start by redirecting hypercalls from the backend to routines >>> which emulate the behaviour that PV backends expect i.e. grant >>> table and interdomain events. Next, we add support for late >>> initialization of xenbus, followed by implementing >>> frontend/backend communication mechanisms (i.e. grant tables and >>> interdomain event channels). Finally, introduce xen-shim.ko, >>> which will setup a limited Xen environment. This uses the added >>> functionality of Xen specific shared memory (grant tables) and >>> notifications (event channels). >> >> I am a bit worried by the last patches, they seem really brittle and >> prone to breakage. I don't know Xen well enough to understand if the >> lack of support for GNTMAP_host_map is fixable, but if not, you have to >> define a completely different hypercall. >> > I guess Ankur already answered this; so just to stack this on top of his comment. > > The xen_shim_domain() is only meant to handle the case where the backend > has/can-have full access to guest memory [i.e. netback and blkback would work > with similar assumptions as vhost?]. For the normal case, where a backend *in a > guest* maps and unmaps other guest memory, this is not applicable and these > changes don't affect that case. > > IOW, the PV backend here sits on the hypervisor, and the hypercalls aren't > actual hypercalls but rather invoking shim_hypercall(). The call chain would go > more or less like: > > <netback|blkback|scsiback> > gnttab_map_refs(map_ops, pages) > HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref,...) > shim_hypercall() > shim_hcall_gntmap() > > Our reasoning was that given we are already in KVM, why mapping a page if the > user (i.e. the kernel PV backend) is himself? The lack of GNTMAP_host_map is how > the shim determines its user doesn't want to map the page. Also, there's another > issue where PV backends always need a struct page to reference the device > inflight data as Ankur pointed out. Ultimately it's up to the Xen people. It does make their API uglier, especially the in/out change for the parameter. If you can at least avoid that, it would alleviate my concerns quite a bit. Paolo
