Re: [RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common

[Oleksandr <olekstysh@xxxxxxxxx>]

On 10.08.20 19:29, Julien Grall wrote:
> Hi,
Hi Julien


> On 06/08/2020 12:37, Oleksandr wrote:
> > On 05.08.20 16:30, Julien Grall wrote:
> > > Hi,
> > Hi Julien
> >
> >
> > > On 03/08/2020 19:21, Oleksandr Tyshchenko wrote:
> > > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > >
> > > > As a lot of x86 code can be re-used on Arm later on, this patch
> > > > splits IOREQ support into common and arch specific parts.
> > > >
> > > > This support is going to be used on Arm to be able run device
> > > > emulator outside of Xen hypervisor.
> > > >
> > > > 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>
> > > > ---
> > > >   xen/arch/x86/Kconfig            |    1 +
> > > >   xen/arch/x86/hvm/dm.c           |    2 +-
> > > >   xen/arch/x86/hvm/emulate.c      |    2 +-
> > > >   xen/arch/x86/hvm/hvm.c          |    2 +-
> > > >   xen/arch/x86/hvm/io.c           |    2 +-
> > > >   xen/arch/x86/hvm/ioreq.c        | 1431 
> > > > +--------------------------------------
> > > >   xen/arch/x86/hvm/stdvga.c       |    2 +-
> > > >   xen/arch/x86/hvm/vmx/realmode.c |    1 +
> > > >   xen/arch/x86/hvm/vmx/vvmx.c     |    2 +-
> > > >   xen/arch/x86/mm.c               |    2 +-
> > > >   xen/arch/x86/mm/shadow/common.c |    2 +-
> > > >   xen/common/Kconfig              |    3 +
> > > >   xen/common/Makefile             |    1 +
> > > >   xen/common/hvm/Makefile         |    1 +
> > > >   xen/common/hvm/ioreq.c          | 1430 
> > > > ++++++++++++++++++++++++++++++++++++++
> > > >   xen/include/asm-x86/hvm/ioreq.h |   45 +-
> > > >   xen/include/asm-x86/hvm/vcpu.h  |    7 -
> > > >   xen/include/xen/hvm/ioreq.h     |   89 +++
> > > >   18 files changed, 1575 insertions(+), 1450 deletions(-)
> > > That's quite a lot of code moved in a single patch. How can we check 
> > > the code moved is still correct? Is it a verbatim copy?
> > In this patch I mostly tried to separate a common IOREQ part which 
> > also resulted in updating x86 sources to include new header.  Also I 
> > moved hvm_ioreq_needs_completion() to common header (which probably 
> > wanted to be in a separate patch). It was a verbatim copy initially 
> > (w/o hvm_map_mem_type_to_ioreq_server) and then updated to deal with 
> > arch specific parts.
> I would prefer if the two parts are done separately. IOW, the code 
> movement be nearly a verbatim copy.
> > In which way do you want me to split this patch?
> >
> > I could think of the following:
> >
> > 1. Copy of x86's ioreq.c/ioreq.h to common code > 2. Update common 
> > ioreq.c/ioreq.h
> > 3. Update x86's parts to be able to deal with common code
> > 4. Move hvm_ioreq_needs_completion() to common code
> Ideally the code movement should be done in the same patch. This helps 
> to check the patch is only moving code and also avoids mistakes on 
> rebase.
> So my preference would be to first modify the x86 code (e.g. renaming) 
> to make it common and then have one patch that will move the code.
ok, will try to split accordingly. Thank you


--
Regards,

Oleksandr Tyshchenko