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

[Oleksandr <olekstysh@xxxxxxxxx>]

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?


> > --- 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.


> > +struct vcpu_io {
> > +    /* I/O request in flight to device model. */
> > +    enum io_completion   completion;
> > +    ioreq_t              req;
> > +};
> > +
> >   struct vcpu
> >   {
> >       int              vcpu_id;
> > @@ -256,6 +271,10 @@ struct vcpu
> >       struct vpci_vcpu vpci;
> >   
> >       struct arch_vcpu arch;
> > +
> > +#ifdef CONFIG_IOREQ_SERVER
> > +    struct vcpu_io io;
> > +#endif
> >   };
> I don't have a good solution in mind, and I'm also not meaning to
> necessarily request a change here, but I'd like to point out that
> this does away (for this part of it only, of course) with the
> overlaying of the PV and HVM sub-structs on x86. As long as the
> HVM part is the far bigger one, that's not a problem, but I wanted
> to mention the aspect nevertheless.
>
> Jan
--
Regards,

Oleksandr Tyshchenko