Hang with nVHE mode and SME

Hang with nVHE mode and SME

[Vincent Donnefort]

Hi All,

I'm experiencing hangs when running a guest on a Qemu platform and a host
started with nVHE mode.

The hang occurs in 

  static void __activate_traps(struct kvm_vcpu *vcpu)
  {
      ...
      if (cpus_have_final_cap(ARM64_SME)) {
           // HANG !

No problem though with either VHE or if the host boots with arm64.nosme.

The host (and the guest) are 6.1-rc1. My Qemu is 7.1.

My Qemu setup:

     $ qemu-system-aarch64 \
      -M virt \
      -machine virtualization=true -machine virt,gic-version=3  \
      -cpu max,pauth=off -smp 1 -m 16384                 \
      -drive file=rootfs.ext4,if=none,format=raw,id=hd0 \
      -device virtio-blk-device,drive=hd0 \
      -object rng-random,filename=/dev/urandom,id=rng0 \
      -netdev user,id=eth0,hostfwd=tcp::8022-:22,hostfwd=tcp::1234-:1234 \
      -device virtio-net-pci,netdev=eth0                \
      -nographic \
      -kernel arm64/boot/Image -append "earlycon root=/dev/vda kvm-arm.mode=nvhe nokaslr" \

The guest is run with kvm-tools:

     $ lkvm run -p "break=mount arm64.nosme" -k ~/Image --force-pci

Is it a known issue?

--
Vincent

Re: Hang with nVHE mode and SME

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1240 bytes --]

On Wed, Oct 26, 2022 at 03:29:44PM +0100, Vincent Donnefort wrote:

>   static void __activate_traps(struct kvm_vcpu *vcpu)
>   {
>       ...
>       if (cpus_have_final_cap(ARM64_SME)) {
>            // HANG !

You're not entirely specific on where the hang is - I assume you mean
the second SME block and that it's hanging on the first sysreg read in
there:

	if (cpus_have_final_cap(ARM64_SME)) {
		u64 val;

		val = read_sysreg_s(SYS_HFGRTR_EL2);

rather than on the if statement?  A brief grep around the qemu source
suggests that this register and fine grained traps in general are not
implemented which is invalid with SME since this is required for SME in
nVHE.  SME is a v9.2 feature, and v9.2 includes all the requirements of
v8.7.  FEAT_FGT has been mandatory since v8.6 (where it was added).

I did originally have a check in there for either/both this or FEAT_HCX
but it got taken out during review due to the architecturual
requirement.

> No problem though with either VHE or if the host boots with arm64.nosme.

> The host (and the guest) are 6.1-rc1. My Qemu is 7.1.

I've only ever tested KVM with the fast model, never with qemu FWIW.
FEAT_FGT is only used with SME when in nVHE mode.

> Is it a known issue?

It wasn't.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Hang with nVHE mode and SME

[Marc Zyngier]

+ Peter

On Wed, 26 Oct 2022 16:07:21 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> On Wed, Oct 26, 2022 at 03:29:44PM +0100, Vincent Donnefort wrote:
> 
> >   static void __activate_traps(struct kvm_vcpu *vcpu)
> >   {
> >       ...
> >       if (cpus_have_final_cap(ARM64_SME)) {
> >            // HANG !
> 
> You're not entirely specific on where the hang is - I assume you mean
> the second SME block and that it's hanging on the first sysreg read in
> there:
> 
> 	if (cpus_have_final_cap(ARM64_SME)) {
> 		u64 val;
> 
> 		val = read_sysreg_s(SYS_HFGRTR_EL2);
> 
> rather than on the if statement?  A brief grep around the qemu source
> suggests that this register and fine grained traps in general are not
> implemented which is invalid with SME since this is required for SME in
> nVHE.  SME is a v9.2 feature, and v9.2 includes all the requirements of
> v8.7.  FEAT_FGT has been mandatory since v8.6 (where it was added).

It is very unfortunate that the SME spec doesn't call out this
explicitly, while it is calling out the dependency on FEAT_HCX (OK,
one is mandatory and the other isn't).

> I did originally have a check in there for either/both this or FEAT_HCX
> but it got taken out during review due to the architecturual
> requirement.

So the question is whether we work around this in the kernel (not
enabling either KVM or SME if FEAT_FGT is not present), or leave it as
is with the hope that QEMU gets updated.

I'm inclined to do the latter. Thoughts anyone?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: Hang with nVHE mode and SME

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1677 bytes --]

On Wed, Oct 26, 2022 at 05:13:46PM +0100, Marc Zyngier wrote:
> Mark Brown <broonie@kernel.org> wrote:
> > On Wed, Oct 26, 2022 at 03:29:44PM +0100, Vincent Donnefort wrote:

> > rather than on the if statement?  A brief grep around the qemu source
> > suggests that this register and fine grained traps in general are not
> > implemented which is invalid with SME since this is required for SME in
> > nVHE.  SME is a v9.2 feature, and v9.2 includes all the requirements of
> > v8.7.  FEAT_FGT has been mandatory since v8.6 (where it was added).

> It is very unfortunate that the SME spec doesn't call out this
> explicitly, while it is calling out the dependency on FEAT_HCX (OK,
> one is mandatory and the other isn't).

Indeed.  It was there in the internal drafts but it looks like it got
removed as redundant when the v9.2 dependency was decided on but it does
make it harder to spot the interaction.

> > I did originally have a check in there for either/both this or FEAT_HCX
> > but it got taken out during review due to the architecturual
> > requirement.

> So the question is whether we work around this in the kernel (not
> enabling either KVM or SME if FEAT_FGT is not present), or leave it as
> is with the hope that QEMU gets updated.

> I'm inclined to do the latter. Thoughts anyone?

I tend to agree. 

Note that we don't need to use FGT in VHE mode so we could restrict
virtualisation with SME only in cases where we're in nVHE mode, though
narrowing the restriction does make things more complicated so may well
be more trouble than it's worth and we might need to revise if we start
using the SMPRI_El1 trap in future.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Hang with nVHE mode and SME

[Peter Maydell]

On Wed, 26 Oct 2022 at 17:34, Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Oct 26, 2022 at 05:13:46PM +0100, Marc Zyngier wrote:
> > Mark Brown <broonie@kernel.org> wrote:
> > > On Wed, Oct 26, 2022 at 03:29:44PM +0100, Vincent Donnefort wrote:
>
> > > rather than on the if statement?  A brief grep around the qemu source
> > > suggests that this register and fine grained traps in general are not
> > > implemented which is invalid with SME since this is required for SME in
> > > nVHE.  SME is a v9.2 feature, and v9.2 includes all the requirements of
> > > v8.7.  FEAT_FGT has been mandatory since v8.6 (where it was added).

Yeah. This is pretty common for QEMU, because we don't have the
resources to implement the full breadth of the architecture. We
just implement various features and rely on the kernel to do
fine-grained checking of ID registers rather than assuming that
because feature X is present feature Y must be too, or that because
some feature is in vX.Y then it must imply presence of all features
in some other vA.B. Mostly this works out OK...

> > It is very unfortunate that the SME spec doesn't call out this
> > explicitly, while it is calling out the dependency on FEAT_HCX (OK,
> > one is mandatory and the other isn't).
>
> Indeed.  It was there in the internal drafts but it looks like it got
> removed as redundant when the v9.2 dependency was decided on but it does
> make it harder to spot the interaction.
>
> > > I did originally have a check in there for either/both this or FEAT_HCX
> > > but it got taken out during review due to the architecturual
> > > requirement.
>
> > So the question is whether we work around this in the kernel (not
> > enabling either KVM or SME if FEAT_FGT is not present), or leave it as
> > is with the hope that QEMU gets updated.
>
> > I'm inclined to do the latter. Thoughts anyone?

Is the "kernel needs FEAT_FGT here" requirement a strong "the design
just means it's kind of expected" one, or a weak "the features
aren't really strongly tied together, it would be easy to do without"
one?

> I tend to agree.
>
> Note that we don't need to use FGT in VHE mode so we could restrict
> virtualisation with SME only in cases where we're in nVHE mode, though
> narrowing the restriction does make things more complicated so may well
> be more trouble than it's worth and we might need to revise if we start
> using the SMPRI_El1 trap in future.

We don't have any super-immediate plans to implement FGT, though
we might want to think about adjusting the roadmap.

We have released 7.1 with SME support, so you will see QEMU binaries
in the field with SME but not FGT.

thanks
-- PMM

Re: Hang with nVHE mode and SME

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

On Thu, Oct 27, 2022 at 10:44:39AM +0100, Peter Maydell wrote:
> On Wed, 26 Oct 2022 at 17:34, Mark Brown <broonie@kernel.org> wrote:
> > On Wed, Oct 26, 2022 at 05:13:46PM +0100, Marc Zyngier wrote:

> > > So the question is whether we work around this in the kernel (not
> > > enabling either KVM or SME if FEAT_FGT is not present), or leave it as
> > > is with the hope that QEMU gets updated.

> > > I'm inclined to do the latter. Thoughts anyone?

> Is the "kernel needs FEAT_FGT here" requirement a strong "the design
> just means it's kind of expected" one, or a weak "the features
> aren't really strongly tied together, it would be easy to do without"
> one?

We need it for nVHE mode since we need nTPIDR2_EL0 to trap access to
TPIDR2_EL0, in VHE mode this is controlled by SCTLR_EL2.EnTP2.  Looking
again I need to double check but I think I missed control of nSMPRI_EL1
which will be needed in both VHE and nVHE cases to trap access to
SMPRI_EL1, that's not controlled by the overall SME enable.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Hang with nVHE mode and SME

[Richard Henderson]

On 10/27/22 19:44, Peter Maydell wrote:
> On Wed, 26 Oct 2022 at 17:34, Mark Brown <broonie@kernel.org> wrote:
>>
>> On Wed, Oct 26, 2022 at 05:13:46PM +0100, Marc Zyngier wrote:
>>> Mark Brown <broonie@kernel.org> wrote:
>>>> On Wed, Oct 26, 2022 at 03:29:44PM +0100, Vincent Donnefort wrote:
>>
>>>> rather than on the if statement?  A brief grep around the qemu source
>>>> suggests that this register and fine grained traps in general are not
>>>> implemented which is invalid with SME since this is required for SME in
>>>> nVHE.  SME is a v9.2 feature, and v9.2 includes all the requirements of
>>>> v8.7.  FEAT_FGT has been mandatory since v8.6 (where it was added).
> 
> Yeah. This is pretty common for QEMU, because we don't have the
> resources to implement the full breadth of the architecture. We
> just implement various features and rely on the kernel to do
> fine-grained checking of ID registers rather than assuming that
> because feature X is present feature Y must be too, or that because
> some feature is in vX.Y then it must imply presence of all features
> in some other vA.B. Mostly this works out OK...

Yep, I knew I was skipping over FGT while implementing SME.
I left some TODO comments at the time.
I simply optimistically hoped that it wouldn't be critical.

We can certainly increase the priority.


r~