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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 10.08.20 21:25, Julien Grall wrote:

Hi Julien

> > > 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. 
> I understand that both Arm and x86 now implement the asm/hvm/ioreq.h.
> However, please keep in mind that there might be other architectures 
> in the future.
> With your change here, you would impose a new arch to implement 
> asm/hvm/ioreq.h even if the developper have no plan to use the feature.
> > Probably I should have rearranged
> > changes in a way to not introduce #ifdef and then remove it...
> Ideally we want to avoid #ifdef in the common code. But if this can't 
> be done in an header, then the #ifdef here would be fine.
Got it.


> > > > > 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?)?
> It feels to me it should be part of the common ioreq.h.
ok, will move.


--
Regards,

Oleksandr Tyshchenko