- Download Diff

Summary:

10235 uts: boot needs simple tem to support mdb

Review Request #1367 — Created Jan. 14, 2019 and submitted Jan. 30, 2019, 9:04 p.m.

Information
Owner:	tsoome
Repository:	illumos-gate
Branch:
Bugs:	10235
Depends On:	~~1366~~
Commit:	c09d01f...
Reviewers
Groups:	general
People:

Description

10235 uts: boot needs simple tem to support mdb

Testing Done

Issues

0
0
6
6
All Issues: 12

Description	From	Last Updated
I've been trying to go through this and think I'm missing something. I see that we often set btem_params[] to ...	rm	Jan. 25, 2019, 11:40 a.m.
Should we make it >= just in case we ever get into a state where cols is in a bad ...	rm	Jan. 25, 2019, 12:31 p.m.
Why do we no longer need the equivalent of the \r of \b cases here?	rm	Jan. 25, 2019, 12:22 p.m.
Doesn't this conversion exist elsewhere? Do we need to duplicate it again?	rm	Jan. 25, 2019, 11:50 a.m.
Where does the 1 << 5 come from?	rm	Jan. 25, 2019, 11:57 a.m.
We're only going to take the lower 8 bits of color, should that actually be an int argument or should ...	rm	Jan. 25, 2019, 12:11 p.m.
Should we guard against this being negative?	rm	Jan. 25, 2019, 12:34 p.m.
Should we cast cons_color to a uint8_t explicitly? Should c be cast to a uint16_t to indicate that we're only ...	rm	Jan. 25, 2019, 12:43 p.m.
Why don't we consider both the rows and columns cases separately? Can't both end up being true?	rm	Jan. 25, 2019, 3:36 p.m.
Is it possible to use the standard function isdigit() here? if (isdigit(c)) {	domag02	Jan. 28, 2019, 4:49 p.m.
's/unsignned/unsigned/'	domag02	Jan. 28, 2019, 7:55 a.m.
's/trunkation/truncation/'	domag02	Jan. 28, 2019, 7:55 a.m.

Change Summary:

rebase on updated 10234.
export boot_get_color() to reduce local color arrays.

implement FB clear screen with boot_fb_eraseline_impl()

Commit:

-	ab46ba70ff6cea85d871af3310d6f07a1d2db97c
+	38e9b66cd5b5566335c1922372aa52f5a34bcf73

Diff:

Revision 2 (+548 -289)

Show changes

	usr/src/uts/i86pc/boot/boot_console.c
	usr/src/uts/i86pc/boot/boot_console_impl.h
	usr/src/uts/i86pc/boot/boot_fb.c
	usr/src/uts/i86pc/boot/boot_vga.c

Commit:

-	38e9b66cd5b5566335c1922372aa52f5a34bcf73
+	564481777abc91ab30a5bcd13b985654819ca5f4

Diff:

Revision 3 (+546 -290)

Show changes

	usr/src/uts/i86pc/boot/boot_console.c
	usr/src/uts/i86pc/boot/boot_console_impl.h
	usr/src/uts/i86pc/boot/boot_fb.c
	usr/src/uts/i86pc/boot/boot_vga.c

Commit:

-	564481777abc91ab30a5bcd13b985654819ca5f4
+	cd543bad962d13520faabcbcee79c460428ee8e3

Diff:

Revision 4 (+546 -290)

Show changes

	usr/src/uts/i86pc/boot/boot_console.c
	usr/src/uts/i86pc/boot/boot_console_impl.h
	usr/src/uts/i86pc/boot/boot_fb.c
	usr/src/uts/i86pc/boot/boot_vga.c

Commit:

-	cd543bad962d13520faabcbcee79c460428ee8e3
+	de38df957e97fc040095d90c958e01b988b4595a

Diff:

Revision 5 (+542 -288)

Show changes

	usr/src/uts/i86pc/boot/boot_console.c
	usr/src/uts/i86pc/boot/boot_console_impl.h
	usr/src/uts/i86pc/boot/boot_fb.c
	usr/src/uts/i86pc/boot/boot_vga.c

Looks good in general, but I have some notes before.

usr/src/uts/i86pc/boot/boot_console.c (Diff revision 5)

Show all issues

I've been trying to go through this and think I'm missing something. I see that we often set btem_params[] to have more values; however, I only see us consuming btem_params[0].

tsoome Jan. 25, 2019, 3:36 p.m.

Yes, I did kept the original size for array, but we only do implement the minimal set of terminal functions for mdb, as mdb is the only consumer for now. Depending on if we might decide to expand the features in the future, we will have the base functionality in place. For example, one possible feature could be to implement colors to be used with debug printouts.

usr/src/uts/i86pc/boot/boot_console.c (Diff revision 5)

Show all issues

Should we make it >= just in case we ever get into a state where cols is in a bad state and would cause us to go beyond fb_info.terminal.x?

usr/src/uts/i86pc/boot/boot_fb.c (Diff revision 5)

Show all issues

Why do we no longer need the equivalent of the \r of \b cases here?

tsoome Jan. 25, 2019, 3:36 p.m.

we do add the \r in bcons_putchar() and it is handled by tem.

usr/src/uts/i86pc/boot/boot_vga.c (Diff revision 5)

Name seems a little confusing, as nothing is actually set.

tsoome Jan. 25, 2019, 3:36 p.m.

yes, renamed to get_vga_color(). It was in boot_console.c before.

usr/src/uts/i86pc/boot/boot_vga.c (Diff revision 5)

Show all issues

Doesn't this conversion exist elsewhere? Do we need to duplicate it again?

tsoome Jan. 25, 2019, 3:36 p.m.

In this patch we did move it from boot_console.c, later, we will collect all those instances and remove duplication. We already do remove a small bit of duplication of dim_xlate and brt_xlate arrays. However, this specific array is only needed for VGA text case, to translate sun colors for vga.

usr/src/uts/i86pc/boot/boot_vga.c (Diff revision 5)
Show all issues
```
Where does the 1 << 5 come from?
```
1. tsoome Jan. 25, 2019, 3:36 p.m.
  see the comment above:) the bit 5 is cursor disable.

