Re: [Xen-devel] [PATCH 8/9] vm_event: Add vm_event_ng interface

[Petre Ovidiu PIRCALABU <ppircalabu@xxxxxxxxxxxxxxx>]

On Thu, 2019-06-06 at 02:37 -0600, Jan Beulich wrote:
> > > > On 05.06.19 at 19:01, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
> > 
> > On Tue, 2019-06-04 at 15:43 +0100, Andrew Cooper wrote:
> > > On 30/05/2019 15:18, Petre Pircalabu wrote:
> > > > +static int vm_event_channels_alloc_buffer(struct
> > > > vm_event_channels_domain *impl)
> > > > +{
> > > > +    int i, rc = -ENOMEM;
> > > > +
> > > > +    for ( i = 0; i < impl->nr_frames; i++ )
> > > > +    {
> > > > +        struct page_info *page = alloc_domheap_page(impl-
> > > > >ved.d,
> > > > 0);
> > > 
> > > This creates pages which are reference-able (in principle) by the
> > > guest,
> > > and are bounded by d->max_pages.
> 
> Not by a HVM one, because they can't reference pages by MFN.
> Or else, as Petre implies, the ioreq approach would be wrong, too.
> 
> > > Both of these are properties of the existing interface which we'd
> > > prefer
> > > to remove.
> > 
> > The allocation mechanism is similar with the one used by ioreq (the
> > main difference is the number of pages).
> 
> Question is whether here you want to use the "caller owned"
> variant. I haven't thought through whether this would actually
> be better, so it's merely a remark.
> 
XENMEM_rsrc_acq_caller_owned flag can only be used then the calling
domain is the hardware domain. Unfortunately for us, this is a severe
limitation as we're running the monitor client form a separate DomU.

From xen/common/memory.c :
....
/*
 * FIXME: Until foreign pages inserted into the P2M are properly
 *        reference counted, it is unsafe to allow mapping of
 *        non-caller-owned resource pages unless the caller is
 *        the hardware domain.
 */
 if ( !(xmar.flags & XENMEM_rsrc_acq_caller_owned) &&
      !is_hardware_domain(currd) )
     return -EACCES;
...
> > > > +        if ( !page )
> > > > +            goto err;
> > > > +
> > > > +        if ( !get_page_and_type(page, impl->ved.d,
> > > > PGT_writable_page) )
> > > > +        {
> > > > +            rc = -ENODATA;
> > > > +            goto err;
> > > > +        }
> > > > +
> > > > +        impl->mfn[i] = page_to_mfn(page);
> > > > +    }
> > > > +
> > > > +    impl->slots = (struct vm_event_slot *)vmap(impl->mfn,
> > > > impl-
> > > > > nr_frames);
> > > 
> > > You appear to have opencoded vmalloc() here.  Is there any reason
> > > not
> > > to
> > > use that?
> > > 
> > 
> > The problem with vmalloc is that if the pages are not assigned to a
> > specific domain the remapping fails in the monitor domain.
> > e.g.:
> > ...
> > (XEN) mm.c:1015:d0v2 pg_owner d1 l1e_owner d0, but real_pg_owner d-
> > 1
> > (XEN) mm.c:1091:d0v7 Error getting mfn 5fbf53 (pfn
> > ffffffffffffffff)
> > from L1 entry 80000005fbf53227 for l1e_owner d0, pg_owner d1
> 
> In which case maybe use vmalloc() and then assign_pages()?
> Jan
Unfortunately I wasn't able to make it work:
I replaced the buffer allocation with this code:
....
    impl->slots = vzalloc(impl->nr_frames * PAGE_SIZE);
    if ( !impl->slots )
        return -ENOMEM;

    for ( i = 0; i < impl->nr_frames; i++ )
    {
        impl->mfn[i] = vmap_to_mfn(impl->slots + i * PAGE_SIZE);
        if ( assign_pages(current->domain, mfn_to_page(impl->mfn[i]),
0, 0/*MEMF_no_refcount*/ ) )
        {
            printk("%s: assign_pages returned error\n", __func__);
        }
    }
...
And the error is similar with the one without assign_pages:
....
(XEN) mm.c:1015:d0v4 pg_owner d1 l1e_owner d0, but real_pg_owner d0
(XEN) mm.c:1091:d0v4 Error getting mfn 60deaf (pfn ffffffffffffffff)
from L1 entry 800000060deaf227 for l1e_owner d0, pg_owner d1

Am I missing something?

Many thanks,
Petre

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel