Message-ID: <BN8PR21MB1140D7D99F0144D398E2D832C0B99@BN8PR21MB1140.namprd21.prod.outlook.com>
Date:   Fri, 15 Oct 2021 17:47:42 +0000
From:   Sunil Muthuswamy <sunilmut@...rosoft.com>
To:     Michael Kelley <mikelley@...rosoft.com>,
        Sunil Muthuswamy <sunilmut@...ux.microsoft.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        "maz@...nel.org" <maz@...nel.org>,
        Dexuan Cui <decui@...rosoft.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>, "hpa@...or.com" <hpa@...or.com>,
        "lorenzo.pieralisi@....com" <lorenzo.pieralisi@....com>,
        "robh@...nel.org" <robh@...nel.org>, "kw@...ux.com" <kw@...ux.com>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "arnd@...db.de" <arnd@...db.de>
CC:     "x86@...nel.org" <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>
Subject: RE: [PATCH v3 0/2] PCI: hv: Hyper-V vPCI for ARM64

On Thursday, October 14, 2021 8:23 PM,
Michael Kelley <mikelley@...rosoft.com> 
> 
> 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

Thanks for the feedback. Overall, I think it makes sense to keep
the irq chip implementation in a separate file because it will give
us more flexibility in the future to alter the irq chip implementation
or which irq chip we pick as parent (for example, we likely will
parent to the LPI irq chip in future) without having to touch the
core of the pci-hyperv. To me that separation sounds more logical
and beneficial than reducing a few lines of code immediately.

- Sunil

Powered by blists - more mailing lists