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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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