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 11:31
> 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>; StefanoStabellini <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 21.12.17 at 11:01, <Paul.Durrant@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 21 December 2017 09:47
> >> >>> 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 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.
> 
> You _need_ to, or you _want_ to?
> 

Well, yes I'd prefer to keep things as uniform as possible.

> > 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.
> 
> And is this a bad thing? It's not going to be the same code anyway
> to map ioreq server pages and grant ones, so why can't one code
> path not use DOMID_SELF for the mapping (but not the
> XENMEM_acquire_resource invocation), and the other the target
> domain's ID?
> 

I could do it that way, but I don't really like the divergence.

> I agree that exposing this sort of implementation detail is not
> very nice, but imo it's not as bad as making this a non-option.
> XENMEM_acquire_resource output could even have a flag added
> telling the caller the ownership properties, such that we could
> change the implementation later on, should that be needed.
> 

Ok, that sounds like a reasonable idea. Obviously the ownership field would only be of relevance to a PV tools domain, but I guess that's ok.

  Paul

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