Patchworkβ ia64: Don't call SAL < 3.2 for extended config space

On Mon, Oct 12, 2009 at 09:56:28AM -0400, Brad Spengler wrote:
> > So how about we go back to adding that check ... this does require that
> > SGI's 750 machine reports its SAL revision correctly.  Could you send
> > the dmesg?
> 
> The dmesg of the working kernel:
> http://grsecurity.net/~spender/dmesg.txt

Thanks, that's reporting SAL 3.0, so it will be correctly caught by
this patch:

---

From: Matthew Wilcox <willy@linux.intel.com>
Subject: Require SAL 3.2 in order to do extended config space ops

We had assumed that SAL firmware would return an error if it didn't
understand extended config space.  Unfortunately, the SAL on the SGI 750
doesn't do that, it panics the machine.  So, condition the extended PCI
config space accesses on SAL revision 3.2.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>

> +	} else if (sal_revision >= SAL_VERSION_CODE(3,2))

Looks like a missing ending { here ^
> +	} else if (sal_revision >= SAL_VERSION_CODE(3,2))
and here ^

I'll test it to confirm it fixes the problem.

-Brad

I've confirmed that the below patch (with the syntax fixes already 
mentioned) resolves the issue on the SGI 750.

-Brad

> From: Matthew Wilcox <willy@linux.intel.com>
> Subject: Require SAL 3.2 in order to do extended config space ops
> 
> We had assumed that SAL firmware would return an error if it didn't
> understand extended config space.  Unfortunately, the SAL on the SGI 750
> doesn't do that, it panics the machine.  So, condition the extended PCI
> config space accesses on SAL revision 3.2.
> 
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> 
> diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c
> index 7de76dd..61363cc 100644
> --- a/arch/ia64/pci/pci.c
> +++ b/arch/ia64/pci/pci.c
> @@ -56,10 +56,13 @@ int raw_pci_read(unsigned int seg, unsigned int bus, unsigned int devfn,
>  	if ((seg | reg) <= 255) {
>  		addr = PCI_SAL_ADDRESS(seg, bus, devfn, reg);
>  		mode = 0;
> -	} else {
> +	} else if (sal_revision >= SAL_VERSION_CODE(3,2))
>  		addr = PCI_SAL_EXT_ADDRESS(seg, bus, devfn, reg);
>  		mode = 1;
> +	} else {
> +		return -EINVAL;
>  	}
> +
>  	result = ia64_sal_pci_config_read(addr, mode, len, &data);
>  	if (result != 0)
>  		return -EINVAL;
> @@ -80,9 +83,11 @@ int raw_pci_write(unsigned int seg, unsigned int bus, unsigned int devfn,
>  	if ((seg | reg) <= 255) {
>  		addr = PCI_SAL_ADDRESS(seg, bus, devfn, reg);
>  		mode = 0;
> -	} else {
> +	} else if (sal_revision >= SAL_VERSION_CODE(3,2))
>  		addr = PCI_SAL_EXT_ADDRESS(seg, bus, devfn, reg);
>  		mode = 1;
> +	} else {
> +		return -EINVAL;
>  	}
>  	result = ia64_sal_pci_config_write(addr, mode, len, value);
>  	if (result != 0)
> 
> -- 
> Matthew Wilcox				Intel Open Source Technology Centre
> "Bill, look, we understand that you're interested in selling us this
> operating system, but compare it to ours.  We can't possibly take such
> a retrograde step."

Ah, the SGI 750.  Brings back memories.

I've dropped the ball on this one though; maybe Tony already picked it
up?  Tony?

Thanks,
Jesse

On Tue, 13 Oct 2009 00:58:08 -0400
spender@grsecurity.net (Brad Spengler) wrote:

> I've confirmed that the below patch (with the syntax fixes already 
> mentioned) resolves the issue on the SGI 750.
> 
> -Brad
> 
> > From: Matthew Wilcox <willy@linux.intel.com>
> > Subject: Require SAL 3.2 in order to do extended config space ops
> > 
> > We had assumed that SAL firmware would return an error if it didn't
> > understand extended config space.  Unfortunately, the SAL on the
> > SGI 750 doesn't do that, it panics the machine.  So, condition the
> > extended PCI config space accesses on SAL revision 3.2.
> > 
> > Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> > 
> > diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c
> > index 7de76dd..61363cc 100644
> > --- a/arch/ia64/pci/pci.c
> > +++ b/arch/ia64/pci/pci.c
> > @@ -56,10 +56,13 @@ int raw_pci_read(unsigned int seg, unsigned int
> > bus, unsigned int devfn, if ((seg | reg) <= 255) {
> >  		addr = PCI_SAL_ADDRESS(seg, bus, devfn, reg);
> >  		mode = 0;
> > -	} else {
> > +	} else if (sal_revision >= SAL_VERSION_CODE(3,2))
> >  		addr = PCI_SAL_EXT_ADDRESS(seg, bus, devfn, reg);
> >  		mode = 1;
> > +	} else {
> > +		return -EINVAL;
> >  	}
> > +
> >  	result = ia64_sal_pci_config_read(addr, mode, len, &data);
> >  	if (result != 0)
> >  		return -EINVAL;
> > @@ -80,9 +83,11 @@ int raw_pci_write(unsigned int seg, unsigned int
> > bus, unsigned int devfn, if ((seg | reg) <= 255) {
> >  		addr = PCI_SAL_ADDRESS(seg, bus, devfn, reg);
> >  		mode = 0;
> > -	} else {
> > +	} else if (sal_revision >= SAL_VERSION_CODE(3,2))
> >  		addr = PCI_SAL_EXT_ADDRESS(seg, bus, devfn, reg);
> >  		mode = 1;
> > +	} else {
> > +		return -EINVAL;
> >  	}
> >  	result = ia64_sal_pci_config_write(addr, mode, len, value);
> >  	if (result != 0)
> > 
> > -- 
> > Matthew Wilcox				Intel Open Source
> > Technology Centre "Bill, look, we understand that you're interested
> > in selling us this operating system, but compare it to ours.  We
> > can't possibly take such a retrograde step."

> I've dropped the ball on this one though; maybe Tony already picked it
> up?  Tony?

Yes.  It went in to Linus' tree with the git pull I sent on Monday (just
prior to -rc6).

Commit: adcd74034 ...

-Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 4 Nov 2009 09:20:13 -0800
"Luck, Tony" <tony.luck@intel.com> wrote:

> > I've dropped the ball on this one though; maybe Tony already picked
> > it up?  Tony?
> 
> Yes.  It went in to Linus' tree with the git pull I sent on Monday
> (just prior to -rc6).
> 
> Commit: adcd74034 ...

Great, thanks for confirming.