RE: [PATCH V2 02/23] xen/ioreq: Make x86's IOREQ feature common

From: Paul Durrant <xadimgnik@gmail.com>
To: "'Oleksandr'" <olekstysh@gmail.com>
Cc: xen-devel@lists.xenproject.org,
	"'Oleksandr Tyshchenko'" <oleksandr_tyshchenko@epam.com>,
	"'Andrew Cooper'" <andrew.cooper3@citrix.com>,
	"'George Dunlap'" <george.dunlap@citrix.com>,
	"'Ian Jackson'" <iwj@xenproject.org>,
	"'Jan Beulich'" <jbeulich@suse.com>,
	"'Julien Grall'" <julien@xen.org>,
	"'Stefano Stabellini'" <sstabellini@kernel.org>,
	"'Wei Liu'" <wl@xen.org>,
	"'Roger Pau Monné'" <roger.pau@citrix.com>,
	"'Tim Deegan'" <tim@xen.org>,
	"'Julien Grall'" <julien.grall@arm.com>
Subject: RE: [PATCH V2 02/23] xen/ioreq: Make x86's IOREQ feature common
Date: Tue, 17 Nov 2020 19:43:40 -0000	[thread overview]
Message-ID: <009601d6bd19$f379b4c0$da6d1e40$@xen.org> (raw)
In-Reply-To: <a2eecf9b-7246-68c8-aee4-b4009ee16ed8@gmail.com>

> -----Original Message-----
[snip]
> 
> Both hvm_ioreq_server_init() and hvm_ioreq_server_deinit() call "legacy"
> hvm_ioreq_server_unmap_pages()
> which we want to be abstracted. The only difference between these two
> usages is that the former calls it during rollback only (in case of error).
> Taking into the account what has been suggested for question #1 could we
> just introduce arch_ioreq_server_unmap_pages() to be called from both
> init and deinit?
> 

That sounds fine, yes.

> 
> [Not completed not tested]
> 
> @@ -762,7 +772,7 @@ static int hvm_ioreq_server_init(struct
> hvm_ioreq_server *s,
> 
>    fail_add:
>       hvm_ioreq_server_remove_all_vcpus(s);
> -    hvm_ioreq_server_unmap_pages(s);
> +    arch_ioreq_server_unmap_pages(s);
> 
>       hvm_ioreq_server_free_rangesets(s);
> 
> @@ -776,7 +786,7 @@ static void hvm_ioreq_server_deinit(struct
> hvm_ioreq_server *s)
>       hvm_ioreq_server_remove_all_vcpus(s);
> 
>       /*
> -     * NOTE: It is safe to call both hvm_ioreq_server_unmap_pages() and
> +     * NOTE: It is safe to call both arch_ioreq_server_unmap_pages() and
>        *       hvm_ioreq_server_free_pages() in that order.
>        *       This is because the former will do nothing if the pages
>        *       are not mapped, leaving the page to be freed by the latter.
> @@ -784,7 +794,7 @@ static void hvm_ioreq_server_deinit(struct
> hvm_ioreq_server *s)
>        *       the page_info pointer to NULL, meaning the latter will do
>        *       nothing.
>        */
> -    hvm_ioreq_server_unmap_pages(s);
> +    arch_ioreq_server_unmap_pages(s);
>       hvm_ioreq_server_free_pages(s);
> 
>       hvm_ioreq_server_free_rangesets(s);
> @@ -918,7 +928,7 @@ int hvm_get_ioreq_server_info(struct domain *d,
> ioservid_t id,
> 
>       if ( ioreq_gfn || bufioreq_gfn )
>       {
> -        rc = hvm_ioreq_server_map_pages(s);
> +        rc = arch_ioreq_server_map_pages(s);
>           if ( rc )
>               goto out;
>       }
> 
> 
> So looks like for leaving legacy mechanism x86 specific we need 4 new
> arch callbacks:
> 
> - arch_ioreq_server_enable
> - arch_ioreq_server_disable
> - arch_ioreq_server_map_pages
> - arch_ioreq_server_unmap_pages

Yes, that looks ok.

  Paul