Re: [PATCH V4 14/24] arm/ioreq: Introduce arch specific bits for IOREQ/DM features

[Oleksandr <olekstysh@xxxxxxxxx>]

On 17.01.21 20:07, Julien Grall wrote:
> On 17/01/2021 17:11, Oleksandr wrote:
> > On 15.01.21 22:26, Julien Grall wrote:
> >
> > Hi Julien
> Hi Oleksandr,

Hi Julien



> > > > +
> > > >       PROGRESS(xen):
> > > >           ret = relinquish_memory(d, &d->xenpage_list);
> > > >           if ( ret )
> > > > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> > > > index ae7ef96..9814481 100644
> > > > --- a/xen/arch/arm/io.c
> > > > +++ b/xen/arch/arm/io.c
> > > > @@ -16,6 +16,7 @@
> > > >    * GNU General Public License for more details.
> > > >    */
> > > >   +#include <xen/ioreq.h>
> > > >   #include <xen/lib.h>
> > > >   #include <xen/spinlock.h>
> > > >   #include <xen/sched.h>
> > > > @@ -23,6 +24,7 @@
> > > >   #include <asm/cpuerrata.h>
> > > >   #include <asm/current.h>
> > > >   #include <asm/mmio.h>
> > > > +#include <asm/hvm/ioreq.h>
> > > Shouldn't this have been included by "xen/ioreq.h"?
> > Well, for V1 asm/hvm/ioreq.h was included by xen/ioreq.h. But, it 
> > turned out that there was nothing inside common header required arch 
> > one to be included and
> > I was asked to include arch header where it was indeed needed 
> > (several *.c files).
> Fair enough.
>
> [...]
>
> > > If you return IO_HANDLED here, then it means the we will take care 
> > > of previous I/O but the current one is going to be ignored. 
> > Which current one? As I understand, if try_fwd_ioserv() gets called 
> > with vio->req.state == STATE_IORESP_READY then this is a second round 
> > after emulator completes the emulation (the first round was when
> > we returned IO_RETRY down the function and claimed that we would need 
> > a completion), so we are still dealing with previous I/O.
> > vcpu_ioreq_handle_completion() -> arch_ioreq_complete_mmio() -> 
> > try_handle_mmio() -> try_fwd_ioserv() -> handle_ioserv()
> > And after we return IO_HANDLED here, handle_ioserv() will be called 
> > to complete the handling of this previous I/O emulation.
> > Or I really missed something?
> Hmmm... I somehow thought try_fw_ioserv() would only be called the 
> first time. Do you have a branch with your code applied? This would 
> help to follow the different paths.
Yes, I mentioned about it in cover letter.

Please see
https://github.com/otyshchenko1/xen/commits/ioreq_4.14_ml5
why 5 - because I started counting from the RFC)



> > > >   diff --git a/xen/include/asm-arm/domain.h 
> > > > b/xen/include/asm-arm/domain.h
> > > > index 6819a3b..c235e5b 100644
> > > > --- a/xen/include/asm-arm/domain.h
> > > > +++ b/xen/include/asm-arm/domain.h
> > > > @@ -10,6 +10,7 @@
> > > >   #include <asm/gic.h>
> > > >   #include <asm/vgic.h>
> > > >   #include <asm/vpl011.h>
> > > > +#include <public/hvm/dm_op.h>
> > > May I ask, why do you need to include dm_op.h here?
> > I needed to include that header to make some bits visible 
> > (XEN_DMOP_IO_RANGE_PCI, struct xen_dm_op_buf, etc). Why here - is a 
> > really good question.
> > I don't remember exactly, probably I followed x86's domain.h which 
> > also included it.
> > So, trying to remove the inclusion here, I get several build failures 
> > on Arm which could be fixed if I include that header from dm.h and 
> > ioreq.h:
> > Shall I do this way?
> If the failure are indeded because ioreq.h and dm.h use definition 
> from public/hvm/dm_op.h, then yes. Can you post the errors?
Please see attached, although I built for Arm32 (and the whole series), 
I think errors are valid for Arm64 also.
error1.txt - when remove #include <public/hvm/dm_op.h> from asm-arm/domain.h
error2.txt - when add #include <public/hvm/dm_op.h> to xen/ioreq.h
error3.txt - when add #include <public/hvm/dm_op.h> to xen/dm.h


> [...]
>
> > > >   #include <public/hvm/params.h>
> > > >     struct hvm_domain
> > > > @@ -262,6 +263,8 @@ static inline void arch_vcpu_block(struct vcpu 
> > > > *v) {}
> > > >     #define arch_vm_assist_valid_mask(d) (1UL << 
> > > > VMASST_TYPE_runstate_update_flag)
> > > >   +#define has_vpci(d)    ({ (void)(d); false; })
> > > > +
> > > >   #endif /* __ASM_DOMAIN_H__ */
> > > >     /*
> > > > diff --git a/xen/include/asm-arm/hvm/ioreq.h 
> > > > b/xen/include/asm-arm/hvm/ioreq.h
> > > > new file mode 100644
> > > > index 0000000..19e1247
> > > > --- /dev/null
> > > > +++ b/xen/include/asm-arm/hvm/ioreq.h
> > > Shouldn't this directly be under asm-arm/ rather than asm-arm/hvm/ 
> > > as the IOREQ is now meant to be agnostic?
> > Good question... The _common_ IOREQ code is indeed arch-agnostic. 
> > But, can the _arch_ IOREQ code be treated as really subarch-agnostic?
> > I think, on Arm it can and it is most likely ok to keep it in 
> > "asm-arm/", but how it would be correlated with x86's IOREQ code 
> > which is HVM specific and located
> > in "hvm" subdir?
> Sorry, I don't understand your answer/questions. So let me ask the 
> question differently, is asm-arm/hvm/ioreq.h going to be included from 
> common code?
Sorry if I was unclear.


> If the answer is no, then I see no reason to follow the x86 here.
> If the answer is yes, then I am quite confused why half of the series 
> tried to remove "hvm" from the function name but we still include 
> "asm/hvm/ioreq.h".
Answer is yes. Even if we could to avoid including that header from the 
common code somehow, we would still have #include <public/hvm/*>, 
is_hvm_domain().

> Cheers,
--
Regards,

Oleksandr Tyshchenko

Attachment: error3.txt
Description: Text document

Attachment: error2.txt
Description: Text document

Attachment: error1.txt
Description: Text document