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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 22.09.20 18:52, Jan Beulich wrote:

Hi Jan

> On 22.09.2020 17:05, Oleksandr wrote:
> > 2. *arch.hvm.params*: Two functions that use it
> > (hvm_alloc_legacy_ioreq_gfn/hvm_free_legacy_ioreq_gfn) either go into
> > arch code completely or
> >       specific macro is used in common code:
> >
> >      #define ioreq_get_params(d, i) ((d)->arch.hvm.params[i])
> If Arm has the concept of params, then perhaps. But I didn't think
> Arm does ...
I think it has in some degree, there is a handling of 
HVMOP_set_param/HVMOP_get_param and
also there is a code to setup HVM_PARAM_CALLBACK_IRQ.


> >      I would prefer macro than moving functions to arch code (which are
> > equal and should remain in sync).
> Yes, if the rest of the code is identical, I agree it's better to
> merely abstract away small pieces like this one.
ok


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

> > --- a/xen/common/ioreq.c
> > +++ b/xen/common/ioreq.c
> > @@ -194,7 +194,7 @@ static bool hvm_wait_for_io(struct hvm_ioreq_vcpu
> > *sv, ioreq_t *p)
> >    bool handle_hvm_io_completion(struct vcpu *v)
> >    {
> >        struct domain *d = v->domain;
> > -    struct hvm_vcpu_io *vio = &v->arch.hvm.hvm_io;
> > +    ioreq_t io_req = ioreq_get_io_req(v);
> >        struct hvm_ioreq_server *s;
> >        struct hvm_ioreq_vcpu *sv;
> >        enum hvm_io_completion io_completion;
> > @@ -209,14 +209,14 @@ bool handle_hvm_io_completion(struct vcpu *v)
> >        if ( sv && !hvm_wait_for_io(sv, get_ioreq(s, v)) )
> >            return false;
> >
> > -    vio->io_req.state = hvm_ioreq_needs_completion(&vio->io_req) ?
> > +    io_req.state = hvm_ioreq_needs_completion(&io_req) ?
> >            STATE_IORESP_READY : STATE_IOREQ_NONE;
> This is unlikely to be correct - you're now updating an on-stack
> copy of the ioreq_t instead of what vio points at.
Oh, thank you for pointing this, I should have used ioreq_t *io_req = 
&ioreq_get_io_req(v);
I don't like ioreq_get_io_req much), probably ioreq_req would sound a 
little bit better?

> >        msix_write_completion(v);
> >        vcpu_end_shutdown_deferral(v);
> >
> > -    io_completion = vio->io_completion;
> > -    vio->io_completion = HVMIO_no_completion;
> > +    io_completion = ioreq_get_io_completion(v);
> > +    ioreq_get_io_completion(v) = HVMIO_no_completion;
> I think it's at least odd to have an lvalue with this kind of a
> name. Perhaps want to drop "get" if it's really meant to be used
> like this.
ok


> > @@ -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*
Am I correct?


--
Regards,

Oleksandr Tyshchenko