RE: [PATCH v3 0/2] PCI: hv: Hyper-V vPCI for ARM64

[Michael Kelley]

From: Sunil Muthuswamy <sunilmut@xxxxxxxxxxxxxxxxxxx> Sent: Thursday, October 14, 2021 8:53 AM
> 
> Current Hyper-V vPCI code only compiles and works for x64. There are some
> hardcoded assumptions about the architectural IRQ chip and other arch
> defines.
> 
> Add support for Hyper-V vPCI for ARM64 by first breaking the current hard
> coded dependency using a set of new interfaces and implementing those for
> X64 first. That is in the first patch. The second patch adds support for
> Hyper-V vPCI for ARM64 by implementing the above mentioned interfaces. That
> is done by introducing a Hyper-V vPCI specific MSI IRQ domain & chip for
> allocating SPI vectors.
> 
> changes in v1 -> v2:
>  - Moved the irqchip implementation to drivers/pci as suggested
>    by Marc Zyngier
>  - Addressed Multi-MSI handling issues identified by Marc Zyngier
>  - Addressed lock/synchronization primitive as suggested by Marc
>    Zyngier
>  - Addressed other code feedback from Marc Zyngier
> 
> changes in v2 -> v3:
>  - Addressed comments from Bjorn Helgaas about patch formatting and
>    verbiage
>  - Using 'git send-email' to ensure that the patch series is correctly
>    threaded. Feedback by Bjorn Helgaas
>  - Fixed Hyper-V vPCI build break for module build, reported by Boqun Feng
> 
> Sunil Muthuswamy (2):
>   PCI: hv: Make the code arch neutral by adding arch specific interfaces
>   arm64: PCI: hv: Add support for Hyper-V vPCI
> 
>  MAINTAINERS                                 |   2 +
>  arch/arm64/include/asm/hyperv-tlfs.h        |   9 +
>  arch/x86/include/asm/hyperv-tlfs.h          |  33 +++
>  arch/x86/include/asm/mshyperv.h             |   7 -
>  drivers/pci/Kconfig                         |   2 +-
>  drivers/pci/controller/Kconfig              |   2 +-
>  drivers/pci/controller/Makefile             |   2 +-
>  drivers/pci/controller/pci-hyperv-irqchip.c | 267 ++++++++++++++++++++
>  drivers/pci/controller/pci-hyperv-irqchip.h |  20 ++
>  drivers/pci/controller/pci-hyperv.c         |  58 +++--
>  include/asm-generic/hyperv-tlfs.h           |  33 ---
>  11 files changed, 373 insertions(+), 62 deletions(-)
>  create mode 100644 drivers/pci/controller/pci-hyperv-irqchip.c
>  create mode 100644 drivers/pci/controller/pci-hyperv-irqchip.h
> 
> 
> base-commit: e4e737bb5c170df6135a127739a9e6148ee3da82
> --
> 2.25.1

At a micro-level, I've reviewed the patch set and could give my
Reviewed-By, though someone more expert on IRQ domains
than I am should definitely review as well.

But I've been thinking about the macro-level organization of
the code, and the handling of the x86 and ARM64 differences.
Short of creating two new .c files, one x86 specific and one
ARM64 specific (which seems like overkill), there's no getting
away from a few #ifdef's, and indeed pci-hyperv.c already
has a couple.  The problem space is just messy.

So if that's the case, then I'm not seeing much value in having
the code in pci-hyperv-irqchip.c broken out into a separate
source code file.  I did some playing around with organizing
the new functionality into the existing pci-hyperv.c with the
needed #ifdef's, and it seems a bit cleaner to me.   The new
.h file is also eliminated, and there are other small simplifications
that can be made by having everything in pci-hyperv.c.   With
this reorg, there are 50+ fewer lines added (though some of
the savings is admittedly just source code file headers).   I
can send you a .diff of the reorg'ed code if you want it.

Also, a good bit of the code under #ifdef ARM64 will compile
just fine on x86, though it wouldn't be used.  It's actually the
ARM64 side that more naturally fits the Linux IRQ domain model,
with the x86 side being the special case because of the quirks of
the x86 interrupt architecture.  When thinking about the code
from that standpoint, it's another reason to put the code all
into pci-hyperv.c.

The best overall structure to use is a judgment call because
there are tradeoffs for any of the choices.  There's no clear
"right" answer.  As such, my preference to combine all the
code into pci-hyperv.c is just a suggestion.  I won't try to hold
up accepting the code if you decide you want to keep the
current structure with separate pci-hyperv-irqchip.[ch] files.

Michael