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

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxxx>
> > >
> > > 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@xxxxxxx>
> > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > ---
> > >   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