February 2015 - coreboot - mail.coreboot.org

Re: [coreboot] Unifying IO accessor macros
by Alexandru Gagniuc Feb. 18, 2015

Feb. 18, 2015

On Wednesday, February 18, 2015 05:14:15 PM ron minnich wrote: > Somebody make a decision and go for it. Just make sure it's type safe so we > can catch it when I get it backwards. write[8|16|32|64](void *addr, uint[8|16|32|64]_t val); There. It's done :) > Sorry I have been absent, somehow I got unsubscribed and did not notice for > a while ... > Same thing happened to me. Alex

1 0

Re: [coreboot] Unifying IO accessor macros
by Alexandru Gagniuc Feb. 18, 2015

Feb. 18, 2015

On Wednesday, February 18, 2015 08:23:08 AM Vadim Bendebury wrote: > On Wed, Feb 18, 2015 at 7:16 AM, Aaron Durbin via coreboot > > > As I have noted on http://review.coreboot.org/#/c/7924/ it's very > > short sighted to go this route. In assembling a coreboot stack (which > > includes libpayload and the payload itself) the code usually comes > > from different software systems. Those include libpayload, linux > > kernel, u-boot, etc. They all have the write(val, addr) semantics. I > > see no good reason to artificially erect an ever present barrier for > > integrating code into a coreboot system. > > This is a great reason to keep those accessors, IMO it tramps other > considerations voiced on this thread. Let's be consistent with other > software systems. > Following that reasoning, we should switch to EFI style so we can better integrate vendorcode: read32 -> LibVendorMemRead(..., AccessWidth32, ...) Doing what other software stacks do without a real reason, and just because it's trendy... Alex

2 1

Re: [coreboot] Unifying IO accessor macros
by Alexandru Gagniuc Feb. 18, 2015

Feb. 18, 2015

On Wednesday, February 18, 2015 04:41:13 PM ron minnich wrote: > Having spent 15 or so years on Plan 9 and Linux, the only conclusion I can > make is that nothing is universal. > > Plan 9 is this: > outl(int port, ulong value) > Finally an implementation that makes sense. Those out[lbw]() accessors always get me. > Linux got it backwards, > Thank you! Alex

1 0

Re: [coreboot] Unifying IO accessor macros
by Kevin Herbert Feb. 18, 2015

Feb. 18, 2015

Once the order is the same on all architectures, we could change it to match Linux at any point. I think its important to stop holding up architectural unification in the name of the perfect solution. The Coccinelle to switch the ordering is much safer to do after you have the ordering consistent and both arguments to the write functions are types which are incompatible with each other. Personally I am in the camp of having the same order as Linux, but there is a lot of ARM work held up because they are *different*. This is the problem I am trying to solve. Lets make the best community decision regardless of our personal opinions. Kevin > On Feb 18, 2015, at 8:40 AM, Aaron Durbin via coreboot <coreboot(a)coreboot.org> wrote: > > On Wed, Feb 18, 2015 at 10:35 AM, Patrick Georgi <pgeorgi(a)google.com <mailto:pgeorgi@google.com>> wrote: >> 2015-02-18 17:23 GMT+01:00 Vadim Bendebury <vbendeb(a)chromium.org>: >>>> kernel, u-boot, etc. They all have the write(val, addr) semantics. I >>>> see no good reason to artificially erect an ever present barrier for >>>> integrating code into a coreboot system. >>>> >> Since all imported code requires some rework before it works for us, >> and redoing that particular issue is a matter of a simple spatch >> (http://coccinelle.lip6.fr/) I'm not sure that's actually that big a >> concern. >> >> @@ >> expression a,v; >> @@ >> -writel(v,a) >> +write32(a, v) > > Can you provide the spatch utility and script as well as the > documentation so that every potential contributor can perform the > necessary steps? Or we could be done w/ it once and for all and fix > the existing coreboot semantics. Then there's not another step for > anyone in perpetuity. Also keep in mind it's not just software going > into coreboot but reusing software within coreboot as well. It's a 2 > way street. > > I did volunteer to do the big change, and if people want I will. Let me know. > > -Aaron > > -- > coreboot mailing list: coreboot(a)coreboot.org <mailto:coreboot@coreboot.org> > http://www.coreboot.org/mailman/listinfo/coreboot <http://www.coreboot.org/mailman/listinfo/coreboot>

1 0

Re: [coreboot] Unifying IO accessor macros
by ron minnich Feb. 18, 2015

Feb. 18, 2015

Having spent 15 or so years on Plan 9 and Linux, the only conclusion I can make is that nothing is universal. Plan 9 is this: outl(int port, ulong value) And I used software that came long before Linux (back in the 70s, if you want to know) that followed that form too. I think Linux got it backwards, but it's the standard, so I guess that's what people want. I don't really care at this point, as long as whatever we use is defined with a correct prototype, so we don't get errors when people from other projects also get it "backwards", like me. It's just not that big a deal. Let's stop fussing about it. ron

1 0

Re: [coreboot] Unifying IO accessor macros
by Aaron Durbin Feb. 18, 2015

Feb. 18, 2015

On Wed, Feb 18, 2015 at 10:35 AM, Patrick Georgi <pgeorgi(a)google.com> wrote: > 2015-02-18 17:23 GMT+01:00 Vadim Bendebury <vbendeb(a)chromium.org>: >>> kernel, u-boot, etc. They all have the write(val, addr) semantics. I >>> see no good reason to artificially erect an ever present barrier for >>> integrating code into a coreboot system. >>> > Since all imported code requires some rework before it works for us, > and redoing that particular issue is a matter of a simple spatch > (http://coccinelle.lip6.fr/) I'm not sure that's actually that big a > concern. > > @@ > expression a,v; > @@ > -writel(v,a) > +write32(a, v) Can you provide the spatch utility and script as well as the documentation so that every potential contributor can perform the necessary steps? Or we could be done w/ it once and for all and fix the existing coreboot semantics. Then there's not another step for anyone in perpetuity. Also keep in mind it's not just software going into coreboot but reusing software within coreboot as well. It's a 2 way street. I did volunteer to do the big change, and if people want I will. Let me know. -Aaron

1 0

Re: [coreboot] Unifying IO accessor macros
by Patrick Georgi Feb. 18, 2015

Feb. 18, 2015

2015-02-18 17:23 GMT+01:00 Vadim Bendebury <vbendeb(a)chromium.org>: >> kernel, u-boot, etc. They all have the write(val, addr) semantics. I >> see no good reason to artificially erect an ever present barrier for >> integrating code into a coreboot system. >> Since all imported code requires some rework before it works for us, and redoing that particular issue is a matter of a simple spatch (http://coccinelle.lip6.fr/) I'm not sure that's actually that big a concern. @@ expression a,v; @@ -writel(v,a) +write32(a, v) Patrick -- Google Germany GmbH ABC-Str. 19 20354 Hamburg Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores

1 0

Re: [coreboot] Unifying IO accessor macros
by Vadim Bendebury Feb. 18, 2015

Feb. 18, 2015

On Wed, Feb 18, 2015 at 7:16 AM, Aaron Durbin via coreboot <coreboot(a)coreboot.org> wrote: > On Tue, Feb 17, 2015 at 10:44 PM, Alexandru Gagniuc > > As I have noted on http://review.coreboot.org/#/c/7924/ it's very > short sighted to go this route. In assembling a coreboot stack (which > includes libpayload and the payload itself) the code usually comes > from different software systems. Those include libpayload, linux > kernel, u-boot, etc. They all have the write(val, addr) semantics. I > see no good reason to artificially erect an ever present barrier for > integrating code into a coreboot system. > This is a great reason to keep those accessors, IMO it tramps other considerations voiced on this thread. Let's be consistent with other software systems. --vb > Alex, you've clearly stated your opinion you've not justified a reason > for keeping the barrier. > > -- > coreboot mailing list: coreboot(a)coreboot.org > http://www.coreboot.org/mailman/listinfo/coreboot

1 0

Re: [coreboot] Unifying IO accessor macros
by Aaron Durbin Feb. 18, 2015

Feb. 18, 2015

On Wed, Feb 18, 2015 at 9:16 AM, Aaron Durbin <adurbin(a)google.com> wrote: > On Tue, Feb 17, 2015 at 10:44 PM, Alexandru Gagniuc > <mr.nuke.me(a)gmail.com> wrote: >> On Tuesday, February 17, 2015 04:32:18 PM Julius Werner wrote: >>> > The x86 was recently fixed to take in void *. The order of arguments was >>> > discussed before, and the agreement is to use write*(addr, val); >>> > This has the advantage of being consistent with other accessor functions, >>> > such as PCI, and follows the logical C ordering of the assignment >>> > operator. destination = value; >>> >>> Oh, wow... that merged literally just this weekend. Talk about bad >>> timing. I wasn't aware this was going on (and none of the reviewers >>> bothered to tell me -.- ). >>> >> Well, I tried pinging you on IRC a few times, but I didn't realize you prefer >> ML over IRC. Sorry about that. To be fair, Ron did warn me about you being on >> the same tracks. >> >>> Guess that brings me back to the drawing board. I don't generally >>> disagree with your reasonings about why you consider certain patterns >>> better than others... at most I'd say that I don't think it's all that >>> important at the end of the day (writel() has always referred to 32 >>> bits wherever I've seen it yet, and 64 could easily be writeq() if we >>> ever need it), and that doing things the way people are used to from >>> other/bigger projects (like Linux) might ultimately be more useful to >>> us than doing what we consider "the right way". >>> >> Well, we're firmware. We're special. We used to do hardware. Now we mostly >> deal with hardhardhardhardhardhardware. Every little imperfection propagates >> faster than light and multiplies exponentially. Linux doesn't deal with that. >> U-boot doesn't deal with that. Not quite a fair comparison. >> >>> But I really don't have a strong opinion in either direction, I just >>> care about unifying this and making sure there is a single, consistent >>> pattern everywhere. If most people here think write32(void *addr, u32 >>> value) is the right way to go, I can instead switch ARM(64) over to >>> that. It would be a pretty massive change, but since all those boards >>> are Chromebooks for now I'm in a reasonably good position to ensure >>> everything stays working. >>> >> This reverse order hit me like a brick wall when I first started doing ARM on >> coreboot. But I was angry at ARM code for having gotten it wrong. I also hated >> uboot's setbits_le32(), but hey, at least it indicates bitness -- compare to >> setbits_lel(). >> >>> One issue we're left with is libpayload, which I think should >>> definitely use the same pattern as coreboot. Right now it uses >>> writel(v,a) on all architectures (typed on ARM and untyped on x86). I >>> can change the library, but all external payloads will need to change >>> their call sites themselves. Are we okay with that? >> >> Personally, I'm okay with unifying the order/type of arguments. As long as the >> the payloads that we have in-tree are updated, the external ones shouldn't be >> that hard to do at a later time. >> > > > As I have noted on http://review.coreboot.org/#/c/7924/ it's very > short sighted to go this route. In assembling a coreboot stack (which > includes libpayload and the payload itself) the code usually comes > from different software systems. Those include libpayload, linux > kernel, u-boot, etc. They all have the write(val, addr) semantics. I > see no good reason to artificially erect an ever present barrier for > integrating code into a coreboot system. > > Alex, you've clearly stated your opinion you've not justified a reason > for keeping the barrier. Hit "send" accidentally. Anyway, I'd like to hear why it's OK to purposefully keep this barrier in place. Thanks. -Aaron

1 0

Re: [coreboot] Unifying IO accessor macros
by Alexandru Gagniuc Feb. 18, 2015

Feb. 18, 2015

On Tuesday, February 17, 2015 04:32:18 PM Julius Werner wrote: > > The x86 was recently fixed to take in void *. The order of arguments was > > discussed before, and the agreement is to use write*(addr, val); > > This has the advantage of being consistent with other accessor functions, > > such as PCI, and follows the logical C ordering of the assignment > > operator. destination = value; > > Oh, wow... that merged literally just this weekend. Talk about bad > timing. I wasn't aware this was going on (and none of the reviewers > bothered to tell me -.- ). > Well, I tried pinging you on IRC a few times, but I didn't realize you prefer ML over IRC. Sorry about that. To be fair, Ron did warn me about you being on the same tracks. > Guess that brings me back to the drawing board. I don't generally > disagree with your reasonings about why you consider certain patterns > better than others... at most I'd say that I don't think it's all that > important at the end of the day (writel() has always referred to 32 > bits wherever I've seen it yet, and 64 could easily be writeq() if we > ever need it), and that doing things the way people are used to from > other/bigger projects (like Linux) might ultimately be more useful to > us than doing what we consider "the right way". > Well, we're firmware. We're special. We used to do hardware. Now we mostly deal with hardhardhardhardhardhardware. Every little imperfection propagates faster than light and multiplies exponentially. Linux doesn't deal with that. U-boot doesn't deal with that. Not quite a fair comparison. > But I really don't have a strong opinion in either direction, I just > care about unifying this and making sure there is a single, consistent > pattern everywhere. If most people here think write32(void *addr, u32 > value) is the right way to go, I can instead switch ARM(64) over to > that. It would be a pretty massive change, but since all those boards > are Chromebooks for now I'm in a reasonably good position to ensure > everything stays working. > This reverse order hit me like a brick wall when I first started doing ARM on coreboot. But I was angry at ARM code for having gotten it wrong. I also hated uboot's setbits_le32(), but hey, at least it indicates bitness -- compare to setbits_lel(). > One issue we're left with is libpayload, which I think should > definitely use the same pattern as coreboot. Right now it uses > writel(v,a) on all architectures (typed on ARM and untyped on x86). I > can change the library, but all external payloads will need to change > their call sites themselves. Are we okay with that? Personally, I'm okay with unifying the order/type of arguments. As long as the the payloads that we have in-tree are updated, the external ones shouldn't be that hard to do at a later time. Alex

2 1