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

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

On 30.07.2018 15:15, Florian Schmidt wrote:
> On 07/30/2018 02:23 PM, Simon Kuenzer wrote:
> > 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?
> OK, you're right, I should explain this more.
> There are a bunch of registers involved in setting the cursor. There is 
> "Cursor Location {High,Low}", which is effectively a 16-bit register 
> (split into 2, because registers are only addressable byte-wise). That 
> register encodes the location of the cursor by just counting the text 
> positions. The first one is 0, the 3d character on the 6th line is 
> (5*VGA_WIDTH + 3), etc. Of course, height and width are mode dependent...
> In addition, there are also two registers called "Cursor Start" and 
> "Cursor End". Those registers encode, in scanlines (i.e., "pixels", 
> kinda), the beginning and the end of the blinking cursor box. By setting 
> Start to 0 and End to MAXLINES (which, again, is mode-dependent and can 
> be read from yet another register; for 80x25, it is 0x0f), you get the 
> full-size "box" cursor. In QEMU, the default for these values is 0x0d 
> and 0x0e, which gives you the "blinking underline" instead of the 
> "blinking box". Yes, this also means you can have a cursor that blink at 
> the top of a character, or in the middle, if anyone ever wanted to...
>
> Not sure whether all of that should go into the commit message though. ;-)
Thanks for the explanation. What about this as message?:

Update the location of the cursor as data is written to the console.
Since we only set the location of a blinking underline character, we use
the Cursor Location {High,Low} registers.

> > > > 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