[PATCH] KVM: arm64: Hide unsupported MPAM from the guest

[PATCH] KVM: arm64: Hide unsupported MPAM from the guest

[James Morse]

Commit 011e5f5bf529 ("arm64/cpufeature: Add remaining feature bits in
ID_AA64PFR0 register") proactively added published features to the
cpufeature id registers.

If the platform supports these features, they are visible in the
sanitised ID registers that are exposed to KVM guests. This is a
problem as KVM doesn't support MPAM.

The hardware reset behaviour of MPAM is to be disabled at EL3. It
is unlikely anyone would ship a platform without firmware support,
the necessary initialisation has been upstream in the TF-A project
for over a year.

Firmware configures the EL2 registers to trap EL1 and EL0 access
to EL2. As KVM doesn't support MPAM, it doesn't change these
registers. Booting an MPAM capable kernel as a guest of mainline
causes KVM to take an unknown trap from an EL1 guest, and inject
an undef in response:
host:
| kvm [126]: Unsupported guest sys_reg access at: ffff800010093f24 [00000005]
|  { Op0( 3), Op1( 0), CRn(10), CRm( 5), Op2( 0), func_read },

guest:
| ------------[ cut here ]------------
| kernel BUG at arch/arm64/kernel/traps.c:409!
| Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
| Modules linked in:
| CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc1-00152-g570fa7e2d2ad #11605
| Hardware name: linux,dummy-virt (DT)
| pstate: 00000005 (nzcv daif -PAN -UAO)
| pc : do_undefinstr+0x2ec/0x310
| lr : do_undefinstr+0x2f8/0x310
...

This is a tad unfair on the guest as KVM said it supported the
feature. Mask out the MPAM feature.

Fixes: 011e5f5bf529 ("arm64/cpufeature: Add remaining feature bits in
ID_AA64PFR0 register")
Cc: <stable@vger.kernel.org>
Cc: Anshuman Khandual <anshuman.khandual@arm.com>
Signed-off-by: James Morse <james.morse@arm.com>

---
I'll be back at rc1 with the minimal KVM support to ensure the traps
are enabled and handled islently.
---
 arch/arm64/kvm/sys_regs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 077293b5115f..f736791f37ca 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1131,6 +1131,7 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 		if (!vcpu_has_sve(vcpu))
 			val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
 		val &= ~(0xfUL << ID_AA64PFR0_AMU_SHIFT);
+		val &= ~(0xfUL << ID_AA64PFR0_MPAM_SHIFT);
 	} else if (id == SYS_ID_AA64ISAR1_EL1 && !vcpu_has_ptrauth(vcpu)) {
 		val &= ~((0xfUL << ID_AA64ISAR1_APA_SHIFT) |
 			 (0xfUL << ID_AA64ISAR1_API_SHIFT) |
-- 
2.28.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Hide unsupported MPAM from the guest

[Andrew Jones]

On Fri, Sep 25, 2020 at 05:01:02PM +0100, James Morse wrote:
> Commit 011e5f5bf529 ("arm64/cpufeature: Add remaining feature bits in
> ID_AA64PFR0 register") proactively added published features to the
> cpufeature id registers.
> 
> If the platform supports these features, they are visible in the
> sanitised ID registers that are exposed to KVM guests. This is a
> problem as KVM doesn't support MPAM.
> 
> The hardware reset behaviour of MPAM is to be disabled at EL3. It
> is unlikely anyone would ship a platform without firmware support,
> the necessary initialisation has been upstream in the TF-A project
> for over a year.
> 
> Firmware configures the EL2 registers to trap EL1 and EL0 access
> to EL2. As KVM doesn't support MPAM, it doesn't change these
> registers. Booting an MPAM capable kernel as a guest of mainline
> causes KVM to take an unknown trap from an EL1 guest, and inject
> an undef in response:
> host:
> | kvm [126]: Unsupported guest sys_reg access at: ffff800010093f24 [00000005]
> |  { Op0( 3), Op1( 0), CRn(10), CRm( 5), Op2( 0), func_read },
> 
> guest:
> | ------------[ cut here ]------------
> | kernel BUG at arch/arm64/kernel/traps.c:409!
> | Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
> | Modules linked in:
> | CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc1-00152-g570fa7e2d2ad #11605
> | Hardware name: linux,dummy-virt (DT)
> | pstate: 00000005 (nzcv daif -PAN -UAO)
> | pc : do_undefinstr+0x2ec/0x310
> | lr : do_undefinstr+0x2f8/0x310
> ...
> 
> This is a tad unfair on the guest as KVM said it supported the
> feature. Mask out the MPAM feature.
> 
> Fixes: 011e5f5bf529 ("arm64/cpufeature: Add remaining feature bits in
> ID_AA64PFR0 register")
> Cc: <stable@vger.kernel.org>
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> 
> ---
> I'll be back at rc1 with the minimal KVM support to ensure the traps
> are enabled and handled islently.
> ---
>  arch/arm64/kvm/sys_regs.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 077293b5115f..f736791f37ca 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1131,6 +1131,7 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
>  		if (!vcpu_has_sve(vcpu))
>  			val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
>  		val &= ~(0xfUL << ID_AA64PFR0_AMU_SHIFT);
> +		val &= ~(0xfUL << ID_AA64PFR0_MPAM_SHIFT);
>  	} else if (id == SYS_ID_AA64ISAR1_EL1 && !vcpu_has_ptrauth(vcpu)) {
>  		val &= ~((0xfUL << ID_AA64ISAR1_APA_SHIFT) |
>  			 (0xfUL << ID_AA64ISAR1_API_SHIFT) |
> -- 
> 2.28.0
>

Hi James,

Thanks for this fix

Reviewed-by: Andrew Jones <drjones@redhat.com>

but, going forward, I think we need a more robust solution to CPU feature
additions in order to avoid these types of issues. Our current approach is
to patch KVM to hide features from the guest as we introduce support to
the [guest] kernel. IOW, we have to remember to maintain a guest CPU
feature reject-list. And, since that's error-prone, we should do regular
audits of the reject-list to ensure it's complete. It would be better to
have an accept-list (all features masked by default) and then only expose
features as we add the KVM support. Maybe we should introduce KVM masks
for each ID register? Also, regarding the current implementation, do you
know if a recent audit has been conducted to ensure (now with MPAM) that
the current feature hiding is complete?

Thanks,
drew


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Hide unsupported MPAM from the guest

[Marc Zyngier]

On 2020-09-26 10:48, Andrew Jones wrote:
> On Fri, Sep 25, 2020 at 05:01:02PM +0100, James Morse wrote:
>> Commit 011e5f5bf529 ("arm64/cpufeature: Add remaining feature bits in
>> ID_AA64PFR0 register") proactively added published features to the
>> cpufeature id registers.
>> 
>> If the platform supports these features, they are visible in the
>> sanitised ID registers that are exposed to KVM guests. This is a
>> problem as KVM doesn't support MPAM.
>> 
>> The hardware reset behaviour of MPAM is to be disabled at EL3. It
>> is unlikely anyone would ship a platform without firmware support,
>> the necessary initialisation has been upstream in the TF-A project
>> for over a year.
>> 
>> Firmware configures the EL2 registers to trap EL1 and EL0 access
>> to EL2. As KVM doesn't support MPAM, it doesn't change these
>> registers. Booting an MPAM capable kernel as a guest of mainline
>> causes KVM to take an unknown trap from an EL1 guest, and inject
>> an undef in response:
>> host:
>> | kvm [126]: Unsupported guest sys_reg access at: ffff800010093f24 
>> [00000005]
>> |  { Op0( 3), Op1( 0), CRn(10), CRm( 5), Op2( 0), func_read },
>> 
>> guest:
>> | ------------[ cut here ]------------
>> | kernel BUG at arch/arm64/kernel/traps.c:409!
>> | Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
>> | Modules linked in:
>> | CPU: 0 PID: 1 Comm: swapper/0 Not tainted 
>> 5.6.0-rc1-00152-g570fa7e2d2ad #11605
>> | Hardware name: linux,dummy-virt (DT)
>> | pstate: 00000005 (nzcv daif -PAN -UAO)
>> | pc : do_undefinstr+0x2ec/0x310
>> | lr : do_undefinstr+0x2f8/0x310
>> ...
>> 
>> This is a tad unfair on the guest as KVM said it supported the
>> feature. Mask out the MPAM feature.
>> 
>> Fixes: 011e5f5bf529 ("arm64/cpufeature: Add remaining feature bits in
>> ID_AA64PFR0 register")
>> Cc: <stable@vger.kernel.org>
>> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
>> Signed-off-by: James Morse <james.morse@arm.com>
>> 
>> ---
>> I'll be back at rc1 with the minimal KVM support to ensure the traps
>> are enabled and handled islently.
>> ---
>>  arch/arm64/kvm/sys_regs.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 077293b5115f..f736791f37ca 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1131,6 +1131,7 @@ static u64 read_id_reg(const struct kvm_vcpu 
>> *vcpu,
>>  		if (!vcpu_has_sve(vcpu))
>>  			val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
>>  		val &= ~(0xfUL << ID_AA64PFR0_AMU_SHIFT);
>> +		val &= ~(0xfUL << ID_AA64PFR0_MPAM_SHIFT);
>>  	} else if (id == SYS_ID_AA64ISAR1_EL1 && !vcpu_has_ptrauth(vcpu)) {
>>  		val &= ~((0xfUL << ID_AA64ISAR1_APA_SHIFT) |
>>  			 (0xfUL << ID_AA64ISAR1_API_SHIFT) |
>> --
>> 2.28.0
>> 
> 
> Hi James,
> 
> Thanks for this fix
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> 
> but, going forward, I think we need a more robust solution to CPU 
> feature
> additions in order to avoid these types of issues. Our current approach 
> is
> to patch KVM to hide features from the guest as we introduce support to
> the [guest] kernel. IOW, we have to remember to maintain a guest CPU
> feature reject-list. And, since that's error-prone, we should do 
> regular
> audits of the reject-list to ensure it's complete. It would be better 
> to
> have an accept-list (all features masked by default) and then only 
> expose
> features as we add the KVM support.

I have started doing that for the NV series [1], as our virtual CPU is 
much
more limited than the HW it runs on. It shouldn't be hard to turn this 
into
something more generic.

However, it doesn't say anything about the traps that can occur as the
architecture grows new extensions. The current position is to always
inject an UNDEF (exactly what James is doing here), but it isn't obvious
to me that it is always the right thing to do. We should probably drop 
the
dmesg screaming and convert it to a trace...

> Maybe we should introduce KVM masks
> for each ID register? Also, regarding the current implementation, do 
> you
> know if a recent audit has been conducted to ensure (now with MPAM) 
> that
> the current feature hiding is complete?

I doubt it is. The number of additions up to ARMv8.6 is huge, and 
someone
would need to carefully comb it and test it on FVP with all the possible
architectural knobs turned in various ways...

Thanks,

         M.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=kvm-arm64/nv-5.10-WIP&id=1669f02ebf8e0aa932549d9487ed6b4258351943
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] KVM: arm64: Hide unsupported MPAM from the guest

[James Morse]

Hi Marc, Drew,

On 28/09/2020 12:52, Marc Zyngier wrote:
> On 2020-09-26 10:48, Andrew Jones wrote:
>> On Fri, Sep 25, 2020 at 05:01:02PM +0100, James Morse wrote:
>>> Commit 011e5f5bf529 ("arm64/cpufeature: Add remaining feature bits in
>>> ID_AA64PFR0 register") proactively added published features to the
>>> cpufeature id registers.
>>>
>>> If the platform supports these features, they are visible in the
>>> sanitised ID registers that are exposed to KVM guests. This is a
>>> problem as KVM doesn't support MPAM.
>>>
>>> The hardware reset behaviour of MPAM is to be disabled at EL3. It
>>> is unlikely anyone would ship a platform without firmware support,
>>> the necessary initialisation has been upstream in the TF-A project
>>> for over a year.
>>>
>>> Firmware configures the EL2 registers to trap EL1 and EL0 access
>>> to EL2. As KVM doesn't support MPAM, it doesn't change these
>>> registers. Booting an MPAM capable kernel as a guest of mainline
>>> causes KVM to take an unknown trap from an EL1 guest, and inject
>>> an undef in response:

>>> This is a tad unfair on the guest as KVM said it supported the
>>> feature. Mask out the MPAM feature.


>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>> index 077293b5115f..f736791f37ca 100644
>>> --- a/arch/arm64/kvm/sys_regs.c
>>> +++ b/arch/arm64/kvm/sys_regs.c
>>> @@ -1131,6 +1131,7 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
>>>          if (!vcpu_has_sve(vcpu))
>>>              val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
>>>          val &= ~(0xfUL << ID_AA64PFR0_AMU_SHIFT);
>>> +        val &= ~(0xfUL << ID_AA64PFR0_MPAM_SHIFT);
>>>      } else if (id == SYS_ID_AA64ISAR1_EL1 && !vcpu_has_ptrauth(vcpu)) {
>>>          val &= ~((0xfUL << ID_AA64ISAR1_APA_SHIFT) |
>>>               (0xfUL << ID_AA64ISAR1_API_SHIFT) |


>> but, going forward, I think we need a more robust solution to CPU feature
>> additions in order to avoid these types of issues. Our current approach is
>> to patch KVM to hide features from the guest as we introduce support to
>> the [guest] kernel. IOW, we have to remember to maintain a guest CPU
>> feature reject-list. And, since that's error-prone, we should do regular
>> audits of the reject-list to ensure it's complete. It would be better to
>> have an accept-list (all features masked by default) and then only expose
>> features as we add the KVM support.

> I have started doing that for the NV series [1], as our virtual CPU is much
> more limited than the HW it runs on. It shouldn't be hard to turn this into
> something more generic.

I think having two lists is a problem as they have to be kept in sync. cpufeature has
'FTR_HIDDEN' which means "hidden from the EL0 emulation of mrs". I think this should be
split into FTR_NO_GUEST and FTR_NO_USER so that the person adding the entry has to answer
the question as to whether this feature is visible to the guest.


> However, it doesn't say anything about the traps that can occur as the
> architecture grows new extensions. The current position is to always
> inject an UNDEF (exactly what James is doing here), but it isn't obvious
> to me that it is always the right thing to do. We should probably drop the
> dmesg screaming and convert it to a trace...

I don't think these "unknown traps" should be silent, we need some kind of warning that
something bad is happening. Currently we wait until we take the trap before warning that
we don't really understand the platform, which is only half the story.

Assuming all new features have a new id-register entry, or are backward compatible: How do
you feel about a boot-time warning that the ID registers describe features this version of
the kernel doesn't understand? This would also catch the issue the other way round: an
unknown feature has guest-accessible state that didn't trap also generates a warning.
Today that is silent.
I think cpufeature can do this, because its equally applicable to the host.

(I'm not volunteering)


>> Maybe we should introduce KVM masks
>> for each ID register? Also, regarding the current implementation, do you
>> know if a recent audit has been conducted to ensure (now with MPAM) that
>> the current feature hiding is complete?

I checked the patch in the fixes tag, which is otherwise fine. But there were a few of
these, when they were on the list I got bogged down in reading up on some of the features.


Thanks,

James


> I doubt it is. The number of additions up to ARMv8.6 is huge, and someone
> would need to carefully comb it and test it on FVP with all the possible
> architectural knobs turned in various ways...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel