Re: [PATCH 2/7] x86/HVM: drop stdvga's "stdvga" struct member

[Andrew Cooper]

On 10/09/2024 3:40 pm, Jan Beulich wrote:
> Two of its consumers are dead (in compile-time constant conditionals)
> and the only remaining ones are merely controlling (debugging) log
> messages.

Minor, but I'd phrase this as "merely controlling debug logging."

>  Hence the field is now pointless to set, which in particular
> allows to get rid of the questionable conditional from which the field's
> value was established (afaict 551ceee97513 ["x86, hvm: stdvga cache
> always on"] had dropped too much of the earlier extra check that was
> there, and quite likely further checks were missing).
>
> Signed-off-by: Jan Beulich <jbeul...@suse.com>
>
> --- a/xen/arch/x86/hvm/stdvga.c
> +++ b/xen/arch/x86/hvm/stdvga.c
> @@ -103,7 +103,7 @@ static void vram_put(struct hvm_hw_stdvg
>  static int stdvga_outb(uint64_t addr, uint8_t val)
>  {
>      struct hvm_hw_stdvga *s = &current->domain->arch.hvm.stdvga;
> -    int rc = 1, prev_stdvga = s->stdvga;
> +    int rc = 1;

This looks to also drop a MISRA essential-type complaint.

>  
>      switch ( addr )
>      {
> @@ -132,19 +132,6 @@ static int stdvga_outb(uint64_t addr, ui
>          break;
>      }
>  
> -    /* When in standard vga mode, emulate here all writes to the vram buffer
> -     * so we can immediately satisfy reads without waiting for qemu. */
> -    s->stdvga = (s->sr[7] == 0x00);
> -
> -    if ( !prev_stdvga && s->stdvga )
> -    {
> -        gdprintk(XENLOG_INFO, "entering stdvga mode\n");
> -    }
> -    else if ( prev_stdvga && !s->stdvga )
> -    {
> -        gdprintk(XENLOG_INFO, "leaving stdvga mode\n");
> -    }
> -
>      return rc;
>  }
>  
> @@ -425,7 +412,6 @@ static int cf_check stdvga_mem_write(
>      const struct hvm_io_handler *handler, uint64_t addr, uint32_t size,
>      uint64_t data)
>  {
> -    struct hvm_hw_stdvga *s = &current->domain->arch.hvm.stdvga;
>      ioreq_t p = {
>          .type = IOREQ_TYPE_COPY,
>          .addr = addr,
> @@ -436,8 +422,7 @@ static int cf_check stdvga_mem_write(
>      };
>      struct ioreq_server *srv;
>  
> -    if ( true || !s->stdvga )
> -        goto done;
> +    goto done;
>  
>      /* Intercept mmio write */
>      switch ( size )
> @@ -498,19 +483,17 @@ static bool cf_check stdvga_mem_accept(
>  
>      spin_lock(&s->lock);
>  
> -    if ( p->dir == IOREQ_WRITE && p->count > 1 )
> +    if ( p->dir != IOREQ_WRITE || p->count > 1 )
>      {
>          /*
>           * We cannot return X86EMUL_UNHANDLEABLE on anything other then the
>           * first cycle of an I/O. So, since we cannot guarantee to always be
>           * able to send buffered writes, we have to reject any multi-cycle
> -         * I/O.
> +         * I/O. And of course we have to reject all reads, for not being
> +         * able to service them.
>           */
>          goto reject;
>      }
> -    else if ( p->dir == IOREQ_READ &&
> -              (true || !s->stdvga) )
> -        goto reject;

Before, we rejected on (WRITE && count) or READ, and I think we still do
afterwards given the boolean-ness of read/write.  However, the comment
is distinctly less relevant.

I think we now just end up with /* Only accept single writes. */ which
matches with with shuffling these into the bufioreq ring.

~Andrew