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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 08.12.20 09:52, Paul Durrant wrote:

Hi Paul, Jan

> > > 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'.
Well, I assume I should rename all hvm_vcpu_io locals in the code to 
"hvio" (even if this patch didn't touch these places so far since no new 
vcpu_io fields were involved).
I am OK, although expecting a lot of places which need touching here...


> > > > --- 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
ok, will follow new renaming scheme IO_ -> VIO_ (io_ -> vio_).


--
Regards,

Oleksandr Tyshchenko