linux-kernel - Re: [PATCH V4 17/23] acpi, mcfg: Add default PCI config accessors implementation and initial support for related quirks.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20160302104244.GA25547@red-moon>
Date:	Wed, 2 Mar 2016 10:42:44 +0000
From:	Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To:	Jayachandran Chandrashekaran Nair 
	<jayachandran.chandrashekaran@...adcom.com>
Cc:	Tomasz Nowicki <tn@...ihalf.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Arnd Bergmann <arnd@...db.de>,
	Will Deacon <will.deacon@....com>,
	Catalin Marinas <catalin.marinas@....com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Hanjun Guo <hanjun.guo@...aro.org>, okaya@...eaurora.org,
	jiang.liu@...ux.intel.com,
	Stefano Stabellini <Stefano.Stabellini@...citrix.com>,
	robert.richter@...iumnetworks.com, Marcin Wojtas <mw@...ihalf.com>,
	Liviu.Dudau@....com, David Daney <ddaney@...iumnetworks.com>,
	wangyijing@...wei.com, Suravee.Suthikulpanit@....com,
	msalter@...hat.com, linux-pci@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-acpi@...r.kernel.org,
	linux-kernel@...r.kernel.org, linaro-acpi@...ts.linaro.org,
	Jayachandran Chandrashekaran Nair <jchandra@...adcom.com>,
	Jon Masters <jcm@...hat.com>
Subject: Re: [PATCH V4 17/23] acpi, mcfg: Add default PCI config accessors
 implementation and initial support for related quirks.

On Mon, Feb 29, 2016 at 01:33:41PM +0530, Jayachandran Chandrashekaran Nair wrote:
> On Thu, Feb 4, 2016 at 10:58 PM, Tomasz Nowicki <tn@...ihalf.com> wrote:
> > We use generic accessors from access.c by default. However, we already
> > know platforms that need special handling while accessing to PCI config
> > space. These platforms will need different accessors set matched against
> > platform ID, domain, bus touple. Therefore we are going to add (in future)
> > DECLARE_ACPI_MCFG_FIXUP which will register platform specific custom
> > accessors. For now, we let pci_mcfg_get_ops to take acpi_pci_root structure
> > as an arguments and left some space for quirk matching algorithm.
> >
> > Signed-off-by: Tomasz Nowicki <tn@...ihalf.com>
> > Tested-by: Duc Dang <dhdang@....com>
> > Tested-by: Dongdong Liu <liudongdong3@...wei.com>
> > Tested-by: Hanjun Guo <hanjun.guo@...aro.org>
> > Tested-by: Graeme Gregory <graeme.gregory@...aro.org>
> > Tested-by: Sinan Kaya <okaya@...eaurora.org>
> > ---
> >  drivers/acpi/mcfg.c      | 30 ++++++++++++++++++++++++++++++
> >  include/linux/pci-acpi.h |  8 ++++++++
> >  2 files changed, 38 insertions(+)
> >
> > diff --git a/drivers/acpi/mcfg.c b/drivers/acpi/mcfg.c
> > index dca4c4e..dfc2d14 100644
> > --- a/drivers/acpi/mcfg.c
> > +++ b/drivers/acpi/mcfg.c
> > @@ -34,6 +34,36 @@ int __weak raw_pci_write(unsigned int domain, unsigned int bus,
> >         return PCIBIOS_DEVICE_NOT_FOUND;
> >  }
> >
> > +void __iomem *
> > +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
> > +{
> > +       struct pci_mmcfg_region *cfg;
> > +
> > +       cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
> 
> In the existing code, calls to pci_mmconfig_lookup() is done inside an
> rcu_read_lock/rcu_read_unlock pair. Any reason that is not required here?
> 
> Also, you can avoid having to do the lookup every time by saving a cfg pointer
> see http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/396921.html

Yes, this is much better (and sysdata usage is self-contained, it does
not trickle into other bits of the kernel - ie arch code), it should be
integrated in the final version of the patchset.

> > +       if (cfg && cfg->virt)
> > +               return cfg->virt +
> > +                       (PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
> > +                       offset;
> 
> PCI_MMCFG_OFFSET(bus->number, devfn) would be better

Ditto.

Thanks,
Lorenzo

> 
> > +       return NULL;
> > +}
> > +
> > +/* Default generic PCI config accessors */
> > +static struct pci_ops default_pci_mcfg_ops = {
> > +       .map_bus = pci_mcfg_dev_base,
> > +       .read = pci_generic_config_read,
> > +       .write = pci_generic_config_write,
> > +};
> > +
> > +struct pci_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
> > +{
> > +       /*
> > +        * TODO: Match against platform specific quirks and return
> > +        * corresponding PCI config space accessor set.
> > +        */
> > +
> > +       return &default_pci_mcfg_ops;
> > +}
> 
> Is it necessary to make these non-static, even in the next patch, the
> functions are only used from this file.
> 
> > +
> >  int __init acpi_parse_mcfg(struct acpi_table_header *header)
> >  {
> >         struct acpi_table_mcfg *mcfg;
> > diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> > index 65b91f3..c974586 100644
> > --- a/include/linux/pci-acpi.h
> > +++ b/include/linux/pci-acpi.h
> > @@ -83,10 +83,18 @@ void acpi_pci_remove_bus(struct pci_bus *bus);
> >  #ifdef CONFIG_PCI_MMCONFIG
> >  int pci_mmcfg_setup_map(struct acpi_pci_root_info *ci);
> >  void pci_mmcfg_teardown_map(struct acpi_pci_root_info *ci);
> > +struct pci_ops *pci_mcfg_get_ops(struct acpi_pci_root *root);
> > +void __iomem *
> > +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset);
> >  #else
> >  static inline int pci_mmcfg_setup_map(struct acpi_pci_root_info *ci)
> >  { return 0; }
> >  static inline void pci_mmcfg_teardown_map(struct acpi_pci_root_info *ci) { }
> > +static inline struct pci_ops *pci_mcfg_get_ops(struct acpi_pci_root *root)
> > +{ return NULL; }
> > +static inline void __iomem *
> > +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
> > +{ return NULL; }
> >  #endif
> >  #ifdef CONFIG_ACPI_PCI_SLOT
> 
> JC.
>