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

[Oleksandr Tyshchenko <Oleksandr_Tyshchenko@xxxxxxxx>]

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).

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 think if we mistakenly mention sth as> supported in e.g. SUPPORT.md 
> we would have no issues adding a Fixes tag. There
>  > are many cases where Fixes was used just to change something in a 
> comment, so
>  > I'm having a hard time reasoning about when it's appropriate to use it.
> I think what we would want is "Amends". This is currently proposed by [1].

Good point.


> 
> [1] https://eur01.safelinks.protection.outlook.com/? 
> url=https%3A%2F%2Flore.kernel.org%2Fxen-devel%2Fe7c99116-f6a9-43e1-80ae- 
> b3a4d44857b7%40amd.com%2FT%2F%23t&data=05%7C02%7COleksandr_Tyshchenko%40epam.com%7C27024902b14c42b7eaf608ddea1b0173%7Cb41b72d04e9f4c268a69f949f367c91d%7C1%7C0%7C638924123934835957%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=7o1CpNkXPxHQqqnWBPEUy1Q1%2BjL%2FM7VmTrMT7fOu4Lw%3D&reserved=0
> 
>>
>> ~Michal
>>
>