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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.12.17 at 11:01, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 21 December 2017 09:47
>> >>> 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 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?

> 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 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.

Jan



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