Re: [PATCH V1 02/16] xen/ioreq: Make x86's IOREQ feature common

[Oleksandr <olekstysh@xxxxxxxxx>]

On 24.09.20 13:58, Jan Beulich wrote:

Hi Jan

> On 23.09.2020 14:28, Oleksandr wrote:
> > On 22.09.20 18:52, Jan Beulich wrote:
> > > On 22.09.2020 17:05, Oleksandr wrote:
> > > > 3. *arch.hvm.hvm_io*: We could also use the following:
> > > >
> > > >       #define ioreq_get_io_completion(v) ((v)->arch.hvm.hvm_io.io_completion)
> > > >       #define ioreq_get_io_req(v) ((v)->arch.hvm.hvm_io.io_req)
> > > >
> > > >       This way struct hvm_vcpu_io won't be used in common code as well.
> > > But if Arm needs similar field, why keep them in arch.hvm.hvm_io?
> > Yes, Arm needs the "some" fields, but not "all of them" as x86 has.
> > For example Arm needs only the following (at least in the context of
> > this series):
> >
> > +struct hvm_vcpu_io {
> > +    /* I/O request in flight to device model. */
> > +    enum hvm_io_completion io_completion;
> > +    ioreq_t                io_req;
> > +
> > +    /*
> > +     * HVM emulation:
> > +     *  Linear address @mmio_gla maps to MMIO physical frame @mmio_gpfn.
> > +     *  The latter is known to be an MMIO frame (not RAM).
> > +     *  This translation is only valid for accesses as per @mmio_access.
> > +     */
> > +    struct npfec        mmio_access;
> > +    unsigned long       mmio_gla;
> > +    unsigned long       mmio_gpfn;
> > +};
> >
> > But for x86 the number of fields is quite bigger. If they were in same
> > way applicable for both archs (as what we have with ioreq_server struct)
> > I would move it to the common domain. I didn't think of a better idea
> > than just abstracting accesses to these (used in common ioreq.c) two
> > fields by macro.
> I'm surprised Arm would need all the three last fields that you
> mention. Both here and ...
>
> > > > @@ -247,7 +247,7 @@ static gfn_t hvm_alloc_legacy_ioreq_gfn(struct
> > > > hvm_ioreq_server *s)
> > > >         for ( i = HVM_PARAM_IOREQ_PFN; i <= HVM_PARAM_BUFIOREQ_PFN; i++ )
> > > >         {
> > > >             if ( !test_and_clear_bit(i, &d->ioreq_gfn.legacy_mask) )
> > > > -            return _gfn(d->arch.hvm.params[i]);
> > > > +            return _gfn(ioreq_get_params(d, i));
> > > >         }
> > > >
> > > >         return INVALID_GFN;
> > > > @@ -279,7 +279,7 @@ static bool hvm_free_legacy_ioreq_gfn(struct
> > > > hvm_ioreq_server *s,
> > > >
> > > >         for ( i = HVM_PARAM_IOREQ_PFN; i <= HVM_PARAM_BUFIOREQ_PFN; i++ )
> > > >         {
> > > > -        if ( gfn_eq(gfn, _gfn(d->arch.hvm.params[i])) )
> > > > +        if ( gfn_eq(gfn, _gfn(ioreq_get_params(d, i))) )
> > > >                  break;
> > > >         }
> > > >         if ( i > HVM_PARAM_BUFIOREQ_PFN )
> > > And these two are needed by Arm? Shouldn't Arm exclusively use
> > > the new model, via acquire_resource?
> > I dropped HVMOP plumbing on Arm as it was requested. Only acquire
> > interface should be used.
> > This code is not supposed to be called on Arm, but it is a part of
> > common code and we need to find a way how to abstract away *arch.hvm.params*
> ... here I wonder whether you aren't moving more pieces to common
> code than are actually arch-independent. I think a prereq step
> missing so far is to clearly identify which pieces of the code
> are arch-independent, and work towards abstracting away all of the
> arch-dependent ones.
Unfortunately, not all things are clear and obvious from the very beginning.
I have to admit, I didn't even imagine earlier that *arch.hvm.* usage in 
the common code is a layering violation issue.
Hopefully, now it is clear as well as the steps to avoid it in future.

...


I saw your advise (but haven't answered yet there) regarding splitting 
struct hvm_vcpu_io in
[PATCH V1 09/16] arm/ioreq: Introduce arch specific bits for IOREQ/DM 
features. I think, it makes sense.
The only remaining bits I would like to clarify here is 
*arch.hvm.params*. Should we really want to move HVM params field to the 
common code
rather than abstracting away by proposed macro? Although stores a few 
IOREQ params, it is not a (completely) IOREQ stuff and is specific to 
the architecture.
--
Regards,

Oleksandr Tyshchenko