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

[Florian Schmidt <florian@xxxxxxxxx>]

Oops, I see i missed the other comments about the actual code, so let me 
respond to them here.
On 07/30/2018 02:23 PM, Simon Kuenzer wrote:
> 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
My thought was that this code is specific to KVM at the moment, and I 
assume that QEMU will always drop us into a defined state. But I can add 
the code to set the cursor. I'll do it for a v2.
> > >   /* 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.
Hmm. I'm not a big fan of disabling interrupts for large amounts of 
code, including several functions. Long strings can take time to print 
out, and I'm not sure I feel comfortable disabling interrupts for that 
long If at all, I would suggest doing that with other kinds of locking 
mechanisms. I'm also wondering, though, whether it shouldn't be the job 
of whatever's calling coutk. The typical behavior on most other systems 
is that, if you print from several threads without synchronization, your 
output will be interleaved, right?

> > > +    /* 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?
I honestly don't know. I wouldn't expect this to change after it is set 
once, but I don't know for sure. Supposedly, 0x3b is mostly there for 
compatibility with older monochrome adapters, so it probably should be 
0x3d pretty much always. If we go by the "this is a driver for QEMU VGA 
only", we can probably scrap it altogether. Otherwise, it's *probably* 
fine to just check it once in the beginning.

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