Re: [PATCH v16 06/11] x86/hvm/ioreq: add a new mappable resource type...

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 21 December 2017 09:47
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: JulienGrall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel@lists.xenproject.org; Tim (Xen.org) <tim@xen.org>
> Subject: RE: [Xen-devel] [PATCH v16 06/11] x86/hvm/ioreq: add a new
> mappable resource type...
> 
> >>> On 20.12.17 at 18:02, <Paul.Durrant@citrix.com> wrote:
> >> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On
> Behalf
> >> Of Jan Beulich
> >> Sent: 20 December 2017 16:35
> >> >>> On 15.12.17 at 11:41, <paul.durrant@citrix.com> wrote:
> >> > +static int hvm_alloc_ioreq_mfn(struct hvm_ioreq_server *s, bool buf)
> >> > +{
> >> > +    struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
> >> > +
> >> > +    if ( iorp->page )
> >> > +    {
> >> > +        /*
> >> > +         * If a guest frame has already been mapped (which may happen
> >> > +         * on demand if hvm_get_ioreq_server_info() is called), then
> >> > +         * allocating a page is not permitted.
> >> > +         */
> >> > +        if ( !gfn_eq(iorp->gfn, INVALID_GFN) )
> >> > +            return -EPERM;
> >> > +
> >> > +        return 0;
> >> > +    }
> >> > +
> >> > +    iorp->va = alloc_xenheap_page();
> >> > +    if ( !iorp->va )
> >> > +        return -ENOMEM;
> >> > +
> >> > +    clear_page(iorp->va);
> >> > +
> >> > +    iorp->page = virt_to_page(iorp->va);
> >> > +    share_xen_page_with_guest(iorp->page, s->domain,
> >> XENSHARE_writable);
> >> > +    return 0;
> >> > +}
> >>
> >> Why the much more limited (on huge systems) Xen heap all of the
> >> sudden?
> >
> > Largely I'm trying to follow the same procedure used for the grant tables.
> > Also, Xen is always going to need a mapping for these pages so using
> xenheap
> > is convenient. If you think that's too limited then I can go back to domheap
> > (but for the target domain rather than the tools domain) and map the page
> > into Xen explicitly.
> 
> With the accounting problem below in mind, I think it'll be better
> to use ordinary domain pages here anyway.
> 
> >> > +static void hvm_free_ioreq_mfn(struct hvm_ioreq_server *s, bool
> buf)
> >> > +{
> >> > +    struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
> >> > +
> >> > +    if ( !iorp->page )
> >> > +        return;
> >> > +
> >> > +    iorp->page = NULL;
> >> > +
> >> > +    free_xenheap_page(iorp->va);
> >> > +    iorp->va = NULL;
> >> > +}
> >>
> >> I've looked over the code paths coming here, and I can't convince
> >> myself that any mapping that the server has established would be
> >> gone by the time the page is being freed. I'm likely (hopefully)
> >> overlooking some aspect here.
> >
> > Hmm. Maybe you're right. The lack of ref counting might be a problem. It
> was
> > so much simpler to allocate from the tools domain's heap, but the
> > restrictions in do_mmu_update() rule that out. I'm really not sure how to
> fix
> > this.
> 
> I'm afraid I don't see that particular restriction: It is the tools
> domain which wants to map the page. Owners of a page are
> permitted to map such pages (hence the removal of ownership
> in the XSA-248 fix). So I don't understand why the tools domain
> wouldn't be able to map that page if ownership is set that way,
> perhaps even without the new sub-op. In the end, the domain
> being serviced has no need to know of the page at all, it's a
> shared entity between hypervisor and ioreq server. But likely
> I'm missing some part of the whole picture here.
> 

The problem is the unification of resource mapping. Somehow, I need to reconcile grant frames and ioreq server pages. The original patches did this by using DOMID_SELF in the mmu_update hypercall and then allowing the mapping to be built to the grant frames, despite the tools domain not being the page owner, because the tools domain had privilege over the owner. That change to do_mmu_update is no longer there and so the caller would now need to know that grant frames belong to the target domain, but ioreq server frames belong to the tools domain. Thus, I think the only way to reconcile things without further change to do_mmu_update, is to have the ioreq server pages owned by the target domain but something then needs to prevent those pages being freed whilst the tools domain has them mapped. Actually, the same is true for the grant frames too come to think of it. I'm going to have to look at priv mapping again, I think, because I don't actually understand why that is in any way safe at the moment. (I assume something is taking an extra page reference somewhere during the mapping).

  Paul

> Jan


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