Re: [PATCH v5 08/11] xen/arm: Enable the existing x86 virtual PCI support for ARM.

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 12 Oct 2021, at 17:20, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 12/10/2021 17:12, Bertrand Marquis wrote:
>> Hi Julien,
>>> On 12 Oct 2021, at 16:04, Julien Grall <julien@xxxxxxx> wrote:
>>> 
>>> On 11/10/2021 13:41, Bertrand Marquis wrote:
>>>> Hi Jan,
>>> 
>>> Hi Bertrand,
>>> 
>>>> As Rahul is on leave, I will answer you and make the changes needed.
>>>>> On 7 Oct 2021, at 14:43, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>> Independent of this - is bare metal Arm enforcing this same
>>>>> alignment restriction (unconditionally)? Iirc on x86 we felt we'd
>>>>> better synthesize misaligned accesses.
>>>> Unaligned IO access could be synthesise also on arm to but I would
>>>> rather not make such a change now without testing it (and there is
>>>> also a question of it making sense).
>>> 
>>> Yes it makes sense. I actually have an item in my TODO list to forbid 
>>> unaligned access because they should not happen on any device we currently 
>>> emulate.
>>> 
>>> Although, I am not aware of any issue other than the guest would shoot 
>>> itself in the foot if this happens.
>>> 
>>>> So if it is ok with you I will keep that check and discuss it with Rahul
>>>> when he is back. I will add a comment in the code to make that clear.
>>> 
>>> I am OK with it.
>>> 
>>> [...]
>>> 
>>>>> Throughout this series I haven't been able to spot where the HAS_VPCI
>>>>> Kconfig symbol would get selected. Hence I cannot tell whether all of
>>>>> this is Arm64-specific. Otherwise I wonder whether size 8 actually
>>>>> can occur on Arm32.
>>>> Dabt.size could be 3 even on ARM32 but we should not allow 64bit
>>>> access on mmio regions on arm32.
>>> 
>>> Hmmm... Looking at the Armv7 and Armv8 spec, ldrd/strd (64-bit read) would 
>>> not present a valid ISV. So I think it is not be possible to have dabt.size 
>>> = 3 for 32-bit domain. But I agree we probably want to harden the code.
>>> 
>>>> So I will surround this code with ifdef CONFIG_ARM_64 and add a test
>>>> for len > 4 to prevent this case on 32bit.
>>>> To be completely right we should disable this also for 32bit guests but
>>>> this change would be a bit more invasive.
>>> 
>>> I think the following should be sufficient:
>>> 
>>> if ( is_domain_32bit_domain() && len > 4 )
>>>  return ...;
>> With the last request from Roger to use the function implemented in 
>> arch/x86/hw/io.c, the function will move to vpci.h so using is_32bit_domain 
>> will not be possible without ifdefery CONFIG_ARM.
>> Also I have no access to the domain there.
>> So the best I can do for now would be something like:
>> #ifdef CONFIG_ARM_32
>> If (len == 8)
>>     return false
>> #endif
>> A 32bit guest on 64bit xen would not be checked.
>> Would that be ok for now ?
> 
> I think the #ifdef is a bit pointless. My preference would be to not add the 
> #ifdef but instead add...
> 
>> I could add a comment in the code to warn about the limitation.
> 
> .. a comment for now as this is only an hardening problem.

Agree I will do that.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall