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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>
> Sent: 15 October 2020 17:44
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>; Andrew Cooper 
> <andrew.cooper3@xxxxxxxxxx>;
> George Dunlap <george.dunlap@xxxxxxxxxx>; Ian Jackson <iwj@xxxxxxxxxxxxxx>; 
> Jan Beulich
> <jbeulich@xxxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano Stabellini 
> <sstabellini@xxxxxxxxxx>; Wei
> Liu <wl@xxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>; Paul Durrant 
> <paul@xxxxxxx>; Tim Deegan
> <tim@xxxxxxx>; Julien Grall <julien.grall@xxxxxxx>
> Subject: [PATCH V2 02/23] xen/ioreq: Make x86's IOREQ feature common
> 
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> 
> As a lot of x86 code can be re-used on Arm later on, this patch
> moves previously prepared x86/hvm/ioreq.c to the common code.
> 
> The common IOREQ feature is supposed to be built with IOREQ_SERVER
> option enabled, which is selected for x86's config HVM for now.
> 
> In order to avoid having a gigantic patch here, the subsequent
> patches will update remaining bits in the common code step by step:
> - Make IOREQ related structs/materials common
> - Drop the "hvm" prefixes and infixes

FWIW you could tackle the naming changes in patch #1.

> - Remove layering violation by moving corresponding fields
>   out of *arch.hvm* or abstracting away accesses to them
> 
> This support is going to be used on Arm to be able run device
> emulator outside of Xen hypervisor.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> 
> ---
> Please note, this is a split/cleanup/hardening of Julien's PoC:
> "Add support for Guest IO forwarding to a device emulator"
> 
> ***
> Please note, this patch depends on the following which is
> on review:
> https://patchwork.kernel.org/patch/11816689/
> ***
> 
> Changes RFC -> V1:
>    - was split into three patches:
>      - x86/ioreq: Prepare IOREQ feature for making it common
>      - xen/ioreq: Make x86's IOREQ feature common
>      - xen/ioreq: Make x86's hvm_ioreq_needs_completion() common
>    - update MAINTAINERS file
>    - do not use a separate subdir for the IOREQ stuff, move it to:
>      - xen/common/ioreq.c
>      - xen/include/xen/ioreq.h
>    - update x86's files to include xen/ioreq.h
>    - remove unneeded headers in arch/x86/hvm/ioreq.c
>    - re-order the headers alphabetically in common/ioreq.c
>    - update common/ioreq.c according to the newly introduced arch functions:
>      arch_hvm_destroy_ioreq_server()/arch_handle_hvm_io_completion()
> 
> Changes V1 -> V2:
>    - update patch description
>    - make everything needed in the previous patch to achieve
>      a truly rename here
>    - don't include unnecessary headers from asm-x86/hvm/ioreq.h
>      and xen/ioreq.h
>    - use __XEN_IOREQ_H__ instead of __IOREQ_H__
>    - move get_ioreq_server() to common/ioreq.c
> ---
>  MAINTAINERS                     |    8 +-
>  xen/arch/x86/Kconfig            |    1 +
>  xen/arch/x86/hvm/Makefile       |    1 -
>  xen/arch/x86/hvm/ioreq.c        | 1422 
> ---------------------------------------
>  xen/arch/x86/mm.c               |    2 +-
>  xen/arch/x86/mm/shadow/common.c |    2 +-
>  xen/common/Kconfig              |    3 +
>  xen/common/Makefile             |    1 +
>  xen/common/ioreq.c              | 1422 
> +++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/hvm/ioreq.h |   39 +-
>  xen/include/xen/ioreq.h         |   71 ++
>  11 files changed, 1509 insertions(+), 1463 deletions(-)
>  delete mode 100644 xen/arch/x86/hvm/ioreq.c
>  create mode 100644 xen/common/ioreq.c
>  create mode 100644 xen/include/xen/ioreq.h
> 
[snip]
> +static void hvm_remove_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
> +
> +{
> +    struct domain *d = s->target;
> +    struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
> +
> +    if ( gfn_eq(iorp->gfn, INVALID_GFN) )
> +        return;
> +
> +    if ( guest_physmap_remove_page(d, iorp->gfn,
> +                                   page_to_mfn(iorp->page), 0) )
> +        domain_crash(d);
> +    clear_page(iorp->va);
> +}
> +
> +static int hvm_add_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
> +{
> +    struct domain *d = s->target;
> +    struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
> +    int rc;
> +
> +    if ( gfn_eq(iorp->gfn, INVALID_GFN) )
> +        return 0;
> +
> +    clear_page(iorp->va);
> +
> +    rc = guest_physmap_add_page(d, iorp->gfn,
> +                                page_to_mfn(iorp->page), 0);
> +    if ( rc == 0 )
> +        paging_mark_pfn_dirty(d, _pfn(gfn_x(iorp->gfn)));
> +
> +    return rc;
> +}
> +

The 'legacy' mechanism of mapping magic pages for ioreq servers should remain 
x86 specific I think that aspect of the code needs to remain behind and not get 
moved into common code. You could do that in arch specific calls in 
hvm_ioreq_server_enable/disable() and hvm_get_ioreq_server_info().

  Paul