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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 06.08.20 14:08, Julien Grall wrote:

Hi Julien

> > > 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()?
For me it sounds fine.



> > > > 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?
Previous patch "xen/mm: Make x86's XENMEM_resource_ioreq_server handling 
common" breaks build on Arm as it includes xen/hvm/ioreq.h which 
requires arch header
to be present (asm/hvm/ioreq.h). But the missing arch header together 
with other arch specific bits are introduced here in current patch. 
Probably I should have rearranged
changes in a way to not introduce #ifdef and then remove it...


> [...]
>
> > > > +
> > > > +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.
ok


> > > > +    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.
Thanks for the explanation. The only one question remains where to put 
common helpers hvm_mmio_first_byte/hvm_mmio_last_byte (common io.h?)?

> > > > +
> > > > +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.
True.


> Would it make sense to have a stub arch_hvm_destroy_ioreq_server()?
With what has been said above, it make sense, will create.


> > > > +
> > > > +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();

Good point, will update.


--
Regards,

Oleksandr Tyshchenko