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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.09.2024 19:47, Andrew Cooper wrote:
> On 10/09/2024 3:40 pm, Jan Beulich wrote:
>> @@ -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.

I'd like to keep a little more, if you don't mind:

        /*
         * Only accept single writes, as that's the only thing we accelerate
         * using buffered ioreq handling. */
         */

Not the least because writing this I noticed another flaw (or even two,
depending on how one looks at it): ioreq_send_buffered() further refuses
data_is_ptr requests. (Checking ->data_is_ptr is actually more important
than ->count, as ->count will be unequal to 1 only if ->data_is_ptr is
set, whereas ->count can also be 1 in that case.) Not the least because
that "refusing" is actually returning "success" (0 == X86EMUL_OKAY ==
IOREQ_STATUS_HANDLED) on x86, and IO_ABORT on Arm. I.e. such requests
would simply be lost on x86, and whether IO_ABORT is okay(ish) on Arm I
simply don't know (I'll see if digging through history reveals intentions).

And then - how can a buffered ioreq be a read, when there's nothing being
returned? (I.e. I consider this an omission in what ioreq_send_buffered()
refuses to process.)

Jan