Hi Julien, > On 12 Oct 2021, at 17:20, Julien Grall <jul...@xen.org> wrote: > > > > On 12/10/2021 17:12, Bertrand Marquis wrote: >> Hi Julien, >>> On 12 Oct 2021, at 16:04, Julien Grall <jul...@xen.org> 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 <jbeul...@suse.com> 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
