On Friday, October 8, 2021 12:17 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > Can you put some specifics in the subject line, please? If this patch > does several things, that might be an indication that it should be > split into several patches. > Will do in v3. But, this patch only makes the current Hyper-V vPCI code architectural neutral by adding those interfaces that you mention below. I will update the wording to make this clear in v3. thanks for the suggestion. > > This patch makes the Hyper-V vPCI code architectural neutral by > > introducing an irqchip that takes care of architectural > > dependencies. This allows for the implementation of Hyper-V vPCI > > for other architecture such as ARM64. > > No need to include "This patch"; we already know we're talking about > this patch. > > Write in "imperative mood", e.g., "Encapsulate arch dependencies in > X ..." instead of "This patch makes the code ...". See > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fchris.be > ams.io%2Fposts%2Fgit- > commit%2F&data=04%7C01%7Csunilmut%40microsoft.com%7Cb40962bf1 > 5614957781608d98a9030c8%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C > 0%7C637693174187863024%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA > wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sda > ta=XlrM%2Bzg92v%2BXxXObWQbznaB%2F6ZW6u79NtDoG4FfyhXw%3D&r > eserved=0 > > Wrap the text to fill 75 columns. > All of the above points noted; thanks. Will address in v3. > You said this "introduces an irqchip", but I don't see a new > struct irq_chip or similar. > > The important part about making this arch-neutral seems to be adding > these interfaces that will encapsulate arch dependencies: > > hv_pci_irqchip_init() > hv_pci_irqchip_free() > hv_msi_get_int_vector() > hv_set_msi_entry_from_desc() > hv_msi_prepare() > Yes, the wording of 'irqchip' is a bit misleading for this patch as it gets added in 2/2. Thanks for the suggestion; I will reword in v3. > I'm not sure wrapping them in "#ifdef CONFIG_X86_64" is the best > approach, but the IRQ folks will know better. > Hyper-V vPCI is not firmware enumerated and so we cannot take a dependency on the firmware to dictate whether or not to instantiate an irqchip or not. I am open to suggestions though. > > +++ b/drivers/pci/controller/pci-hyperv-irqchip.c > > @@ -0,0 +1,51 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > Spurious blank line (follow style of nearby files). > Noted, thanks. Will fix in v3. > > +/* > > + * Hyper-V vPCI irqchip. > > > +++ b/drivers/pci/controller/pci-hyperv-irqchip.h > > @@ -0,0 +1,21 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > + > > Spurious blank line (follow style of nearby files). > Noted, thanks. Will fix in v3. - Sunil
