Re: [PATCH V2] xen/arm: dm: Drop XEN_DMOP_get_ioreq_server_info from supported

[Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>]

On 02.09.25 17:36, Julien Grall wrote:
> Hi Oleksandr,

Hello Julien

> 
> On 02/09/2025 15:00, Oleksandr Tyshchenko wrote:
>>
>>
>> On 02.09.25 15:19, Julien Grall wrote:
>>
>> Hello Julien
>>
>>> On 02/09/2025 13:10, Orzel, Michal wrote:
>>>>
>>>>
>>>> On 02/09/2025 13:54, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 02/09/2025 11:18, Orzel, Michal wrote:
>>>>>>
>>>>>>
>>>>>> On 02/09/2025 11:49, Oleksandr Tyshchenko wrote:
>>>>>>> The said sub-op is not supported on Arm, since it:
>>>>>>>     - does not support the buffered emulation (so bufioreq_port/
>>>>>>> bufioreq_gfn
>>>>>>>       cannot be returned), please refer to ioreq_server_create()
>>>>>>>     - does not support "legacy" mechanism of mapping IOREQ Server
>>>>>>>       magic pages (so ioreq_gfn/bufioreq_gfn cannot be returned),
>>>>>>> please
>>>>>>>       refer to arch_ioreq_server_map_pages(). On Arm, only the 
>>>>>>> Acquire
>>>>>>>       Resource infrastructure is used to query and map the IOREQ
>>>>>>> Server pages.
>>>>>>>
>>>>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>>>>>> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
>>>>>>
>>>>>> Could we perhaps add a Fixes tag here pointing to the commit
>>>>>> introducing these
>>>>>> DM ops and thus add this patch for this release? Not sure what
>>>>>> others think.
>>>>>
>>>>> Fixes usually implies a bug and I don't see what bug we are 
>>>>> solving. In
>>>>> fact, I don't understand why we are trying to remove the subop...
>>>> Hmm, the issue is that the subop that is not supported at the moment
>>>> is listed
>>>> as supported in the public header.
>>>
>>> [...]
>>>
>>>> As for the code, from safety perspective if this subop is listed
>>>> explicilty in Arm's
>>>> dm.c, we would need to write a separate test case and test to cover it
>>>> that at
>>>> the end, still returns -EOPNOTSUPP.
>>>
>>> Why do you think it is not supported? AFAICT, it is still possible to
>>> pass XEN_DMOP_nognfs to figure out whwether bufioreq is currently
>>> available. The error code would be 0 not -EOPNOTSUPP.
>>
>>
>> Yes, by passing XEN_DMOP_no_gfns we will bypass
>> arch_ioreq_server_map_pages() call, and yes, ioreq_server_get_info()
>> will return 0 in that case.
>>
>> But, "bufioreq_port" field in struct xen_dm_op_get_ioreq_server_info
>> (out param) won't be really updated (since the IOREQ Server was created
>> with HVM_IOREQSRV_BUFIOREQ_OFF before that).
> 
> Sure. But this would be the same behavior on x86, right? 

seems so, yes

If so...
> 
>>
>> So, "at the moment", XEN_DMOP_get_ioreq_server_info sub-op is
>> non-functional/useless on Arm ("unsupported" is probably not a precise
>> word in that particular case), this is my understanding (which might be
>> wrong). That is why I have sent a patch to remove the mention from the
>> public header.
> ... I still don't understand why we are just trying to sweep the problem 
> under the rug on Arm.
> 
> That's assuming there is one. If there is a problem, then we should fix 
> it properly for all architectures. If there is no problem, then this 
> patch should not exists.

I think, I got your point. Let's ignore the current patch.


> 
> Cheers,
>