Re: [Xen-devel] [PATCH v5 02/16] x86/hvm: remove multiple open coded 'chunking' loops

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/07/15 16:55, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
>> Sent: 02 July 2015 16:38
>> To: Paul Durrant; xen-devel@xxxxxxxxxxxxxxxxxxxx
>> Cc: Keir (Xen.org); Jan Beulich
>> Subject: Re: [PATCH v5 02/16] x86/hvm: remove multiple open coded
>> 'chunking' loops
>>
>> On 30/06/15 14:05, Paul Durrant wrote:
>>> ...in hvmemul_read/write()
>>>
>>> Add hvmemul_phys_mmio_access() and hvmemul_linear_mmio_access()
>> functions
>>> to reduce code duplication.
>>>
>>> NOTE: This patch also introduces a change in 'chunking' around a page
>>>       boundary. Previously (for example) an 8 byte access at the last
>>>       byte of a page would get carried out as 8 single-byte accesses.
>>>       It will now be carried out as a single-byte access, followed by
>>>       a 4-byte access, a 2-byte access and then another single-byte
>>>       access.
>>>
>>> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
>>> Cc: Keir Fraser <keir@xxxxxxx>
>>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> ---
>>>  xen/arch/x86/hvm/emulate.c |  223 +++++++++++++++++++++++----------
>> -----------
>>>  1 file changed, 116 insertions(+), 107 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>>> index 8b60843..b67f5db 100644
>>> --- a/xen/arch/x86/hvm/emulate.c
>>> +++ b/xen/arch/x86/hvm/emulate.c
>>> @@ -539,6 +539,117 @@ static int hvmemul_virtual_to_linear(
>>>      return X86EMUL_EXCEPTION;
>>>  }
>>>
>>> +static int hvmemul_phys_mmio_access(
>>> +    paddr_t gpa, unsigned int size, uint8_t dir, uint8_t *buffer)
>>> +{
>>> +    unsigned long one_rep = 1;
>>> +    unsigned int chunk;
>>> +    int rc;
>>> +
>>> +    /* Accesses must fall within a page */
>> Full stop.
>>
>>> +    BUG_ON((gpa & (PAGE_SIZE - 1)) + size > PAGE_SIZE);
>> ~PAGE_MASK as opposed to (PAGE_SIZE - 1)
>>
>>> +
>>> +    /*
>>> +     * hvmemul_do_io() cannot handle non-power-of-2 accesses or
>>> +     * accesses larger than sizeof(long), so choose the highest power
>>> +     * of 2 not exceeding sizeof(long) as the 'chunk' size.
>>> +     */
>>> +    chunk = 1 << (fls(size) - 1);
>> Depending on size, chunk can become undefined (shifting by 31 or -1) or
>> zero (shifting by 32).
>>
>> How about
>>
>> if ( size > sizeof(long) )
>>     chunk = sizeof(long);
>> else
>>     chunk = 1U << (fls(size) - 1);
>>
> fls(size) - 1 can't be more than 31 (since size is an unsigned int). I can 
> assert size != 0. So would

Ah yes - very true (until someone goes and changes size to an unsigned long)

Size being 0 is probably the kind of thing which needs checking at the
very top of the emulation calltree and assumed thereafter.  Might also
want an upper sanity check as well.

>
> chunk = 1u << (fls(size) - 1);
>
> be ok? (I.e. I just missed the 'u' suffix before).

Yeah.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel