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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 14.09.20 16:52, Jan Beulich wrote:

Hi Jan

> 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).
Got it. Will add an explanation.


> 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...
For me without "handle" it will sound a bit confusing because of the 
enum type which
has a similar name but without "arch" prefix:
bool arch_hvm_io_completion(enum hvm_io_completion io_completion)


Shall I then move "hvm" to the front of the name here?


> > @@ -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).
I didn't have a plan to add error handling here. Agree, will make 
arch_hvm_destroy_ioreq_server() returning void.

> > @@ -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?
As I understand from the review comment on that for the RFC patch, there 
is no
a lot of point of keeping this. Indeed, looking at the code I got the 
same opinion.
I should have added an explanation in the commit description at least.
Or shall I return the call back?


> > @@ -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?
Agree, will add.


> > +                                    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.
This function is just an attempt to split arch specific things (cf8 
handling) from "common" hvm_select_ioreq_server().

>  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)?
Yes, hvm_ioreq_server_get_type_addr() better represents what function does.


> > @@ -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_*.
Agree, I would prefer 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?
Will remove.


--
Regards,

Oleksandr Tyshchenko