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

[Oleksandr <olekstysh@gmail.com>]

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