| Message ID | 20171017214138.27663-1-cardoe@cardoe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 17.10.17 at 23:41, <cardoe@cardoe.com> wrote: > From: David Esler <drumandstrum@gmail.com> > > In 9180f5365524 a change was made to the send_chr function to take in > C-strings and print out a character at a time until a NULL was > encountered. However there is no code to increment the current character > position resulting in an endless loop of the first character. This adds > a simple increment. This description is not accurate (it should have changed together with the change to how you fix the issue) - with VGA the increment does happen. Hence "display" in the title is perhaps also at least misleading. I would be fine to adjust both while committing (and then adding my R-b), but feel free to propose an alternative. > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -173,10 +173,11 @@ not_multiboot: > .Lget_vtb: > mov sym_esi(vga_text_buffer),%edi > .Lsend_chr: > - mov (%esi),%bl > - test %bl,%bl # Terminate on '\0' sentinel > + lodsb > + test %al,%al # Terminate on '\0' sentinel > je .Lhalt > mov $0x3f8+5,%dx # UART Line Status Register > + mov %al,%bl > 2: in %dx,%al > test $0x20,%al # Test THR Empty flag > je 2b Daniel: What tells you that there is a UART at 0x3f8? Aren't we risking to enter an infinite loop here if there isn't? At the very least it would seem better to me if the VGA output was done first - when you have a screen, you'd at least know something was attempted to be output by seeing the first character. And when you have neither screen nor UART (at the right port) you're hosed anyway. Jan
On Tue, Oct 17, 2017 at 04:41:37PM -0500, Doug Goldstein wrote: > From: David Esler <drumandstrum@gmail.com> > > In 9180f5365524 a change was made to the send_chr function to take in > C-strings and print out a character at a time until a NULL was > encountered. However there is no code to increment the current character > position resulting in an endless loop of the first character. This adds > a simple increment. > > Reviewed-by: Doug Goldstein <cardoe@cardoe.com> I was told that "Reviewed-by: ..." should be after SOB. > Signed-off-by: David Esler <drumandstrum@gmail.com> In general Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> Though please take into account Jan's request WRT to commit message. Or I am OK with Jan's changes before committing. Daniel
On 10/18/17 4:50 AM, Jan Beulich wrote: >>>> On 17.10.17 at 23:41, <cardoe@cardoe.com> wrote: >> From: David Esler <drumandstrum@gmail.com> >> >> In 9180f5365524 a change was made to the send_chr function to take in >> C-strings and print out a character at a time until a NULL was >> encountered. However there is no code to increment the current character >> position resulting in an endless loop of the first character. This adds >> a simple increment. > > This description is not accurate (it should have changed together with > the change to how you fix the issue) - with VGA the increment does > happen. Hence "display" in the title is perhaps also at least misleading. > I would be fine to adjust both while committing (and then adding my > R-b), but feel free to propose an alternative. David and I are both ok with you changing the wording as necessary. I apologize for not updating the commit message.
On 10/18/17 4:50 AM, Jan Beulich wrote: >>>> On 17.10.17 at 23:41, <cardoe@cardoe.com> wrote: >> From: David Esler <drumandstrum@gmail.com> >> >> In 9180f5365524 a change was made to the send_chr function to take in >> C-strings and print out a character at a time until a NULL was >> encountered. However there is no code to increment the current character >> position resulting in an endless loop of the first character. This adds >> a simple increment. > > This description is not accurate (it should have changed together with > the change to how you fix the issue) - with VGA the increment does > happen. Hence "display" in the title is perhaps also at least misleading. > I would be fine to adjust both while committing (and then adding my > R-b), but feel free to propose an alternative. Jan, Can you queue this for 4.9 as well? That's where we ran into the issue in the first place.
>>> On 26.10.17 at 23:05, <cardoe@cardoe.com> wrote: > On 10/18/17 4:50 AM, Jan Beulich wrote: >>>>> On 17.10.17 at 23:41, <cardoe@cardoe.com> wrote: >>> From: David Esler <drumandstrum@gmail.com> >>> >>> In 9180f5365524 a change was made to the send_chr function to take in >>> C-strings and print out a character at a time until a NULL was >>> encountered. However there is no code to increment the current character >>> position resulting in an endless loop of the first character. This adds >>> a simple increment. >> >> This description is not accurate (it should have changed together with >> the change to how you fix the issue) - with VGA the increment does >> happen. Hence "display" in the title is perhaps also at least misleading. >> I would be fine to adjust both while committing (and then adding my >> R-b), but feel free to propose an alternative. > > Jan, > > Can you queue this for 4.9 as well? That's where we ran into the issue > in the first place. That how I did understand it, so I've queued this already, but for it to become eligible to applying to 4.9 it first needs to pass the push gate on master. Jan
On 10/27/17 1:37 AM, Jan Beulich wrote: >>>> On 26.10.17 at 23:05, <cardoe@cardoe.com> wrote: >> On 10/18/17 4:50 AM, Jan Beulich wrote: >>>>>> On 17.10.17 at 23:41, <cardoe@cardoe.com> wrote: >>>> From: David Esler <drumandstrum@gmail.com> >>>> >>>> In 9180f5365524 a change was made to the send_chr function to take in >>>> C-strings and print out a character at a time until a NULL was >>>> encountered. However there is no code to increment the current character >>>> position resulting in an endless loop of the first character. This adds >>>> a simple increment. >>> >>> This description is not accurate (it should have changed together with >>> the change to how you fix the issue) - with VGA the increment does >>> happen. Hence "display" in the title is perhaps also at least misleading. >>> I would be fine to adjust both while committing (and then adding my >>> R-b), but feel free to propose an alternative. >> >> Jan, >> >> Can you queue this for 4.9 as well? That's where we ran into the issue >> in the first place. > > That how I did understand it, so I've queued this already, but for > it to become eligible to applying to 4.9 it first needs to pass the > push gate on master. > > Jan > Of course. I didn't mean to imply this should jump any existing process. I had just realized that I didn't explicitly mention versions in my original email and I just wanted to make sure I was explicit instead of assuming. Thanks again.
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index fd6fc337fe..9cc35da558 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -173,10 +173,11 @@ not_multiboot: .Lget_vtb: mov sym_esi(vga_text_buffer),%edi .Lsend_chr: - mov (%esi),%bl - test %bl,%bl # Terminate on '\0' sentinel + lodsb + test %al,%al # Terminate on '\0' sentinel je .Lhalt mov $0x3f8+5,%dx # UART Line Status Register + mov %al,%bl 2: in %dx,%al test $0x20,%al # Test THR Empty flag je 2b @@ -185,7 +186,7 @@ not_multiboot: out %al,%dx # Send a character over the serial line test %edi,%edi # Is the VGA text buffer available? jz .Lsend_chr - movsb # Write a character to the VGA text buffer + stosb # Write a character to the VGA text buffer mov $7,%al stosb # Write an attribute to the VGA text buffer jmp .Lsend_chr