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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 21.09.20 15:31, Jan Beulich wrote:

Hi

> On 21.09.2020 14:22, Oleksandr wrote:
> > On 14.09.20 16:52, Jan Beulich wrote:
> > > On 10.09.2020 22:21, Oleksandr Tyshchenko wrote:
> > > > --- 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)
> Every function handles something; there's no point including
> "handle" in every name. Or else we'd have handle_memset()
> instead of just memset(), for example.
Got it. Will remove "handle" here.


> > Shall I then move "hvm" to the front of the name here?
> As per comments on later patches, I think we want to consider dropping
> hvm prefixes or infixes altogether from the functions involved here.
> > > > @@ -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?
> If there's a reason to drop it (which I can't see, but I also
> don't recall seeing the discussion you're mentioning), then doing
> so should be a separate patch with suitable reasoning. In the
> patch here you definitely should only transform what's already
> there.
Sounds reasonable. Please see the comment below 
relocate_portio_handler() here:
https://www.mail-archive.com/xen-devel@xxxxxxxxxxxxxxxxxxxx/msg78512.html

However, I might interpret the request incorrectly.

--
Regards,

Oleksandr Tyshchenko