Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features

[Julien Grall <julien@xen.org>]

On 05/08/2020 20:30, Oleksandr wrote:
> 
> On 05.08.20 17:12, Julien Grall wrote:
>> Hi,
> 
> Hi Julien
> 
> 
>>
>> On 03/08/2020 19:21, Oleksandr Tyshchenko wrote:
>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>
>>> This patch makes possible to forward Guest MMIO accesses
>>> to a device emulator on Arm and enables that support for
>>> Arm64.
>>>
>>> Also update XSM code a bit to let DM op be used on Arm.
>>> New arch DM op will be introduced in the follow-up patch.
>>>
>>> Please note, at the moment build on Arm32 is broken
>>> (see cmpxchg usage in hvm_send_buffered_ioreq()) if someone
>>> wants to enable CONFIG_IOREQ_SERVER due to the lack of
>>> cmpxchg_64 support on Arm32.
>>>
>>> Please note, this is a split/cleanup of Julien's PoC:
>>> "Add support for Guest IO forwarding to a device emulator"
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> ---
>>>   tools/libxc/xc_dom_arm.c        |  25 +++++++---
>>>   xen/arch/arm/Kconfig            |   1 +
>>>   xen/arch/arm/Makefile           |   2 +
>>>   xen/arch/arm/dm.c               |  34 +++++++++++++
>>>   xen/arch/arm/domain.c           |   9 ++++
>>>   xen/arch/arm/hvm.c              |  46 +++++++++++++++++-
>>>   xen/arch/arm/io.c               |  67 +++++++++++++++++++++++++-
>>>   xen/arch/arm/ioreq.c            |  86 
>>> +++++++++++++++++++++++++++++++++
>>>   xen/arch/arm/traps.c            |  17 +++++++
>>>   xen/common/memory.c             |   5 +-
>>>   xen/include/asm-arm/domain.h    |  80 +++++++++++++++++++++++++++++++
>>>   xen/include/asm-arm/hvm/ioreq.h | 103 
>>> ++++++++++++++++++++++++++++++++++++++++
>>>   xen/include/asm-arm/mmio.h      |   1 +
>>>   xen/include/asm-arm/p2m.h       |   7 +--
>>>   xen/include/xsm/dummy.h         |   4 +-
>>>   xen/include/xsm/xsm.h           |   6 +--
>>>   xen/xsm/dummy.c                 |   2 +-
>>>   xen/xsm/flask/hooks.c           |   5 +-
>>>   18 files changed, 476 insertions(+), 24 deletions(-)
>>>   create mode 100644 xen/arch/arm/dm.c
>>>   create mode 100644 xen/arch/arm/ioreq.c
>>>   create mode 100644 xen/include/asm-arm/hvm/ioreq.h
>>
>> It feels to me the patch is doing quite a few things that are 
>> indirectly related. Can this be split to make the review easier?
>>
>> I would like at least the following split from the series:
>>    - The tools changes
>>    - The P2M changes
>>    - The HVMOP plumbing (if we still require them)
> Sure, will split.
> However, I don't quite understand where we should leave HVMOP plumbing.

I think they will need to be droppped if we decide to use the acquire 
interface.

> If I understand correctly the suggestion was to switch to acquire 
> interface instead (which requires a Linux version to be v4.17 at least)?

This was the suggestion.

> I suspect, this is all about "xen/privcmd: add 
> IOCTL_PRIVCMD_MMAP_RESOURCE" support for Linux?

Correct.

>> What is this function supposed to do?
> Agree, sounds confusing a bit. I assume it is supposed to complete Guest 
> MMIO access after finishing emulation.
> 
> Shall I rename it to something appropriate (maybe by adding ioreq prefix)?

How about ioreq_handle_complete_mmio()?

>>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>>> index 9283e5e..0000477 100644
>>> --- a/xen/common/memory.c
>>> +++ b/xen/common/memory.c
>>> @@ -8,6 +8,7 @@
>>>    */
>>>     #include <xen/domain_page.h>
>>> +#include <xen/hvm/ioreq.h>
>>>   #include <xen/types.h>
>>>   #include <xen/lib.h>
>>>   #include <xen/mm.h>
>>> @@ -30,10 +31,6 @@
>>>   #include <public/memory.h>
>>>   #include <xsm/xsm.h>
>>>   -#ifdef CONFIG_IOREQ_SERVER
>>> -#include <xen/hvm/ioreq.h>
>>> -#endif
>>> -
>>
>> Why do you remove something your just introduced?
> The reason I guarded that header is to make "xen/mm: Make x86's 
> XENMEM_resource_ioreq_server handling common" (previous) patch buildable 
> on Arm
> without arch IOREQ header added yet. I tried to make sure that the 
> result after each patch was buildable to retain bisectability.
> As current patch adds Arm IOREQ specific bits (including header), that 
> guard could be removed as not needed anymore.
I agree we want to have the build bisectable. However, I am still 
puzzled why it is necessary to remove the #ifdef and move it earlier in 
the list.

