> -----Original Message-----
> From: Oleksandr <olekst...@gmail.com>
> Sent: 07 December 2020 21:00
> To: Jan Beulich <jbeul...@suse.com>; Paul Durrant <p...@xen.org>
> Cc: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>; Andrew Cooper
> <andrew.coop...@citrix.com>;
> Roger Pau Monné <roger....@citrix.com>; Wei Liu <w...@xen.org>; George Dunlap
> <george.dun...@citrix.com>; Ian Jackson <i...@xenproject.org>; Julien Grall
> <jul...@xen.org>; Stefano
> Stabellini <sstabell...@kernel.org>; Jun Nakajima <jun.nakaj...@intel.com>;
> Kevin Tian
> <kevin.t...@intel.com>; Julien Grall <julien.gr...@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;
> >> +};
> >> +
