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
