Date:	Wed, 16 Dec 2009 09:23:15 +0800
From:	Huang Ying <ying.huang@...el.com>
To:	Bjorn Helgaas <bjorn.helgaas@...com>
Cc:	"lenb@...nel.org" <lenb@...nel.org>,
	ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
	Andi Kleen <ak@...ux.intel.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATH 1/5 -v2] acpi, IO memory pre-mapping and atomic accessing

On Wed, 2009-12-16 at 01:47 +0800, Bjorn Helgaas wrote: 
> On Monday 14 December 2009 06:04:13 pm Huang Ying wrote:
> > On Sat, 2009-12-12 at 01:36 +0800, Bjorn Helgaas wrote: 
> > > I think your code would be simpler if acpi_pre_map_gar() returned a
> > > struct acpi_iomap pointer (from the caller's point of view, this would
> > > be an opaque cookie).  Then you could just supply that cookie to
> > > acpi_atomic_write(), and you wouldn't have to look it up again.  Maybe
> > > you could even get rid of the list and all the fancy RCU & kref stuff
> > > then, too.
> > 
> > The interface chosen is based on usage model, which is:
> > 
> > 1. In init function, all GARs needed are pre-mapped
> > 2. In atomic context, pre-mapped GARs are accessed
> > 3. In exit function, all GARs are post-unmapped
> > 
> > In 3), if struct acpi_iomap* is used as parameter for post-unmap
> > function, we need to record that pointer in another list. In 2), we need
> > find mapped address from GAR.
> 
> I understand that my proposal would require a slight change in your
> usage model.  I am suggesting that you make it follow the same pattern
> as pci_iomap(), e.g.:
> 
> 	void *pci_iomap(struct pci_dev *, int bar, unsigned long len);
> 	unsigned int ioread32(void *);
> 	void pci_iounmap(struct pci_dev *, void *);
> 
> 	void *acpi_map_generic_address(struct acpi_generic_address *);
> 	u64 acpi_read_atomic(void *);
> 	void acpi_unmap_generic_address(void *);
> 
> acpi_map_generic_address() would validate the GAR and do the ioremap().
> If the validation or ioremap() failed, it would return a NULL pointer.
> 
> This would require the caller of acpi_map_generic_address() to hang onto
> the pointer returned (just as callers of pci_iomap() must hang onto the
> pointer it returns).
> 
> The pointer would be supplied to acpi_read_atomic(), so it would not
> need to do acpi_check_gar() because we know it was done successfully
> in acpi_map_generic_address().  It wouldn't need to look up the GAR
> in the list because the list entry was passed in.  Since all the
> possible failure conditions were checked in acpi_map_generic_address(),
> acpi_read_atomic() doesn't need to return status and could simply
> return the u64 directly.

The usage model is different. Please take a look at
__apei_exec_read_register(). We need to get virtual address from
physical address in GAR. And many small-size GARs in ERST or EINJ may
share same page, so some kind of virtual space optimization is
necessary.

> > > > +/* In NMI handler, should set silent = 1 */
> > > > +static int acpi_check_gar(struct acpi_generic_address *reg,
> > > > +			  u64 *paddr, int silent)
> > > > +{
> > > > +	u32 width;
> > > > +
> > > > +	/* Handle possible alignment issues */
> > > > +	memcpy(paddr, &reg->address, sizeof(*paddr));
> > > > +	if (!*paddr) {
> > > > +		if (!silent)
> > > > +			pr_info(
> > > > +			"Invalid physical address in GAR, firmware bug?\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	width = reg->bit_width;
> > > > +	if ((width != 8) && (width != 16) && (width != 32) && (width != 64)) {
> > > > +		if (!silent)
> > > > +			pr_info(
> > > > +			"Invalid bit width in GAR, firmware bug?\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY &&
> > > > +	    reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO) {
> > > > +		if (!silent)
> > > > +			pr_info(
> > > > +			"Invalid address space type in GAR, firmware bug?\n");
> > > 
> > > Error messages with constant text are nearly useless because they
> > > don't give much of a clue about where to look for a problem.
> > > Personally, for something this, I would just return failure and
> > > never print anything.  If a map fails, the caller should notice
> > > and you then have a good idea of where to look.
> > 
> > The checking here is for bug in firmware not software. I think it is
> > necessary for the user to know where the bugs may come from, and it is
> > hard to express the bug in return code.
> 
> Yes, I understand that this is checking for firmware bugs.  My point
> is that when a user sees this in his dmesg log:
> 
> 	Invalid bit width in GAR, firmware bug?
> 
> we have no context, and even a kernel developer can't figure out what
> to do.  We could ask for a copy of the FADT and DSDT, but even then,
> we don't know *which* GAR structure to look at, so we'll probably have
> to add some instrumentation and ask the user to reproduce the problem.
> 
> If the check were in the caller, it could at least say something like:
> 
> 	ACPI: couldn't map generic address [io 0xcf8] for PCI config access
> 
> which would give us more useful information.

En... Yes, some information about the invalid GAR is helpful. But the
GAR information is available in acpi_check_gar too, so we can output
something as follow in acpi_check_gar:

    Invalid bit width in GAR [mem 0x8029ff8 24], firmware bug?

Your message looks like a software issue instead of firmware bug. This
may confuse the user and developer.

Best Regards,
Huang Ying


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists