[PATCH] x86/hvm: Process pending softirqs while dumping VMC[SB]s

[PATCH] x86/hvm: Process pending softirqs while dumping VMC[SB]s

[Andrew Cooper]

24 guests with 8 vcpus each is sufficient to hit a 5 second watchdog.

Drop a piece of trailing whitespace while here.

Reported-by: Aidan Allen <aidan.allen1@cloud.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Aidan Allen <aidan.allen1@cloud.com>
---
 xen/arch/x86/hvm/svm/vmcb.c | 4 ++++
 xen/arch/x86/hvm/vmx/vmcs.c | 4 +++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 4e1f61dbe038..839d3ff91b5a 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -12,6 +12,8 @@
 #include <xen/mm.h>
 #include <xen/rcupdate.h>
 #include <xen/sched.h>
+#include <xen/softirq.h>
+
 #include <asm/hvm/svm/vmcb.h>
 #include <asm/msr-index.h>
 #include <asm/p2m.h>
@@ -246,6 +248,8 @@ static void cf_check vmcb_dump(unsigned char ch)
             }
             printk("\tVCPU %d\n", v->vcpu_id);
             svm_vmcb_dump("key_handler", v->arch.hvm.svm.vmcb);
+
+            process_pending_softirqs();
         }
     }
 
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 57d49364db56..57bae6679dd5 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -2165,7 +2165,7 @@ static void cf_check vmcs_dump(unsigned char ch)
 {
     struct domain *d;
     struct vcpu *v;
-    
+
     printk("*********** VMCS Areas **************\n");
 
     rcu_read_lock(&domlist_read_lock);
@@ -2184,6 +2184,8 @@ static void cf_check vmcs_dump(unsigned char ch)
             }
             printk("\tVCPU %d\n", v->vcpu_id);
             vmcs_dump_vcpu(v);
+
+            process_pending_softirqs();
         }
     }
 

base-commit: eb57fe072232c9836d085020450ce1434b21a819
prerequisite-patch-id: 32a8746877e6b92075be2f022dca25c6bfa6f31e
prerequisite-patch-id: a048b84683314d3a731d79fb3cb11406afa29d7b
-- 
2.39.5

Re: [PATCH] x86/hvm: Process pending softirqs while dumping VMC[SB]s

[Roger Pau Monné]

On Wed, Jun 04, 2025 at 02:02:53PM +0100, Andrew Cooper wrote:
> 24 guests with 8 vcpus each is sufficient to hit a 5 second watchdog.
> 
> Drop a piece of trailing whitespace while here.
> 
> Reported-by: Aidan Allen <aidan.allen1@cloud.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH] x86/hvm: Process pending softirqs while dumping VMC[SB]s

[Aidan Allen]

On Wed, Jun 4, 2025 at 2:02 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> 24 guests with 8 vcpus each is sufficient to hit a 5 second watchdog.
>
> Drop a piece of trailing whitespace while here.
>
> Reported-by: Aidan Allen <aidan.allen1@cloud.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

LGTM. Boots fine and cannot reproduce the crash with this patch.

Tested-by: Aidan Allen <aidan.allen1@cloud.com>

Thanks, Aidan.

Re: [PATCH] x86/hvm: Process pending softirqs while dumping VMC[SB]s

[Jan Beulich]

On 04.06.2025 15:02, Andrew Cooper wrote:
> @@ -246,6 +248,8 @@ static void cf_check vmcb_dump(unsigned char ch)
>              }
>              printk("\tVCPU %d\n", v->vcpu_id);
>              svm_vmcb_dump("key_handler", v->arch.hvm.svm.vmcb);
> +
> +            process_pending_softirqs();

It's only an RCU read lock we're holding here, but it still feels somewhat
odd to do this with any kind of lock held. Then again (I didn't even
consider this upon earlier insertions of such into keyhandler functions)
we may even be holding a real lock (the sysctl one) when getting here, yet
apparently that was deemed fine in the past. Plus dump_domains() does the
same as what we end up with here ...

Jan

Re: [PATCH] x86/hvm: Process pending softirqs while dumping VMC[SB]s

[Andrew Cooper]

On 04/06/2025 4:15 pm, Jan Beulich wrote:
> On 04.06.2025 15:02, Andrew Cooper wrote:
>> @@ -246,6 +248,8 @@ static void cf_check vmcb_dump(unsigned char ch)
>>              }
>>              printk("\tVCPU %d\n", v->vcpu_id);
>>              svm_vmcb_dump("key_handler", v->arch.hvm.svm.vmcb);
>> +
>> +            process_pending_softirqs();
> It's only an RCU read lock we're holding here, but it still feels somewhat
> odd to do this with any kind of lock held. Then again (I didn't even
> consider this upon earlier insertions of such into keyhandler functions)
> we may even be holding a real lock (the sysctl one) when getting here, yet
> apparently that was deemed fine in the past. Plus dump_domains() does the
> same as what we end up with here ...

The debug keys are debug functionality, and do play rather fast and loose.

While the Xen watchdog does hit first (5s), spending too long does cause
problems for the vCPU that's interrupted (usually soft lockup).

I was wondering if we should force schedule to idle before running most
keyhandlers.  That prevents holding a vCPU hostage (and if it's hard
pinned, then tough luck).

We would want a way of blocking further sysctl-debug-key's while one is
pending.

~Andrew

Re: [PATCH] x86/hvm: Process pending softirqs while dumping VMC[SB]s

[Jan Beulich]

On 04.06.2025 17:20, Andrew Cooper wrote:
> On 04/06/2025 4:15 pm, Jan Beulich wrote:
>> On 04.06.2025 15:02, Andrew Cooper wrote:
>>> @@ -246,6 +248,8 @@ static void cf_check vmcb_dump(unsigned char ch)
>>>              }
>>>              printk("\tVCPU %d\n", v->vcpu_id);
>>>              svm_vmcb_dump("key_handler", v->arch.hvm.svm.vmcb);
>>> +
>>> +            process_pending_softirqs();
>> It's only an RCU read lock we're holding here, but it still feels somewhat
>> odd to do this with any kind of lock held. Then again (I didn't even
>> consider this upon earlier insertions of such into keyhandler functions)
>> we may even be holding a real lock (the sysctl one) when getting here, yet
>> apparently that was deemed fine in the past. Plus dump_domains() does the
>> same as what we end up with here ...
> 
> The debug keys are debug functionality, and do play rather fast and loose.
> 
> While the Xen watchdog does hit first (5s), spending too long does cause
> problems for the vCPU that's interrupted (usually soft lockup).
> 
> I was wondering if we should force schedule to idle before running most
> keyhandlers.  That prevents holding a vCPU hostage (and if it's hard
> pinned, then tough luck).

We already invoke a tasklet in most situations, the main exception being
invocation via sysctl afaict.

> We would want a way of blocking further sysctl-debug-key's while one is
> pending.

That's guaranteed already by the sysctl lock, isn't it? Or did you mean
blocking sysctl ones while a non-sysctl one is in progress? (Along the
lines of what you say in the first sentence of your reply, right now we
simply assume responsible use by the host admin here.)

Jan