Re: [PATCH v2 5/7] virt: geniezone: Add irqchip support for virtual interrupt injection

[Marc Zyngier]

On Fri, 12 May 2023 09:16:58 +0100,
"Yi-De Wu (吳一德)" <Yi-De.Wu@xxxxxxxxxxxx> wrote:
> 
> On Fri, 2023-05-12 at 08:51 +0100, Marc Zyngier wrote:
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > 
> > 
> > On Fri, 12 May 2023 08:19:31 +0100,
> > "Yi-De Wu (吳一德)" <Yi-De.Wu@xxxxxxxxxxxx> wrote:
> > > 
> > > On Fri, 2023-04-28 at 19:59 +0100, Marc Zyngier wrote:
> > > > External email : Please do not click links or open attachments
> > > > until
> > > > you have verified the sender or the content.
> > > > 
> > > > 
> > > > On 2023-04-28 11:36, Yi-De Wu wrote:
> > > > > From: "Yingshiuan Pan" <yingshiuan.pan@xxxxxxxxxxxx>
> > > > > 
> > > > > Enable GenieZone to handle virtual interrupt injection request.
> > > > > 
> > > > > Signed-off-by: Yingshiuan Pan <yingshiuan.pan@xxxxxxxxxxxx>
> > > > > Signed-off-by: Yi-De Wu <yi-de.wu@xxxxxxxxxxxx>
> > > > > ---
> > > > >  arch/arm64/geniezone/Makefile       |  2 +-
> > > > >  arch/arm64/geniezone/gzvm_arch.c    | 24 ++++++--
> > > > >  arch/arm64/geniezone/gzvm_arch.h    | 11 ++++
> > > > >  arch/arm64/geniezone/gzvm_irqchip.c | 88
> > > > > +++++++++++++++++++++++++++++
> > > > >  drivers/virt/geniezone/gzvm_vm.c    | 75
> > > > > ++++++++++++++++++++++++
> > > > >  include/linux/gzvm_drv.h            |  4 ++
> > > > >  include/uapi/linux/gzvm.h           | 38 ++++++++++++-
> > > > >  7 files changed, 235 insertions(+), 7 deletions(-)
> > > > >  create mode 100644 arch/arm64/geniezone/gzvm_irqchip.c
> > > > 
> > > > [...]
> > > > 
> > > > > +++ b/arch/arm64/geniezone/gzvm_irqchip.c
> > > > > @@ -0,0 +1,88 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Copyright (c) 2023 MediaTek Inc.
> > > > > + */
> > > > > +
> > > > > +#include <linux/irqchip/arm-gic-v3.h>
> > > > > +#include <kvm/arm_vgic.h>
> > > > 
> > > > NAK.
> > > > 
> > > > There is no way you can rely on anything from KVM in
> > > > your own hypervisor code.
> > > > 
> > > 
> > > Same with previous discussion, we'd like to copy or rename the
> > > related
> > > part from KVM and keep the maintainance at our own if it's ok.
> > 
> > Why do you need *ANY* of the KVM stuff? Please fully enumerate these
> > dependencies and why you have them.
> > 
> > Directly using KVM stuff for something completely unrelated is not
> > OK,
> > and will never be.
> > 
> >         M.
> > 
> > --
> > Without deviation from the norm, progress is not possible.
> 
> The particular part we'd like to leverage from KVM are as followed
> 1. `gfn_to_pfn_memslot` to convert guest physical address(or
> intermediate physical address) to physical address

What is a memslot in your hypervisor? How does it relate to KVM's?
What about the use of struct kvm?

I'm sorry, but your use of *internal* structures and API would make it
impossible for us to make any further change without potentially
affecting your hypervisor. Which is closed source and untestable.

To sum it up, I'm strongly opposed to any use of these data
structures.  If you can spot some commonalities, expose them as a
0-cost abstraction. But don't use them as is your code.

> 2. get ARM's number of interrupt of different types e.g. number of SGI,
> number of PPI...etc

These are architectural constants, and you can define your own. That
will cost you nothing but a handful of #define, and keep the two
subsystem independent.

	M.

-- 
Without deviation from the norm, progress is not possible.