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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 21 December 2017 09:47
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: JulienGrall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; Tim (Xen.org) <tim@xxxxxxx>
> 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@xxxxxxxxxx> wrote:
> >> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On
> Behalf
> >> Of Jan Beulich
> >> Sent: 20 December 2017 16:35
> >> >>> On 15.12.17 at 11:41, <paul.durrant@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel