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
