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

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

[Oleksandr Tyshchenko]

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@epam.com>
---
  V2:
   - s/Arm64/Arm
   - drop from dm_op() in xen/arch/arm/dm.c
   - update commit subject
---
---
 xen/arch/arm/dm.c             | 1 -
 xen/include/public/arch-arm.h | 1 -
 2 files changed, 2 deletions(-)

diff --git a/xen/arch/arm/dm.c b/xen/arch/arm/dm.c
index fdb3d967ec..67d2c950e5 100644
--- a/xen/arch/arm/dm.c
+++ b/xen/arch/arm/dm.c
@@ -21,7 +21,6 @@ int dm_op(const struct dmop_args *op_args)
 
     static const uint8_t op_size[] = {
         [XEN_DMOP_create_ioreq_server]              = sizeof(struct xen_dm_op_create_ioreq_server),
-        [XEN_DMOP_get_ioreq_server_info]            = sizeof(struct xen_dm_op_get_ioreq_server_info),
         [XEN_DMOP_map_io_range_to_ioreq_server]     = sizeof(struct xen_dm_op_ioreq_server_range),
         [XEN_DMOP_unmap_io_range_from_ioreq_server] = sizeof(struct xen_dm_op_ioreq_server_range),
         [XEN_DMOP_set_ioreq_server_state]           = sizeof(struct xen_dm_op_set_ioreq_server_state),
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index e2412a1747..023cc2f468 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -130,7 +130,6 @@
  *  HYPERVISOR_dm_op
  *   Exactly these sub-operations are supported:
  *    * XEN_DMOP_create_ioreq_server
- *    * XEN_DMOP_get_ioreq_server_info
  *    * XEN_DMOP_map_io_range_to_ioreq_server
  *    * XEN_DMOP_unmap_io_range_from_ioreq_server
  *    * XEN_DMOP_set_ioreq_server_state
-- 
2.34.1

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

[Orzel, Michal]

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@epam.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

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.

~Michal

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

[Julien Grall]

Hi,

On 02/09/2025 10: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()

Yet. I can see use in the future to optimize some emulation (e.g. virtio 
notification).

To be honest, I don't quite see the benefit of removing this code as 
most of it is mainly generic.

Cheers,

-- 
Julien Grall

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

[Julien Grall]

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@epam.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> 
> 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...

Cheers,

-- 
Julien Grall

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

[Orzel, Michal]

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@epam.com>
>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>
>> 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. 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. 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.

~Michal

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

[Julien Grall]

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@epam.com>
>>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>>
>>> 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.

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

[1] 
https://lore.kernel.org/xen-devel/e7c99116-f6a9-43e1-80ae-b3a4d44857b7@amd.com/T/#t

> 
> ~Michal
> 

-- 
Julien Grall

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

[Oleksandr Tyshchenko]

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@epam.com>
>>>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>>>
>>>> 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
>>
> 

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

[Julien Grall]

Hi Oleksandr,

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@epam.com>
>>>>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>>>>
>>>>> 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? 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.

Cheers,

-- 
Julien Grall

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

[Oleksandr Tyshchenko]

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@epam.com>
>>>>>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>>>>>
>>>>>> 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,
>