[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi,

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.

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.