Re: [Minios-devel] [UNIKRAFT PATCH 2/2] plat/kvm: Update VGA console cursor location

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

On 29.07.2018 12:44, Florian Schmidt wrote:
> On 07/29/2018 12:39 PM, Florian Schmidt wrote:
> > Update the location of the cursor as data is written to the console.
> > Note that this does not set the cursor location registers, hence all we
> And I just realized this isn't very clear. This of course sets the 
> "Cursor Location {High,Low}" registers, but it doesn't set the "Cursor 
> {Start,End}" registers. I won't send a v2 for just that. Waiting for 
> other potential comments instead.
Hum, I am still not getting it from the description. ;-) How is printing 
a block different from a blinking underline?
> > get is the blinking underline character, not the full-size block
> > character.
> >
> > Signed-off-by: Florian Schmidt <florian.schmidt@xxxxxxxxx>
> > ---
> >   plat/kvm/x86/vga_console.c | 33 +++++++++++++++++++++++++++++++++
> >   1 file changed, 33 insertions(+)
> >
> > diff --git a/plat/kvm/x86/vga_console.c b/plat/kvm/x86/vga_console.c
> > index a1b5cf7..efcddde 100644
> > --- a/plat/kvm/x86/vga_console.c
> > +++ b/plat/kvm/x86/vga_console.c
> > @@ -29,6 +29,8 @@
> >   #include <sys/types.h>
> >   #include <stdint.h>
> >   #include <string.h>
> > +#include <x86/cpu.h>
> > +#include <x86/irq.h>
> >   #include <kvm-x86/vga_console.h>
Do you know if we better enable the cursor? I am not sure if we can 
assume a common state in which the boot loader left us.
https://wiki.osdev.org/Text_Mode_Cursor#Enabling_the_Cursor_2

> >   /* Hardware text mode color constants. */
> > @@ -109,6 +111,36 @@ static void vga_scroll(void)
> >               = vga_entry(' ', terminal_color);
> >   }
> > +static void vga_update_cursor(void)
> > +{
> > +    unsigned long irq_flags;
> > +    uint8_t old;
> > +    uint8_t ioas;  // VGA Input/Output select
> > +    uint16_t areg; // VGA address register
> > +    uint16_t dreg; // VGA data register
> > +
> > +    local_irq_save(irq_flags);
I wonder if it makes sense to protect the whole "_libkvmplat_vga_putc()" 
function where this function is just called from. This would truly 
enable printing from multiple threads since it protects the global state 
in this VGA driver.
> > +    /* Location of the address and data registers is variable and 
> > denoted
> > +     * by the least significant bit in the Input/Output register.
> > +     */
> > +    ioas = inb(0x3cc) & 0x1;
> > +    if (ioas) {
> > +        areg = 0x3d4;
> > +        dreg = 0x3d5;
> > +    } else {
> > +        areg = 0x3b4;
> > +        dreg = 0x3b5;
> > +    }
Is this changing during runtime or could be figure out the location 
during initialization and use it?
> > +
> > +    old = inb(areg);
> > +    outb(areg, 0x0e); // Cursor Location High
> > +    outb(dreg, ((terminal_row * VGA_WIDTH) + terminal_column) >> 8);
> > +    outb(areg, 0x0f); // Cursor Location Low
> > +    outb(dreg, ((terminal_row * VGA_WIDTH) + terminal_column) & 0xff);
> > +    outb(areg, old);
> > +    local_irq_restore(irq_flags);
> > +}
> > +
> >   static void vga_newline(void)
> >   {
> >       if (terminal_row == VGA_HEIGHT - 1)
> > @@ -160,4 +192,5 @@ void _libkvmplat_vga_putc(char c)
> >           }
> >           break;
> >       }
> > +    vga_update_cursor();
> >   }
Thanks,

Simon

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel