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

[Oleksandr <olekstysh@gmail.com>]

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