Re: [PATCH 3/2] x86/pv: Rename pv.iobmp_limit to iobmp_nr and clarify behaviour

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/10/2024 2:35 pm, Jan Beulich wrote:
> On 01.10.2024 14:24, Andrew Cooper wrote:
>> Ever since it's introduction in commit 013351bd7ab3 ("Define new 
>> event-channel
>> and physdev hypercalls"), the public interface was named nr_ports while the
>> internal field was called iobmp_limit.
>>
>> Rename the intenral field to iobmp_nr to match the public interface, and
>> clarify that, when nonzero, Xen will read 2 bytes.
>>
>> There isn't a perfect parallel with a real TSS, but iobmp_nr being 0 is the
>> paravirt "no IOPB" case, and it is important that no read occurs in this 
>> case.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

> with one cosmetic request:
>
>> --- a/xen/arch/x86/pv/emul-priv-op.c
>> +++ b/xen/arch/x86/pv/emul-priv-op.c
>> @@ -167,7 +167,12 @@ static int guest_io_okay(unsigned int port, unsigned 
>> int bytes,
>>      if ( iopl_ok(v, regs) )
>>          return X86EMUL_OKAY;
>>  
>> -    if ( (port + bytes) <= v->arch.pv.iobmp_limit )
>> +    /*
>> +     * When @iobmp_nr is non-zero, Xen, like real CPUs and the TSS IOPB,
>> +     * always reads 2 bytes from @iobmp, which might be one byte beyond
>> +     * @nr_ports.
>> +     */
>> +    if ( (port + bytes) <= v->arch.pv.iobmp_nr )
> If you use @nr_ports in the comment here, then I think some connection wants
> establishing as to where this comes from. Otherwise use @iobmp_nr a 2nd time.

Oops, that was a copy&paste error from the header.  I'll switch it back
to @iobmp_nr.

~Andrew