Do you mind to provide more details?

[...]

>>> +
>>> +bool handle_mmio(void);
>>> +
>>> +static inline bool handle_pio(uint16_t port, unsigned int size, int 
>>> dir)
>>> +{
>>> +    /* XXX */
>>
>> Can you expand this TODO? What do you expect to do?
> I didn't expect this to be called on Arm. Sorry, I am not sure l have an 
> idea how to handle this properly. I would keep it unimplemented until a 
> real reason.
> Will expand TODO.

Let see how the conversation on patch#1 goes about PIO vs MMIO.

>>
>>
>>> +    BUG();
>>> +    return true;
>>> +}
>>> +
>>> +static inline paddr_t hvm_mmio_first_byte(const ioreq_t *p)
>>> +{
>>> +    return p->addr;
>>> +}
>>
>> I understand that the x86 version is more complex as it check p->df. 
>> However, aside reducing the complexity, I am not sure why we would 
>> want to diverge it.
>>
>> After all, IOREQ is now meant to be a common feature.
> Well, no objections at all.
> Could you please clarify how could 'df' (Direction Flag?) be 
> handled/used on Arm?

On x86, this is used by 'rep' instruction to tell the direction to 
iterate (forward or backward).

On Arm, all the accesses to MMIO region will do a single memory access. 
So for now, we can safely always set to 0.

> I see that try_fwd_ioserv() always sets it 0. Or I 
> need to just reuse x86's helpers as is,
> which (together with count = df = 0) will result in what we actually 
> have here?
AFAIU, both count and df should be 0 on Arm.

>>
>>
>>> +
>>> +static inline int p2m_set_ioreq_server(struct domain *d,
>>> +                                       unsigned int flags,
>>> +                                       struct hvm_ioreq_server *s)
>>> +{
>>> +    return -EOPNOTSUPP;
>>> +}
>>
>> This should be defined in p2m.h. But I am not even sure what it is 
>> meant for. Can you expand it?
> 
> ok, will move.
> 
> 
> In this series I tried to make as much IOREQ code common as possible and 
> avoid complicating things, in order to achieve that a few stubs were 
> added here. Please note,
> that I also considered splitting into arch parts. But some functions 
> couldn't be split easily.
> This one is called from common hvm_destroy_ioreq_server() with flag 
> being 0 (which will result in unmapping ioreq server from p2m type on x86).
> I could add a comment describing why this stub is present here.

Sorry if I wasn't clear. I wasn't asking why the stub is there but what 
should be the expected implementation of the function.

In particular, you are returning -EOPNOTSUPP. The only reason you are 
getting away from trouble is because the caller doesn't check the return.

Would it make sense to have a stub arch_hvm_destroy_ioreq_server()?

> 
> 
>>
>>
>>> +
>>> +static inline void msix_write_completion(struct vcpu *v)
>>> +{
>>> +}
>>> +
>>> +static inline void handle_realmode_completion(void)
>>> +{
>>> +    ASSERT_UNREACHABLE();
>>> +}
>>
>> realmode is very x86 specific. So I don't think this function should 
>> be called from common code. It might be worth considering to split 
>> handle_hvm_io_completion() is 2 parts: common and arch specific.
> 
> I agree with you that realmode is x86 specific and looks not good in Arm 
> header. 
It is not a problem of looking good or not. Instead, it is about 
abstraction. A developper shouldn't need to understand all the other 
architectures we support in order to follow the common code.

> I was thinking how to split handle_hvm_io_completion() 
> gracefully but I failed find a good solution for that, so decided to add 
> two stubs (msix_write_completion and handle_realmode_completion) on Arm. 
> I could add a comment describing why they are here if appropriate. But 
> if you think they shouldn't be called from the common code in any way, I 
> will try to split it.

I am not entirely sure what msix_write_completion is meant to do on x86. 
Is it dealing with virtual MSIx? Maybe Jan, Roger or Paul could help?

Regarding handle_realmode_completion, I would add a new stub:

arch_ioreq_handle_io_completion() that is called from the default case 
of the switch.

On x86 it would be implemented as:

  switch (io_completion)
  {
     case HVMIO_realmode_completion:
       ...
     default:
       ASSERT_UNREACHABLE();
  }

On Arm, it would be implemented as:

   ASSERT_UNREACHABLE();

Cheers,

-- 
Julien Grall