[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] xen/arm: don't read aarch32 regs when aarch32 isn't available


  • To: Julien Grall <julien@xxxxxxx>
  • From: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
  • Date: Tue, 12 Jan 2021 13:12:31 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=nLY4aT2BgS14cLhRBVGJGuPk2m/jHHNLkX62JkX8/Gw=; b=adpxShX4XOGKlESghkqkdmpY1Xdw87hRGG/y5MrYRO/14j+jSk/EiOJ5QvbH/slq+DpsrpHzkLs2kC8nbOG5tzCdCDMIqHAiUiJWpzGSxPHmTkAhk4xUTuR+9uKbUq/Kw1kQ+tuEUPMe7OVls/O51aUVd6YAOyDPyiLdj8jyFmF0VfeQOzRJr5O9w94hLHsWnDGTsGpgcDp9Q5/byM9/aGh+4pmcXfouJct/Fu+0Y2PwPvlzbCKRvy5ItXDYwnPawJCQxh2ADrjRpdRFPAcWcGUGYtslqU9ru6z4hoVV5Xc/7dNXmlOvCzecg96YGhHab4dinTjnL4UWkJjlYRGuvg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=jHdP5eYJk6SOEaBOVllcrFUQ4l6gixvF1EnNKM1Rj1nFFMMEsR4Alx2n/Z2x7BUmDA9N4d+lYthWTHPMREWVY/JcjMVHSzGtJcsrjoEaP+XJ44Meh/S0BFsFkOv+KaZVm8CuFAqjdIWozKqSRguyhtZbUAmDsJ7fRoS+HIAGGqtMe1PCY/3aEgNnT0XB0Rn1ZxIBbPyLOEfazGG73jRy419tH5xpPpZhlNELK0mh4hQnB4kJndyN3EtBXKDFm4xlNZ1TnpfI1HtDWEu59He32h6CCsvEHgdfs1y+gRo3Fsk+LkvaHawAzfoYET+la807L8xUJDgrC72H48yby5g1iA==
  • Authentication-results-original: xen.org; dkim=none (message not signed) header.d=none;xen.org; dmarc=none action=none header.from=arm.com;
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "Volodymyr_Babchuk@xxxxxxxx" <Volodymyr_Babchuk@xxxxxxxx>, Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
  • Delivery-date: Tue, 12 Jan 2021 13:12:53 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: xen.org; dkim=none (message not signed) header.d=none;xen.org; dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHW6HhLZe62+lKsX0KJMppOaCzey6oj0IcAgAAgdoCAAAc7AA==
  • Thread-topic: [PATCH] xen/arm: don't read aarch32 regs when aarch32 isn't available

Hi Julien,

