| Message ID | 20171012205007.13001-1-cardoe@cardoe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/10/2017 21:50, 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> > Signed-off-by: David Esler <drumandstrum@gmail.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> CC'ing Julien as release manager. This should be included in 4.10 IMO, and is a backport candidate. > --- > xen/arch/x86/boot/head.S | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S > index fd6fc337fe..f48bbbd2e5 100644 > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -174,6 +174,7 @@ not_multiboot: > mov sym_esi(vga_text_buffer),%edi > .Lsend_chr: > mov (%esi),%bl > + inc %esi > test %bl,%bl # Terminate on '\0' sentinel > je .Lhalt > mov $0x3f8+5,%dx # UART Line Status Register
On Thu, Oct 12, 2017 at 03:50:06PM -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> > Signed-off-by: David Esler <drumandstrum@gmail.com> > --- > xen/arch/x86/boot/head.S | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S > index fd6fc337fe..f48bbbd2e5 100644 > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -174,6 +174,7 @@ not_multiboot: > mov sym_esi(vga_text_buffer),%edi > .Lsend_chr: > mov (%esi),%bl > + inc %esi > test %bl,%bl # Terminate on '\0' sentinel > je .Lhalt > mov $0x3f8+5,%dx # UART Line Status Register I have a feeling that you have tested this on machine without VGA text buffer available. Then your fix works. However, if VGA text buffer is available then %esi is increased twice. First time by inc here and once again by movsb below. So, I think that the issue have to be fixed in a bit different way. Daniel
> On Oct 12, 2017, at 4:27 PM, Daniel Kiper <daniel.kiper@oracle.com> wrote: > >> On Thu, Oct 12, 2017 at 03:50:06PM -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> >> Signed-off-by: David Esler <drumandstrum@gmail.com> >> --- >> xen/arch/x86/boot/head.S | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S >> index fd6fc337fe..f48bbbd2e5 100644 >> --- a/xen/arch/x86/boot/head.S >> +++ b/xen/arch/x86/boot/head.S >> @@ -174,6 +174,7 @@ not_multiboot: >> mov sym_esi(vga_text_buffer),%edi >> .Lsend_chr: >> mov (%esi),%bl >> + inc %esi >> test %bl,%bl # Terminate on '\0' sentinel >> je .Lhalt >> mov $0x3f8+5,%dx # UART Line Status Register > > I have a feeling that you have tested this on machine without > VGA text buffer available. Then your fix works. However, if VGA > text buffer is available then %esi is increased twice. First time > by inc here and once again by movsb below. So, I think that the > issue have to be fixed in a bit different way. > > Daniel Correct. It was an EFI machine with serial only where I saw it in action. David has put together a new change and I’ll get it submitted tomorrow. — Doug
Hi Andrew, On 12/10/17 21:55, Andrew Cooper wrote: > On 12/10/2017 21:50, 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> >> Signed-off-by: David Esler <drumandstrum@gmail.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > CC'ing Julien as release manager. This should be included in 4.10 IMO, > and is a backport candidate. I agree. Release-acked-by: Julien Grall <julien.grall@linaro.org> Cheers, > >> --- >> xen/arch/x86/boot/head.S | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S >> index fd6fc337fe..f48bbbd2e5 100644 >> --- a/xen/arch/x86/boot/head.S >> +++ b/xen/arch/x86/boot/head.S >> @@ -174,6 +174,7 @@ not_multiboot: >> mov sym_esi(vga_text_buffer),%edi >> .Lsend_chr: >> mov (%esi),%bl >> + inc %esi >> test %bl,%bl # Terminate on '\0' sentinel >> je .Lhalt >> mov $0x3f8+5,%dx # UART Line Status Register > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel >
>>> On 13.10.17 at 11:51, <julien.grall@linaro.org> wrote: > On 12/10/17 21:55, Andrew Cooper wrote: >> On 12/10/2017 21:50, 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> >>> Signed-off-by: David Esler <drumandstrum@gmail.com> >> >> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> >> >> CC'ing Julien as release manager. This should be included in 4.10 IMO, >> and is a backport candidate. > > I agree. > > Release-acked-by: Julien Grall <julien.grall@linaro.org> So I take this is a pre-ack to the eventual patch with the bug fixed that Daniel has pointed out? Jan
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index fd6fc337fe..f48bbbd2e5 100644 --- a/xen/arch/x86/boot/head.S +++ b/xen/arch/x86/boot/head.S @@ -174,6 +174,7 @@ not_multiboot: mov sym_esi(vga_text_buffer),%edi .Lsend_chr: mov (%esi),%bl + inc %esi test %bl,%bl # Terminate on '\0' sentinel je .Lhalt mov $0x3f8+5,%dx # UART Line Status Register