Re: [PATCH v2 2/2] x86/hvm/emulate: make sure rep I/O emulation does not cross GFN boundaries

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 15/08/18 13:41, Jan Beulich wrote:
>>>> On 10.08.18 at 16:48, <paul.durrant@citrix.com> wrote:
>> When emulating a rep I/O operation it is possible that the ioreq will
>> describe a single operation that spans multiple GFNs. This is fine as long
>> as all those GFNs fall within an MMIO region covered by a single device
>> model, but unfortunately the higher levels of the emulation code do not
>> guarantee that. This is something that should almost certainly be fixed,
>> but in the meantime this patch makes sure that MMIO is truncated at GFN
>> boundaries and hence the appropriate device model is re-evaluated for each
>> target GFN.
>>
>> NOTE: This patch does not deal with the case of a single MMIO operation
>>       spanning a GFN boundary. That is more complex to deal with and is
>>       deferred to a subsequent patch.
>>
>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with a type change request:
>
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -184,6 +184,25 @@ static int hvmemul_do_io(
>>          hvmtrace_io_assist(&p);
>>      }
>>  
>> +    /*
>> +     * Make sure that we truncate rep MMIO at any GFN boundary. This is
>> +     * necessary to ensure that the correct device model is targetted
>> +     * or that we correctly handle a rep op spanning MMIO and RAM.
>> +     */
>> +    if ( unlikely(p.count > 1) && p.type == IOREQ_TYPE_COPY )
>> +    {
>> +        unsigned long off = p.addr & ~PAGE_MASK;
> If this was "unsigned int", all calculations below could be slightly
> cheaper 32-bit ones and ...
>
>> +        if ( PAGE_SIZE - off < p.size ) /* single rep spans GFN */
>> +            p.count = 1;
>> +        else
>> +            p.count = min_t(unsigned long,
> ... this could be just min(), as long as ...
>
>> +                            p.count,
>> +                            ((p.df ? (off + p.size) : (PAGE_SIZE - off)) /
> ... the 3rd arg of the ?: gets cast to unsigned int. If you agree, I'd
> be fine doing the adjustments while committing.

Paul is OoO for a while now.  In lieu, I'd say "yes please" to these
suggestions.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel