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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.09.2020 17:38, Oleksandr wrote:
> On 24.09.20 13:58, Jan Beulich wrote:
>> 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:
>>>>> @@ -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?

I don't think I suggested doing so. In fact I recall having voiced
my expectation that Arm wouldn't use this at all. So yes, I agree
this better wouldn't be moved out of arch.hvm, but instead accesses
be abstracted by another means.

Jan