usr/src/uts/i86pc/boot/boot_vga.c (Diff revision 5)

Show all issues

We're only going to take the lower 8 bits of color, should that actually be an int argument or should it be a uint8_t?

tsoome Jan. 25, 2019, 3:36 p.m.

you mean the color value itself? the vga text memory is array of 16-bit words, upper 8-bits are for foreground and background colors (4-bit values), lower 8-bits are for char to be displayed. If we do define color as uint8_t, we would need to deal with type casting for shift. I'm not sure if the uint8_t would add here enough benefit as we only set the color via translation checks anyhow.

usr/src/uts/i86pc/boot/boot_vga.c (Diff revision 5)
Show all issues
```
Should we guard against this being negative?
```

usr/src/uts/i86pc/boot/boot_vga.c (Diff revision 5)

Show all issues

Should we cast cons_color to a uint8_t explicitly? Should c be cast to a uint16_t to indicate that we're only taking the lower bits?

tsoome Jan. 25, 2019, 3:36 p.m.

VGA_SCREEN is defined as 16-bit unsigned int array and the compiler will do the trunkation. But of course it does not hurt to have the cast as an reminder there - except it does add noise. I did add comment instead:)

usr/src/uts/i86pc/boot/boot_vga.c (Diff revision 5)

Show all issues

Why don't we consider both the rows and columns cases separately? Can't both end up being true?

tsoome Jan. 25, 2019, 3:36 p.m.

Well, if we are not at the end of the line, then displaying one char will advance us by one column, no rows are advanced. If we are at the end of the line, then we output at the start of the next row. The only case when we could advance both column and row, is when we are at the end of the last row; but then we scroll the screen up and stay at the beginning of the last row.

Change Summary:

usr/src/uts/i86pc/boot/boot_console_impl.h: line 16: Invalid or missing header guard
usr/src/uts/i86pc/boot/boot_console_impl.h: Missing or invalid ending header guard

Commit:

-	de38df957e97fc040095d90c958e01b988b4595a
+	b01dec646d529ba2105f6a1ec758269c59c5c322

Diff:

Revision 6 (+545 -291)

Show changes

	usr/src/uts/i86pc/boot/boot_console.c
	usr/src/uts/i86pc/boot/boot_console_impl.h
	usr/src/uts/i86pc/boot/boot_fb.c
	usr/src/uts/i86pc/boot/boot_vga.c

Change Summary:

