Re: [PATCH V1 01/16] x86/ioreq: Prepare IOREQ feature for making it common

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.09.2020 22:21, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> 
> As a lot of x86 code can be re-used on Arm later on, this patch
> prepares IOREQ support before moving to the common code. This way
> we will get almost a verbatim copy for a code movement.
> 
> This support is going to be used on Arm to be able run device
> emulator outside of Xen hypervisor.

This is all fine, but you should then go on and explain what you're
doing, and why (at which point it may become obvious that it would
be more helpful to split this into a couple of steps). In particular
something as suspicious as ...

> Changes RFC -> V1:
>    - new patch, was split from:
>      "[RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common"
>    - fold the check of p->type into hvm_get_ioreq_server_range_type()
>      and make it return success/failure
>    - remove relocate_portio_handler() call from arch_hvm_ioreq_destroy()
>      in arch/x86/hvm/ioreq.c

... this (see below).

> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -170,6 +170,29 @@ static bool hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, 
> ioreq_t *p)
>      return true;
>  }
>  
> +bool arch_handle_hvm_io_completion(enum hvm_io_completion io_completion)

Do we need "handle" in here? Without it, I'd also not have to ask about
moving hvm further to the front of the name...

> @@ -836,6 +848,12 @@ int hvm_create_ioreq_server(struct domain *d, int 
> bufioreq_handling,
>      return rc;
>  }
>  
> +/* Called when target domain is paused */
> +int arch_hvm_destroy_ioreq_server(struct hvm_ioreq_server *s)
> +{
> +    return p2m_set_ioreq_server(s->target, 0, s);
> +}

Why return "int" when ...

> @@ -855,7 +873,7 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t 
> id)
>  
>      domain_pause(d);
>  
> -    p2m_set_ioreq_server(d, 0, s);
> +    arch_hvm_destroy_ioreq_server(s);

... the result has been ignored anyway? Or otherwise I guess you'd
want to add error handling here (but then the result of
p2m_set_ioreq_server() should still get ignored, for backwards
compatibility).

> @@ -1215,8 +1233,7 @@ void hvm_destroy_all_ioreq_servers(struct domain *d)
>      struct hvm_ioreq_server *s;
>      unsigned int id;
>  
> -    if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) )
> -        return;
> +    arch_hvm_ioreq_destroy(d);

So the call to relocate_portio_handler() simply goes away. No
replacement, no explanation?

> @@ -1239,19 +1256,15 @@ void hvm_destroy_all_ioreq_servers(struct domain *d)
>      spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
>  }
>  
> -struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
> -                                                 ioreq_t *p)
> +int hvm_get_ioreq_server_range_type(struct domain *d,
> +                                    ioreq_t *p,

At least p, but perhaps also d can gain const?

> +                                    uint8_t *type,
> +                                    uint64_t *addr)

By the name the function returns a type for a range (albeit without
it being clear where the two bounds of such a range actually live).
By the implementation is looks more like you mean "range_and_type",
albeit still without there really being a range passed back to the
caller. Therefore I think I need some clarification on what's
intended before even being able to suggest something. From ...

> +struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
> +                                                 ioreq_t *p)
> +{
> +    struct hvm_ioreq_server *s;
> +    uint8_t type;
> +    uint64_t addr;
> +    unsigned int id;
> +
> +    if ( hvm_get_ioreq_server_range_type(d, p, &type, &addr) )
> +        return NULL;

... this use - maybe hvm_ioreq_server_get_type_addr() (albeit I
don't like this name very much)?

> @@ -1351,7 +1378,7 @@ static int hvm_send_buffered_ioreq(struct 
> hvm_ioreq_server *s, ioreq_t *p)
>      pg = iorp->va;
>  
>      if ( !pg )
> -        return X86EMUL_UNHANDLEABLE;
> +        return IOREQ_IO_UNHANDLED;

At this example - why the IO infix, duplicating the prefix? I'd
suggest to either drop it (if the remaining identifiers are deemed
unambiguous enough) or use e.g. IOREQ_STATUS_*.

> @@ -1515,11 +1542,21 @@ static int hvm_access_cf8(
>      return X86EMUL_UNHANDLEABLE;
>  }
>  
> +void arch_hvm_ioreq_init(struct domain *d)
> +{
> +    register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
> +}
> +
> +void arch_hvm_ioreq_destroy(struct domain *d)
> +{
> +
> +}

Stray blank line?

Jan