RE: [PATCH V3 11/23] xen/ioreq: Move x86's io_completion/io_req fields to struct vcpu

[Paul Durrant <xadimgnik@gmail.com>]

> -----Original Message-----
> From: Oleksandr <olekstysh@gmail.com>
> Sent: 07 December 2020 21:00
> To: Jan Beulich <jbeulich@suse.com>; Paul Durrant <paul@xen.org>
> Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
> Roger Pau Monné <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; George Dunlap
> <george.dunlap@citrix.com>; Ian Jackson <iwj@xenproject.org>; Julien Grall <julien@xen.org>; Stefano
> Stabellini <sstabellini@kernel.org>; Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian
> <kevin.tian@intel.com>; Julien Grall <julien.grall@arm.com>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH V3 11/23] xen/ioreq: Move x86's io_completion/io_req fields to struct vcpu
> 
> 
> On 07.12.20 14:32, Jan Beulich wrote:
> 
> Hi Jan, Paul.
> 
> > On 30.11.2020 11:31, Oleksandr Tyshchenko wrote:
> >> --- a/xen/arch/x86/hvm/emulate.c
> >> +++ b/xen/arch/x86/hvm/emulate.c
> >> @@ -142,8 +142,8 @@ void hvmemul_cancel(struct vcpu *v)
> >>   {
> >>       struct hvm_vcpu_io *vio = &v->arch.hvm.hvm_io;
> >>
> >> -    vio->io_req.state = STATE_IOREQ_NONE;
> >> -    vio->io_completion = HVMIO_no_completion;
> >> +    v->io.req.state = STATE_IOREQ_NONE;
> >> +    v->io.completion = IO_no_completion;
> >>       vio->mmio_cache_count = 0;
> >>       vio->mmio_insn_bytes = 0;
> >>       vio->mmio_access = (struct npfec){};
> >> @@ -159,7 +159,7 @@ static int hvmemul_do_io(
> >>   {
> >>       struct vcpu *curr = current;
> >>       struct domain *currd = curr->domain;
> >> -    struct hvm_vcpu_io *vio = &curr->arch.hvm.hvm_io;
> >> +    struct vcpu_io *vio = &curr->io;
> > Taking just these two hunks: "vio" would now stand for two entirely
> > different things. I realize the name is applicable to both, but I
> > wonder if such naming isn't going to risk confusion.Despite being
> > relatively familiar with the involved code, I've been repeatedly
> > unsure what exactly "vio" covers, and needed to go back to the
> 
>   Good comment... Agree that with the naming scheme in current patch the
> code became a little bit confusing to read.
> 
> 
> > header. So together with the name possible adjustment mentioned
> > further down, maybe "vcpu_io" also wants it name changed, such that
> > the variable then also could sensibly be named (slightly)
> > differently? struct vcpu_io_state maybe? Or alternatively rename
> > variables of type struct hvm_vcpu_io * to hvio or hio? Otoh the
> > savings aren't very big for just ->io, so maybe better to stick to
> > the prior name with the prior type, and not introduce local
> > variables at all for the new field, like you already have it in the
> > former case?
> I would much prefer the last suggestion which is "not introduce local
> variables at all for the new field" (I admit I was thinking almost the
> same, but haven't chosen this direction).
> But I am OK with any suggestions here. Paul what do you think?
> 

I personally don't think there is that much risk of confusion. If there is a desire to disambiguate though, I would go the route of naming hvm_vcpu_io locals 'hvio'.

> 
> >
> >> --- a/xen/include/xen/sched.h
> >> +++ b/xen/include/xen/sched.h
> >> @@ -145,6 +145,21 @@ void evtchn_destroy_final(struct domain *d); /* from complete_domain_destroy
> */
> >>
> >>   struct waitqueue_vcpu;
> >>
> >> +enum io_completion {
> >> +    IO_no_completion,
> >> +    IO_mmio_completion,
> >> +    IO_pio_completion,
> >> +#ifdef CONFIG_X86
> >> +    IO_realmode_completion,
> >> +#endif
> >> +};
> > I'm not entirely happy with io_ / IO_ here - they seem a little
> > too generic. How about ioreq_ / IOREQ_ respectively?
> 
> I am OK, would like to hear Paul's opinion on both questions.
> 

No, I think the 'IO_' prefix is better. They relate to a field in the vcpu_io struct. An alternative might be 'VIO_'...

> 
> >
> >> +struct vcpu_io {
> >> +    /* I/O request in flight to device model. */
> >> +    enum io_completion   completion;

... in which case, you could also name the enum 'vio_completion'.

  Paul

> >> +    ioreq_t              req;
> >> +};
> >> +