[REGRESSION] from 74a0e79df68a8042fb84fd7207e57b70722cf825: VFIO PCI passthrough no longer works

[REGRESSION] from 74a0e79df68a8042fb84fd7207e57b70722cf825: VFIO PCI passthrough no longer works

[Simon Pilkington]

Hi,

With the aforementioned commit I am no longer able to use PCI
passthrough to a
Windows guest on the X570 chipset with a 5950X CPU.

The minimal reproducer for me is to attach a GPU to the VM and attempt
to start
Windows setup from an iso image. The VM will apparently livelock at the
setup splash
screen before the spinner appears as one of my CPU cores goes up to 100%
usage
until I force off the VM. This could be very machine-specific though.

Reverting to the old XOR check fixes both 6.12.y stable and 6.13-rc2 for me.
Otherwise they're both bad. Can you please look into it? I can share the
config
I used for test builds if it would help.

Regards,
Simon

Re: [REGRESSION] from 74a0e79df68a8042fb84fd7207e57b70722cf825: VFIO PCI passthrough no longer works

[Sean Christopherson]

+Tom

On Tue, Dec 10, 2024, Simon Pilkington wrote:
> Hi,
> 
> With the aforementioned commit I am no longer able to use PCI passthrough to
> a Windows guest on the X570 chipset with a 5950X CPU.
> 
> The minimal reproducer for me is to attach a GPU to the VM and attempt to
> start Windows setup from an iso image. The VM will apparently livelock at the
> setup splash screen before the spinner appears as one of my CPU cores goes up
> to 100% usage until I force off the VM. This could be very machine-specific
> though.

Ugh.  Yeah, it's pretty much guaranteed to be CPU specific behavior.

Tom, any idea what the guest might be trying to do?  It probably doesn't matter
in the end, it's not like KVM does anything with the value...

> Reverting to the old XOR check fixes both 6.12.y stable and 6.13-rc2 for me.
> Otherwise they're both bad. Can you please look into it? I can share the
> config I used for test builds if it would help.

Can you run with the below to see what bits the guest is trying to set (or clear)?
We could get the same info via tracepoints, but this will likely be faster/easier.

---
 arch/x86/kvm/svm/svm.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index dd15cc635655..5144d0283c9d 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3195,11 +3195,14 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 	case MSR_AMD64_DE_CFG: {
 		u64 supported_de_cfg;
 
-		if (svm_get_feature_msr(ecx, &supported_de_cfg))
+		if (WARN_ON_ONCE(svm_get_feature_msr(ecx, &supported_de_cfg)))
 			return 1;
 
-		if (data & ~supported_de_cfg)
+		if (data & ~supported_de_cfg) {
+			pr_warn("DE_CFG supported = %llx, WRMSR = %llx\n",
+				supported_de_cfg, data);
 			return 1;
+		}
 
 		/*
 		 * Don't let the guest change the host-programmed value.  The
@@ -3207,8 +3210,11 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 		 * are completely unknown to KVM, and the one bit known to KVM
 		 * is simply a reflection of hardware capabilities.
 		 */
-		if (!msr->host_initiated && data != svm->msr_decfg)
+		if (!msr->host_initiated && data != svm->msr_decfg) {
+			pr_warn("DE_CFG current = %llx, WRMSR = %llx\n",
+				svm->msr_decfg, data);
 			return 1;
+		}
 
 		svm->msr_decfg = data;
 		break;

base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
-- 

Re: [REGRESSION] from 74a0e79df68a8042fb84fd7207e57b70722cf825: VFIO PCI passthrough no longer works

[Tom Lendacky]

On 12/10/24 09:47, Sean Christopherson wrote:
> +Tom
> 
> On Tue, Dec 10, 2024, Simon Pilkington wrote:
>> Hi,
>>
>> With the aforementioned commit I am no longer able to use PCI passthrough to
>> a Windows guest on the X570 chipset with a 5950X CPU.
>>
>> The minimal reproducer for me is to attach a GPU to the VM and attempt to
>> start Windows setup from an iso image. The VM will apparently livelock at the
>> setup splash screen before the spinner appears as one of my CPU cores goes up
>> to 100% usage until I force off the VM. This could be very machine-specific
>> though.
> 
> Ugh.  Yeah, it's pretty much guaranteed to be CPU specific behavior.
> 
> Tom, any idea what the guest might be trying to do?  It probably doesn't matter
> in the end, it's not like KVM does anything with the value...

No clue. I do see that in Linux there is a zenbleed-related bit that is
used in DE_CFG:

  522b1d69219d ("x86/cpu/amd: Add a Zenbleed fix")

I wonder if it might be related to that.

Your suggestion below to see what bits are being requested might shed
some light on it.

Thanks,
Tom

> 
>> Reverting to the old XOR check fixes both 6.12.y stable and 6.13-rc2 for me.
>> Otherwise they're both bad. Can you please look into it? I can share the
>> config I used for test builds if it would help.
> 
> Can you run with the below to see what bits the guest is trying to set (or clear)?
> We could get the same info via tracepoints, but this will likely be faster/easier.
> 
> ---
>  arch/x86/kvm/svm/svm.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index dd15cc635655..5144d0283c9d 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3195,11 +3195,14 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  	case MSR_AMD64_DE_CFG: {
>  		u64 supported_de_cfg;
>  
> -		if (svm_get_feature_msr(ecx, &supported_de_cfg))
> +		if (WARN_ON_ONCE(svm_get_feature_msr(ecx, &supported_de_cfg)))
>  			return 1;
>  
> -		if (data & ~supported_de_cfg)
> +		if (data & ~supported_de_cfg) {
> +			pr_warn("DE_CFG supported = %llx, WRMSR = %llx\n",
> +				supported_de_cfg, data);
>  			return 1;
> +		}
>  
>  		/*
>  		 * Don't let the guest change the host-programmed value.  The
> @@ -3207,8 +3210,11 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  		 * are completely unknown to KVM, and the one bit known to KVM
>  		 * is simply a reflection of hardware capabilities.
>  		 */
> -		if (!msr->host_initiated && data != svm->msr_decfg)
> +		if (!msr->host_initiated && data != svm->msr_decfg) {
> +			pr_warn("DE_CFG current = %llx, WRMSR = %llx\n",
> +				svm->msr_decfg, data);
>  			return 1;
> +		}
>  
>  		svm->msr_decfg = data;
>  		break;
> 
> base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4

Re: [REGRESSION] from 74a0e79df68a8042fb84fd7207e57b70722cf825: VFIO PCI passthrough no longer works

[Simon Pilkington]

On 10/12/2024 16:47, Sean Christopherson wrote:
> Can you run with the below to see what bits the guest is trying to set (or clear)?
> We could get the same info via tracepoints, but this will likely be faster/easier.
> 
> ---
>  arch/x86/kvm/svm/svm.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index dd15cc635655..5144d0283c9d 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3195,11 +3195,14 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  	case MSR_AMD64_DE_CFG: {
>  		u64 supported_de_cfg;
>  
> -		if (svm_get_feature_msr(ecx, &supported_de_cfg))
> +		if (WARN_ON_ONCE(svm_get_feature_msr(ecx, &supported_de_cfg)))
>  			return 1;
>  
> -		if (data & ~supported_de_cfg)
> +		if (data & ~supported_de_cfg) {
> +			pr_warn("DE_CFG supported = %llx, WRMSR = %llx\n",
> +				supported_de_cfg, data);
>  			return 1;
> +		}
>  
>  		/*
>  		 * Don't let the guest change the host-programmed value.  The
> @@ -3207,8 +3210,11 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  		 * are completely unknown to KVM, and the one bit known to KVM
>  		 * is simply a reflection of hardware capabilities.
>  		 */
> -		if (!msr->host_initiated && data != svm->msr_decfg)
> +		if (!msr->host_initiated && data != svm->msr_decfg) {
> +			pr_warn("DE_CFG current = %llx, WRMSR = %llx\n",
> +				svm->msr_decfg, data);
>  			return 1;
> +		}
>  
>  		svm->msr_decfg = data;
>  		break;
> 
> base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4

Relevant dmesg output with some context below. VM locked up as expected.

[   85.834971] vfio-pci 0000:0c:00.0: resetting
[   85.937573] vfio-pci 0000:0c:00.0: reset done
[   86.494210] vfio-pci 0000:0c:00.0: resetting
[   86.494264] vfio-pci 0000:0c:00.1: resetting
[   86.761442] vfio-pci 0000:0c:00.0: reset done
[   86.761480] vfio-pci 0000:0c:00.1: reset done
[   86.762392] vfio-pci 0000:0c:00.0: resetting
[   86.865462] vfio-pci 0000:0c:00.0: reset done
[   86.977360] virbr0: port 1(vnet1) entered learning state
[   88.993052] virbr0: port 1(vnet1) entered forwarding state
[   88.993057] virbr0: topology change detected, propagating
[  103.459114] kvm_amd: DE_CFG current = 0, WRMSR = 2
[  161.442032] virbr0: port 1(vnet1) entered disabled state // VM shut down

Re: [REGRESSION] from 74a0e79df68a8042fb84fd7207e57b70722cf825: VFIO PCI passthrough no longer works

[Tom Lendacky]

On 12/10/24 14:33, Simon Pilkington wrote:
> On 10/12/2024 16:47, Sean Christopherson wrote:
>> Can you run with the below to see what bits the guest is trying to set (or clear)?
>> We could get the same info via tracepoints, but this will likely be faster/easier.
>>
>> ---
>>  arch/x86/kvm/svm/svm.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index dd15cc635655..5144d0283c9d 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -3195,11 +3195,14 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>>  	case MSR_AMD64_DE_CFG: {
>>  		u64 supported_de_cfg;
>>  
>> -		if (svm_get_feature_msr(ecx, &supported_de_cfg))
>> +		if (WARN_ON_ONCE(svm_get_feature_msr(ecx, &supported_de_cfg)))
>>  			return 1;
>>  
>> -		if (data & ~supported_de_cfg)
>> +		if (data & ~supported_de_cfg) {
>> +			pr_warn("DE_CFG supported = %llx, WRMSR = %llx\n",
>> +				supported_de_cfg, data);
>>  			return 1;
>> +		}
>>  
>>  		/*
>>  		 * Don't let the guest change the host-programmed value.  The
>> @@ -3207,8 +3210,11 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>>  		 * are completely unknown to KVM, and the one bit known to KVM
>>  		 * is simply a reflection of hardware capabilities.
>>  		 */
>> -		if (!msr->host_initiated && data != svm->msr_decfg)
>> +		if (!msr->host_initiated && data != svm->msr_decfg) {
>> +			pr_warn("DE_CFG current = %llx, WRMSR = %llx\n",
>> +				svm->msr_decfg, data);
>>  			return 1;
>> +		}
>>  
>>  		svm->msr_decfg = data;
>>  		break;
>>
>> base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
> 
> Relevant dmesg output with some context below. VM locked up as expected.
> 
> [   85.834971] vfio-pci 0000:0c:00.0: resetting
> [   85.937573] vfio-pci 0000:0c:00.0: reset done
> [   86.494210] vfio-pci 0000:0c:00.0: resetting
> [   86.494264] vfio-pci 0000:0c:00.1: resetting
> [   86.761442] vfio-pci 0000:0c:00.0: reset done
> [   86.761480] vfio-pci 0000:0c:00.1: reset done
> [   86.762392] vfio-pci 0000:0c:00.0: resetting
> [   86.865462] vfio-pci 0000:0c:00.0: reset done
> [   86.977360] virbr0: port 1(vnet1) entered learning state
> [   88.993052] virbr0: port 1(vnet1) entered forwarding state
> [   88.993057] virbr0: topology change detected, propagating
> [  103.459114] kvm_amd: DE_CFG current = 0, WRMSR = 2
> [  161.442032] virbr0: port 1(vnet1) entered disabled state // VM shut down

That is the MSR_AMD64_DE_CFG_LFENCE_SERIALIZE bit. Yeah, that actually
does change the behavior of LFENCE and isn't just a reflection of the
hardware.

Linux does set that bit on boot, too (if LFENCE always serializing isn't
advertised 8000_0021_EAX[2]), so I'm kind of surprised it didn't pop up
there.

I imagine that the above CPUID bit isn't set, so an attempt is made to
set the MSR bit.

Thanks,
Tom

Re: [REGRESSION] from 74a0e79df68a8042fb84fd7207e57b70722cf825: VFIO PCI passthrough no longer works

[Sean Christopherson]

On Tue, Dec 10, 2024, Tom Lendacky wrote:
> On 12/10/24 14:33, Simon Pilkington wrote:
> > On 10/12/2024 16:47, Sean Christopherson wrote:
> >> Can you run with the below to see what bits the guest is trying to set (or clear)?
> >> We could get the same info via tracepoints, but this will likely be faster/easier.
> >>
> >> ---
> >>  arch/x86/kvm/svm/svm.c | 12 +++++++++---
> >>  1 file changed, 9 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> >> index dd15cc635655..5144d0283c9d 100644
> >> --- a/arch/x86/kvm/svm/svm.c
> >> +++ b/arch/x86/kvm/svm/svm.c
> >> @@ -3195,11 +3195,14 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
> >>  	case MSR_AMD64_DE_CFG: {
> >>  		u64 supported_de_cfg;
> >>  
> >> -		if (svm_get_feature_msr(ecx, &supported_de_cfg))
> >> +		if (WARN_ON_ONCE(svm_get_feature_msr(ecx, &supported_de_cfg)))
> >>  			return 1;
> >>  
> >> -		if (data & ~supported_de_cfg)
> >> +		if (data & ~supported_de_cfg) {
> >> +			pr_warn("DE_CFG supported = %llx, WRMSR = %llx\n",
> >> +				supported_de_cfg, data);
> >>  			return 1;
> >> +		}
> >>  
> >>  		/*
> >>  		 * Don't let the guest change the host-programmed value.  The
> >> @@ -3207,8 +3210,11 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
> >>  		 * are completely unknown to KVM, and the one bit known to KVM
> >>  		 * is simply a reflection of hardware capabilities.
> >>  		 */
> >> -		if (!msr->host_initiated && data != svm->msr_decfg)
> >> +		if (!msr->host_initiated && data != svm->msr_decfg) {
> >> +			pr_warn("DE_CFG current = %llx, WRMSR = %llx\n",
> >> +				svm->msr_decfg, data);
> >>  			return 1;
> >> +		}
> >>  
> >>  		svm->msr_decfg = data;
> >>  		break;
> >>
> >> base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
> > 
> > Relevant dmesg output with some context below. VM locked up as expected.
> > 
> > [   85.834971] vfio-pci 0000:0c:00.0: resetting
> > [   85.937573] vfio-pci 0000:0c:00.0: reset done
> > [   86.494210] vfio-pci 0000:0c:00.0: resetting
> > [   86.494264] vfio-pci 0000:0c:00.1: resetting
> > [   86.761442] vfio-pci 0000:0c:00.0: reset done
> > [   86.761480] vfio-pci 0000:0c:00.1: reset done
> > [   86.762392] vfio-pci 0000:0c:00.0: resetting
> > [   86.865462] vfio-pci 0000:0c:00.0: reset done
> > [   86.977360] virbr0: port 1(vnet1) entered learning state
> > [   88.993052] virbr0: port 1(vnet1) entered forwarding state
> > [   88.993057] virbr0: topology change detected, propagating
> > [  103.459114] kvm_amd: DE_CFG current = 0, WRMSR = 2
> > [  161.442032] virbr0: port 1(vnet1) entered disabled state // VM shut down
> 
> That is the MSR_AMD64_DE_CFG_LFENCE_SERIALIZE bit. Yeah, that actually
> does change the behavior of LFENCE and isn't just a reflection of the
> hardware.
> 
> Linux does set that bit on boot, too (if LFENCE always serializing isn't
> advertised 8000_0021_EAX[2]), so I'm kind of surprised it didn't pop up
> there.

Linux may be running afoul of this, but it would only become visible if someone
checked dmesg.  Even the "unsafe" MSR accesses in Linux gracefully handle faults
these days, the only symptom would be a WARN.

> I imagine that the above CPUID bit isn't set, so an attempt is made to
> set the MSR bit.

Yep.  And LFENCE_RDTSC _is_ supported, otherwise the supported_de_cfg check would
have failed.  Which means it's a-ok for the guest to set the bit, i.e. KVM won't
let the guest incorrectly think it's running on CPU for which LFENCE is serializing.

Unless you (Tom) disagree, I vote to simply drop the offending code, i.e. make
all supported bits fully writable from the guest.  KVM is firmly in the wrong here,
and I can't think of any reason to disallow the guest from clearing LFENCE_SERIALIZE.

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 6a350cee2f6c..5a82ead3bf0f 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3201,15 +3201,6 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
                if (data & ~supported_de_cfg)
                        return 1;
 
-               /*
-                * Don't let the guest change the host-programmed value.  The
-                * MSR is very model specific, i.e. contains multiple bits that
-                * are completely unknown to KVM, and the one bit known to KVM
-                * is simply a reflection of hardware capabilities.
-                */
-               if (!msr->host_initiated && data != svm->msr_decfg)
-                       return 1;
-
                svm->msr_decfg = data;
                break;
        }

Re: [REGRESSION] from 74a0e79df68a8042fb84fd7207e57b70722cf825: VFIO PCI passthrough no longer works

[Simon Pilkington]

On 10/12/2024 23:43, Sean Christopherson wrote:
> Unless you (Tom) disagree, I vote to simply drop the offending code, i.e. make
> all supported bits fully writable from the guest.  KVM is firmly in the wrong here,
> and I can't think of any reason to disallow the guest from clearing LFENCE_SERIALIZE.
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 6a350cee2f6c..5a82ead3bf0f 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3201,15 +3201,6 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>                 if (data & ~supported_de_cfg)
>                         return 1;
>  
> -               /*
> -                * Don't let the guest change the host-programmed value.  The
> -                * MSR is very model specific, i.e. contains multiple bits that
> -                * are completely unknown to KVM, and the one bit known to KVM
> -                * is simply a reflection of hardware capabilities.
> -                */
> -               if (!msr->host_initiated && data != svm->msr_decfg)
> -                       return 1;
> -
>                 svm->msr_decfg = data;
>                 break;
>         }
> 

This also produces a good kernel.

Re: [REGRESSION] from 74a0e79df68a8042fb84fd7207e57b70722cf825: VFIO PCI passthrough no longer works

[Tom Lendacky]

On 12/10/24 16:43, Sean Christopherson wrote:
> On Tue, Dec 10, 2024, Tom Lendacky wrote:
>> On 12/10/24 14:33, Simon Pilkington wrote:
>>> On 10/12/2024 16:47, Sean Christopherson wrote:
>>>> Can you run with the below to see what bits the guest is trying to set (or clear)?
>>>> We could get the same info via tracepoints, but this will likely be faster/easier.
>>>>
>>>> ---
>>>>  arch/x86/kvm/svm/svm.c | 12 +++++++++---
>>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>>> index dd15cc635655..5144d0283c9d 100644
>>>> --- a/arch/x86/kvm/svm/svm.c
>>>> +++ b/arch/x86/kvm/svm/svm.c
>>>> @@ -3195,11 +3195,14 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>>>>  	case MSR_AMD64_DE_CFG: {
>>>>  		u64 supported_de_cfg;
>>>>  
>>>> -		if (svm_get_feature_msr(ecx, &supported_de_cfg))
>>>> +		if (WARN_ON_ONCE(svm_get_feature_msr(ecx, &supported_de_cfg)))
>>>>  			return 1;
>>>>  
>>>> -		if (data & ~supported_de_cfg)
>>>> +		if (data & ~supported_de_cfg) {
>>>> +			pr_warn("DE_CFG supported = %llx, WRMSR = %llx\n",
>>>> +				supported_de_cfg, data);
>>>>  			return 1;
>>>> +		}
>>>>  
>>>>  		/*
>>>>  		 * Don't let the guest change the host-programmed value.  The
>>>> @@ -3207,8 +3210,11 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>>>>  		 * are completely unknown to KVM, and the one bit known to KVM
>>>>  		 * is simply a reflection of hardware capabilities.
>>>>  		 */
>>>> -		if (!msr->host_initiated && data != svm->msr_decfg)
>>>> +		if (!msr->host_initiated && data != svm->msr_decfg) {
>>>> +			pr_warn("DE_CFG current = %llx, WRMSR = %llx\n",
>>>> +				svm->msr_decfg, data);
>>>>  			return 1;
>>>> +		}
>>>>  
>>>>  		svm->msr_decfg = data;
>>>>  		break;
>>>>
>>>> base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
>>>
>>> Relevant dmesg output with some context below. VM locked up as expected.
>>>
>>> [   85.834971] vfio-pci 0000:0c:00.0: resetting
>>> [   85.937573] vfio-pci 0000:0c:00.0: reset done
>>> [   86.494210] vfio-pci 0000:0c:00.0: resetting
>>> [   86.494264] vfio-pci 0000:0c:00.1: resetting
>>> [   86.761442] vfio-pci 0000:0c:00.0: reset done
>>> [   86.761480] vfio-pci 0000:0c:00.1: reset done
>>> [   86.762392] vfio-pci 0000:0c:00.0: resetting
>>> [   86.865462] vfio-pci 0000:0c:00.0: reset done
>>> [   86.977360] virbr0: port 1(vnet1) entered learning state
>>> [   88.993052] virbr0: port 1(vnet1) entered forwarding state
>>> [   88.993057] virbr0: topology change detected, propagating
>>> [  103.459114] kvm_amd: DE_CFG current = 0, WRMSR = 2
>>> [  161.442032] virbr0: port 1(vnet1) entered disabled state // VM shut down
>>
>> That is the MSR_AMD64_DE_CFG_LFENCE_SERIALIZE bit. Yeah, that actually
>> does change the behavior of LFENCE and isn't just a reflection of the
>> hardware.
>>
>> Linux does set that bit on boot, too (if LFENCE always serializing isn't
>> advertised 8000_0021_EAX[2]), so I'm kind of surprised it didn't pop up
>> there.
> 
> Linux may be running afoul of this, but it would only become visible if someone
> checked dmesg.  Even the "unsafe" MSR accesses in Linux gracefully handle faults
> these days, the only symptom would be a WARN.
> 
>> I imagine that the above CPUID bit isn't set, so an attempt is made to
>> set the MSR bit.
> 
> Yep.  And LFENCE_RDTSC _is_ supported, otherwise the supported_de_cfg check would
> have failed.  Which means it's a-ok for the guest to set the bit, i.e. KVM won't
> let the guest incorrectly think it's running on CPU for which LFENCE is serializing.
> 
> Unless you (Tom) disagree, I vote to simply drop the offending code, i.e. make
> all supported bits fully writable from the guest.  KVM is firmly in the wrong here,
> and I can't think of any reason to disallow the guest from clearing LFENCE_SERIALIZE.
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 6a350cee2f6c..5a82ead3bf0f 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3201,15 +3201,6 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>                 if (data & ~supported_de_cfg)
>                         return 1;
>  
> -               /*
> -                * Don't let the guest change the host-programmed value.  The
> -                * MSR is very model specific, i.e. contains multiple bits that
> -                * are completely unknown to KVM, and the one bit known to KVM
> -                * is simply a reflection of hardware capabilities.
> -                */
> -               if (!msr->host_initiated && data != svm->msr_decfg)
> -                       return 1;
> -

That works for me.

Thanks,
Tom

>                 svm->msr_decfg = data;
>                 break;
>         }
> 

Re: [REGRESSION] from 74a0e79df68a8042fb84fd7207e57b70722cf825: VFIO PCI passthrough no longer works

[Simon Pilkington]

On 11/12/2024 15:37, Tom Lendacky wrote:
> On 12/10/24 16:43, Sean Christopherson wrote:
>> On Tue, Dec 10, 2024, Tom Lendacky wrote:
>>> On 12/10/24 14:33, Simon Pilkington wrote:
>>>> On 10/12/2024 16:47, Sean Christopherson wrote:
>>>>> Can you run with the below to see what bits the guest is trying to set (or clear)?
>>>>> We could get the same info via tracepoints, but this will likely be faster/easier.
>>>>>
>>>>> ---
>>>>>  arch/x86/kvm/svm/svm.c | 12 +++++++++---
>>>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>>>> index dd15cc635655..5144d0283c9d 100644
>>>>> --- a/arch/x86/kvm/svm/svm.c
>>>>> +++ b/arch/x86/kvm/svm/svm.c
>>>>> @@ -3195,11 +3195,14 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>>>>>  	case MSR_AMD64_DE_CFG: {
>>>>>  		u64 supported_de_cfg;
>>>>>  
>>>>> -		if (svm_get_feature_msr(ecx, &supported_de_cfg))
>>>>> +		if (WARN_ON_ONCE(svm_get_feature_msr(ecx, &supported_de_cfg)))
>>>>>  			return 1;
>>>>>  
>>>>> -		if (data & ~supported_de_cfg)
>>>>> +		if (data & ~supported_de_cfg) {
>>>>> +			pr_warn("DE_CFG supported = %llx, WRMSR = %llx\n",
>>>>> +				supported_de_cfg, data);
>>>>>  			return 1;
>>>>> +		}
>>>>>  
>>>>>  		/*
>>>>>  		 * Don't let the guest change the host-programmed value.  The
>>>>> @@ -3207,8 +3210,11 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>>>>>  		 * are completely unknown to KVM, and the one bit known to KVM
>>>>>  		 * is simply a reflection of hardware capabilities.
>>>>>  		 */
>>>>> -		if (!msr->host_initiated && data != svm->msr_decfg)
>>>>> +		if (!msr->host_initiated && data != svm->msr_decfg) {
>>>>> +			pr_warn("DE_CFG current = %llx, WRMSR = %llx\n",
>>>>> +				svm->msr_decfg, data);
>>>>>  			return 1;
>>>>> +		}
>>>>>  
>>>>>  		svm->msr_decfg = data;
>>>>>  		break;
>>>>>
>>>>> base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
>>>>
>>>> Relevant dmesg output with some context below. VM locked up as expected.
>>>>
>>>> [   85.834971] vfio-pci 0000:0c:00.0: resetting
>>>> [   85.937573] vfio-pci 0000:0c:00.0: reset done
>>>> [   86.494210] vfio-pci 0000:0c:00.0: resetting
>>>> [   86.494264] vfio-pci 0000:0c:00.1: resetting
>>>> [   86.761442] vfio-pci 0000:0c:00.0: reset done
>>>> [   86.761480] vfio-pci 0000:0c:00.1: reset done
>>>> [   86.762392] vfio-pci 0000:0c:00.0: resetting
>>>> [   86.865462] vfio-pci 0000:0c:00.0: reset done
>>>> [   86.977360] virbr0: port 1(vnet1) entered learning state
>>>> [   88.993052] virbr0: port 1(vnet1) entered forwarding state
>>>> [   88.993057] virbr0: topology change detected, propagating
>>>> [  103.459114] kvm_amd: DE_CFG current = 0, WRMSR = 2
>>>> [  161.442032] virbr0: port 1(vnet1) entered disabled state // VM shut down
>>>
>>> That is the MSR_AMD64_DE_CFG_LFENCE_SERIALIZE bit. Yeah, that actually
>>> does change the behavior of LFENCE and isn't just a reflection of the
>>> hardware.
>>>
>>> Linux does set that bit on boot, too (if LFENCE always serializing isn't
>>> advertised 8000_0021_EAX[2]), so I'm kind of surprised it didn't pop up
>>> there.
>>
>> Linux may be running afoul of this, but it would only become visible if someone
>> checked dmesg.  Even the "unsafe" MSR accesses in Linux gracefully handle faults
>> these days, the only symptom would be a WARN.
>>
>>> I imagine that the above CPUID bit isn't set, so an attempt is made to
>>> set the MSR bit.
>>
>> Yep.  And LFENCE_RDTSC _is_ supported, otherwise the supported_de_cfg check would
>> have failed.  Which means it's a-ok for the guest to set the bit, i.e. KVM won't
>> let the guest incorrectly think it's running on CPU for which LFENCE is serializing.
>>
>> Unless you (Tom) disagree, I vote to simply drop the offending code, i.e. make
>> all supported bits fully writable from the guest.  KVM is firmly in the wrong here,
>> and I can't think of any reason to disallow the guest from clearing LFENCE_SERIALIZE.
>>
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index 6a350cee2f6c..5a82ead3bf0f 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -3201,15 +3201,6 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>>                 if (data & ~supported_de_cfg)
>>                         return 1;
>>  
>> -               /*
>> -                * Don't let the guest change the host-programmed value.  The
>> -                * MSR is very model specific, i.e. contains multiple bits that
>> -                * are completely unknown to KVM, and the one bit known to KVM
>> -                * is simply a reflection of hardware capabilities.
>> -                */
>> -               if (!msr->host_initiated && data != svm->msr_decfg)
>> -                       return 1;
>> -
> 
> That works for me.
> 
> Thanks,
> Tom
> 
>>                 svm->msr_decfg = data;
>>                 break;
>>         }
>>

Thanks for the prompt response on this Sean & Tom.

Regards,
Simon