| Message ID | b6d656bd249e85ef192a0bbddae1eb8492e51583.1629713932.git.bertrand.marquis@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Sanitize cpuinfo | expand |
Hi Bertrand, On 23/08/2021 11:32, Bertrand Marquis wrote: > Replace the code in p2m trying to find a sane value for the VMID size > supported and the PAR to use. We are now using the boot cpuinfo as the > values there are sanitized during boot and the value for those > parameters is now the safest possible value on the system. > > On arm32, the system will panic if there are different types of core so > those checks were not needed anyway. So the assumption is that if you have the same MIDR, then you must have the same features. I understand this is what Xen assumes today but I never viewed that check as the truth. It is more to limit the damage on most platform. So can you confirm whether this is something that Arm guarantees? That said, I am not against removing the code. But I would like the comment to be amended if this is not a correct assumption. Cheers,
On 23/08/2021 12:47, Julien Grall wrote: > Hi Bertrand, > > On 23/08/2021 11:32, Bertrand Marquis wrote: >> Replace the code in p2m trying to find a sane value for the VMID size >> supported and the PAR to use. We are now using the boot cpuinfo as the >> values there are sanitized during boot and the value for those >> parameters is now the safest possible value on the system. >> >> On arm32, the system will panic if there are different types of core so >> those checks were not needed anyway. > > So the assumption is that if you have the same MIDR, then you must have > the same features. I understand this is what Xen assumes today but I > never viewed that check as the truth. It is more to limit the damage on > most platform. > > So can you confirm whether this is something that Arm guarantees? > > That said, I am not against removing the code. But I would like the > comment to be amended if this is not a correct assumption. s/comment/commit message/. > Cheers, >
Hi Julien, > On 23 Aug 2021, at 12:47, Julien Grall <julien@xen.org> wrote: > > Hi Bertrand, > > On 23/08/2021 11:32, Bertrand Marquis wrote: >> Replace the code in p2m trying to find a sane value for the VMID size >> supported and the PAR to use. We are now using the boot cpuinfo as the >> values there are sanitized during boot and the value for those >> parameters is now the safest possible value on the system. >> On arm32, the system will panic if there are different types of core so >> those checks were not needed anyway. > > So the assumption is that if you have the same MIDR, then you must have the same features. I understand this is what Xen assumes today but I never viewed that check as the truth. It is more to limit the damage on most platform. This was the assumption before, I did not change anything but just explained in the commit message why this was not possible to come to this code. > > So can you confirm whether this is something that Arm guarantees? For a specific MIDR from Arm (ie a Cortex) the PAR is fixed and VMID size to. But for an other Arm architecture processor I cannot say. > > That said, I am not against removing the code. But I would like the comment to be amended if this is not a correct assumption. Would the following be acceptable: On arm32, Xen is not booting on systems having different MIDR amongst cores and it is assumed that cores with the same MIDR will have the same PAR and VMID size. Cheers Bertrand > > Cheers, > > -- > Julien Grall
On 23/08/2021 13:39, Bertrand Marquis wrote: > Hi Julien, Hi Bertrand, > >> On 23 Aug 2021, at 12:47, Julien Grall <julien@xen.org> wrote: >> >> Hi Bertrand, >> >> On 23/08/2021 11:32, Bertrand Marquis wrote: >>> Replace the code in p2m trying to find a sane value for the VMID size >>> supported and the PAR to use. We are now using the boot cpuinfo as the >>> values there are sanitized during boot and the value for those >>> parameters is now the safest possible value on the system. >>> On arm32, the system will panic if there are different types of core so >>> those checks were not needed anyway. >> >> So the assumption is that if you have the same MIDR, then you must have the same features. I understand this is what Xen assumes today but I never viewed that check as the truth. It is more to limit the damage on most platform. > > This was the assumption before, I did not change anything but just explained in the commit message why this was not possible to come to this code. Yes. This is not a new, however I thought I would mention it because I want to avoid continuing to use wrong assumptions. However, this code is arm64 only as it is #ifdef right? (Sorry I should have looked at the code the first time) So ... > >> >> So can you confirm whether this is something that Arm guarantees? > > For a specific MIDR from Arm (ie a Cortex) the PAR is fixed and VMID size to. > But for an other Arm architecture processor I cannot say. > >> >> That said, I am not against removing the code. But I would like the comment to be amended if this is not a correct assumption. > > Would the following be acceptable: > On arm32, Xen is not booting on systems having different MIDR amongst cores and it is assumed that cores with the same MIDR will have the same PAR and VMID size. ... I would just drop any mention of arm32 here. Cheers,
Hi, > On 23 Aug 2021, at 14:22, Julien Grall <julien@xen.org> wrote: > > > > On 23/08/2021 13:39, Bertrand Marquis wrote: >> Hi Julien, > > Hi Bertrand, > >>> On 23 Aug 2021, at 12:47, Julien Grall <julien@xen.org> wrote: >>> >>> Hi Bertrand, >>> >>> On 23/08/2021 11:32, Bertrand Marquis wrote: >>>> Replace the code in p2m trying to find a sane value for the VMID size >>>> supported and the PAR to use. We are now using the boot cpuinfo as the >>>> values there are sanitized during boot and the value for those >>>> parameters is now the safest possible value on the system. >>>> On arm32, the system will panic if there are different types of core so >>>> those checks were not needed anyway. >>> >>> So the assumption is that if you have the same MIDR, then you must have the same features. I understand this is what Xen assumes today but I never viewed that check as the truth. It is more to limit the damage on most platform. >> This was the assumption before, I did not change anything but just explained in the commit message why this was not possible to come to this code. > > Yes. This is not a new, however I thought I would mention it because I want to avoid continuing to use wrong assumptions. > > However, this code is arm64 only as it is #ifdef right? (Sorry I should have looked at the code the first time) So ... > >>> >>> So can you confirm whether this is something that Arm guarantees? >> For a specific MIDR from Arm (ie a Cortex) the PAR is fixed and VMID size to. >> But for an other Arm architecture processor I cannot say. >>> >>> That said, I am not against removing the code. But I would like the comment to be amended if this is not a correct assumption. >> Would the following be acceptable: >> On arm32, Xen is not booting on systems having different MIDR amongst cores and it is assumed that cores with the same MIDR will have the same PAR and VMID size. > ... I would just drop any mention of arm32 here. Ok Cheers Bertrand > > Cheers, > > -- > Julien Grall
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index eff9a105e7..41b6430c30 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -2045,31 +2045,21 @@ void __init setup_virt_paging(void) [7] = { 0 } /* Invalid */ }; - unsigned int i, cpu; + unsigned int i; unsigned int pa_range = 0x10; /* Larger than any possible value */ - bool vmid_8_bit = false; - - for_each_online_cpu ( cpu ) - { - const struct cpuinfo_arm *info = &cpu_data[cpu]; - /* - * Restrict "p2m_ipa_bits" if needed. As P2M table is always configured - * with IPA bits == PA bits, compare against "pabits". - */ - if ( pa_range_info[info->mm64.pa_range].pabits < p2m_ipa_bits ) - p2m_ipa_bits = pa_range_info[info->mm64.pa_range].pabits; - - /* Set a flag if the current cpu does not support 16 bit VMIDs. */ - if ( info->mm64.vmid_bits != MM64_VMID_16_BITS_SUPPORT ) - vmid_8_bit = true; - } + /* + * Restrict "p2m_ipa_bits" if needed. As P2M table is always configured + * with IPA bits == PA bits, compare against "pabits". + */ + if ( pa_range_info[system_cpuinfo.mm64.pa_range].pabits < p2m_ipa_bits ) + p2m_ipa_bits = pa_range_info[system_cpuinfo.mm64.pa_range].pabits; /* - * If the flag is not set then it means all CPUs support 16-bit - * VMIDs. + * cpu info sanitization made sure we support 16bits VMID only if all + * cores are supporting it. */ - if ( !vmid_8_bit ) + if ( system_cpuinfo.mm64.vmid_bits == MM64_VMID_16_BITS_SUPPORT ) max_vmid = MAX_VMID_16_BIT; /* Choose suitable "pa_range" according to the resulted "p2m_ipa_bits". */
Replace the code in p2m trying to find a sane value for the VMID size supported and the PAR to use. We are now using the boot cpuinfo as the values there are sanitized during boot and the value for those parameters is now the safest possible value on the system. On arm32, the system will panic if there are different types of core so those checks were not needed anyway. Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> --- Changes in v2: - use system_cpuinfo --- xen/arch/arm/p2m.c | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-)