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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

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