notes from Robert.

Commit:

-	b01dec646d529ba2105f6a1ec758269c59c5c322
+	7a3bd4fc8579e60052cfc945227357c1f166fc9b

Diff:

Revision 7 (+551 -291)

Show changes

	usr/src/uts/i86pc/boot/boot_console.c
	usr/src/uts/i86pc/boot/boot_console_impl.h
	usr/src/uts/i86pc/boot/boot_fb.c
	usr/src/uts/i86pc/boot/boot_vga.c

```
Typos.
```
usr/src/uts/i86pc/boot/boot_vga.c (Diff revision 7)
Show all issues
```
's/unsignned/unsigned/'
```
usr/src/uts/i86pc/boot/boot_vga.c (Diff revision 7)
Show all issues
```
's/trunkation/truncation/'
```

Change Summary:

typos fixed.

Commit:

-	7a3bd4fc8579e60052cfc945227357c1f166fc9b
+	0dedbc362cfc6021a74d87ae3b1137347b7f4d85

Diff:

Revision 8 (+551 -291)

Show changes

	usr/src/uts/i86pc/boot/boot_console.c
	usr/src/uts/i86pc/boot/boot_console_impl.h
	usr/src/uts/i86pc/boot/boot_fb.c
	usr/src/uts/i86pc/boot/boot_vga.c

usr/src/uts/i86pc/boot/boot_console.c (Diff revision 7)

Show all issues

Is it possible to use the standard function isdigit() here?
    if (isdigit(c)) {

tsoome Jan. 28, 2019, 4:50 p.m.
```
We actually can, thanks.
```

Commit:

-	0dedbc362cfc6021a74d87ae3b1137347b7f4d85
+	54200e1f6950c0b65d5ae2534d5e55da62a5e727

Diff:

Revision 9 (+551 -291)

Show changes

	usr/src/uts/i86pc/boot/boot_console.c
	usr/src/uts/i86pc/boot/boot_console_impl.h
	usr/src/uts/i86pc/boot/boot_fb.c
	usr/src/uts/i86pc/boot/boot_vga.c

Ship it!

```
Ship It!
```

Change Summary:

The early screen clear needs to be implemented as whole screen clean, not just terminal area. This is needed because the font sizes might be quite different and we do not want to leave any noise on the screen.

Commit:

-	54200e1f6950c0b65d5ae2534d5e55da62a5e727
+	f532b6c0f314f74025072854bd9bc1d4d9ea8e75

Diff:

Revision 10 (+556 -289)

Show changes

	usr/src/uts/i86pc/boot/boot_console.c
	usr/src/uts/i86pc/boot/boot_console_impl.h
	usr/src/uts/i86pc/boot/boot_fb.c
	usr/src/uts/i86pc/boot/boot_vga.c

Change Summary:

use VGA_TEXT_COLS and VGA_TEXT_ROWS

Commit:

-	f532b6c0f314f74025072854bd9bc1d4d9ea8e75
+	c09d01f52f9bf3c481c3bd8fbfee86c93eab9399

Diff:

Revision 11 (+558 -291)

Show changes

	usr/src/uts/i86pc/boot/boot_console.c
	usr/src/uts/i86pc/boot/boot_console_impl.h
	usr/src/uts/i86pc/boot/boot_fb.c
	usr/src/uts/i86pc/boot/boot_vga.c

Ship it!

```
Ship It!
```

Ship it!

usr/src/uts/i86pc/boot/boot_fb.c (Diff revisions 9 - 11)

I assume this change was because we may need to change the way the screen foreground/background in addition to just a normal clearing of the line? That's why we fill the destination and no longer need to call boot_fb_eraseline_impl()?

tsoome Jan. 30, 2019, 8:56 p.m.

Well, the clear screen in case of FB console means we need to fill the area with BG color - this FB memory does only have color values. So the eraseline also did fill with bg color.

However, I did create eraseline to deal with terminal lines - that means, it does not touch the "frame" area. But if we had boot loader with different kind of font, we also have different frames on screen and using eraseline means we will have garbage left over.

rm Jan. 30, 2019, 9:01 p.m.

OK, that makes sense. Thanks for clarifying.

You have a pending review.

Review Board 3.0.18

10235 uts: boot needs simple tem to support mdb

Change Summary:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Status: Closed (submitted)