> On 12 Jan 2021, at 12:46, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 12/01/2021 10:50, Bertrand Marquis wrote:
>> Hi Stefano,
>>> On 12 Jan 2021, at 00:16, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
>>> 
>>> Don't read aarch32 system registers at boot time when the aarch32 state
>>> is not available. They are UNKNOWN, so it is not useful to read them.
>>> Moreover, on Cavium ThunderX reading ID_PFR2_EL1 causes a Xen crash.
>>> Instead, only read them when aarch32 is available.
>>> 
>>> Leave the corresponding fields in struct cpuinfo_arm so that they
>>> are read-as-zero from a guest.
>> I agree with the fact that all users of identify_cpu are currently using 
>> spaces
>> which are 0 but to make the core a bit more robust we could do a memset(0)
>> of the structure at the beginning of the function.
>>> 
>>> Since we are editing identify_cpu, also fix the indentation: 4 spaces
>>> instead of 8.
>>> 
>>> Fixes: 9cfdb489af81 ("xen/arm: Add ID registers and complete cpuinfo")
>>> Link: https://marc.info/?l=xen-devel&m=161035501118086
>>> Link: 
>>> http://logs.test-lab.xenproject.org/osstest/logs/158293/test-arm64-arm64-xl-xsm/info.html
>>> Suggested-by: Julien Grall <julien@xxxxxxx>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
>>> ---
>>> xen/arch/arm/cpufeature.c | 35 +++++++++++++++++++++--------------
>>> 1 file changed, 21 insertions(+), 14 deletions(-)
>>> 
>>> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
>>> index 698bfa0201..b1c82ade49 100644
>>> --- a/xen/arch/arm/cpufeature.c
>>> +++ b/xen/arch/arm/cpufeature.c
>>> @@ -101,29 +101,35 @@ int enable_nonboot_cpu_caps(const struct 
>>> arm_cpu_capabilities *caps)
>>> 
>>> void identify_cpu(struct cpuinfo_arm *c)
>>> {
>>> -        c->midr.bits = READ_SYSREG(MIDR_EL1);
>>> -        c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
>>> +    bool aarch32 = true;
>>> +
>>> +    c->midr.bits = READ_SYSREG(MIDR_EL1);
>>> +    c->mpidr.bits = READ_SYSREG(MPIDR_EL1);
>>> 
>>> #ifdef CONFIG_ARM_64
>>> -        c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
>>> -        c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
>>> +    c->pfr64.bits[0] = READ_SYSREG(ID_AA64PFR0_EL1);
>>> +    c->pfr64.bits[1] = READ_SYSREG(ID_AA64PFR1_EL1);
>>> +
>>> +    c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
>>> +    c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
>>> 
>>> -        c->dbg64.bits[0] = READ_SYSREG(ID_AA64DFR0_EL1);
>>> -        c->dbg64.bits[1] = READ_SYSREG(ID_AA64DFR1_EL1);
>>> +    c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
>>> +    c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
>>> 
>>> -        c->aux64.bits[0] = READ_SYSREG(ID_AA64AFR0_EL1);
>>> -        c->aux64.bits[1] = READ_SYSREG(ID_AA64AFR1_EL1);
>>> +    c->mm64.bits[0]  = READ_SYSREG(ID_AA64MMFR0_EL1);
>>> +    c->mm64.bits[1]  = READ_SYSREG(ID_AA64MMFR1_EL1);
>>> +    c->mm64.bits[2]  = READ_SYSREG(ID_AA64MMFR2_EL1);
>>> 
>>> -        c->mm64.bits[0]  = READ_SYSREG(ID_AA64MMFR0_EL1);
>>> -        c->mm64.bits[1]  = READ_SYSREG(ID_AA64MMFR1_EL1);
>>> -        c->mm64.bits[2]  = READ_SYSREG(ID_AA64MMFR2_EL1);
>>> +    c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
>>> +    c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
>>> 
>>> -        c->isa64.bits[0] = READ_SYSREG(ID_AA64ISAR0_EL1);
>>> -        c->isa64.bits[1] = READ_SYSREG(ID_AA64ISAR1_EL1);
>>> +    c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
>>> 
>>> -        c->zfr64.bits[0] = READ_SYSREG(ID_AA64ZFR0_EL1);
>>> +    aarch32 = c->pfr64.el1 == 2;
>> I would put some () around the test.
>>> #endif
>>> 
>>> +    if ( aarch32 )
>>> +    {
>>>         c->pfr32.bits[0] = READ_SYSREG(ID_PFR0_EL1);
>>>         c->pfr32.bits[1] = READ_SYSREG(ID_PFR1_EL1);
>>>         c->pfr32.bits[2] = READ_SYSREG(ID_PFR2_EL1);
>>> @@ -153,6 +159,7 @@ void identify_cpu(struct cpuinfo_arm *c)
>>> #ifndef MVFR2_MAYBE_UNDEFINED
>>>         c->mvfr.bits[2] = READ_SYSREG(MVFR2_EL1);
>>> #endif
>> If we test for aarch32, the ifndef here should not be needed anymore.
>> > Is your previous patch really still needed if we go with the aarch32 
> path ?
> 
> There were two undefs discovered:
>   1) On Armv7 when accessing MVFR2_EL1
>   2) On Cavium Thunder-X (Armv8) when accessing ID_PFR2_EL1
> 
> If you remove the #ifdef, then you will re-introduce the UNDEF on Armv7.

Right sorry missed that one.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.