On Wed, 2022-11-23 at 19:32 +0000, Sean Christopherson wrote:
> On Wed, Nov 23, 2022, David Woodhouse wrote:
> > Now, perhaps a GPC user which knows that *it* is not going to use its
> > GPC from IRQ context, could refrain from bothering to disable
> > interrupts when taking its own gpc->lock. And that's probably true for
> > the runstate area.
>
> This is effectively what I was asking about.
So yes, I suppose we could do that. Incrementally...
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index af5ac495feec..5b93c3ed137c 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -177,7 +177,6 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
size_t user_len, user_len1, user_len2;
struct vcpu_runstate_info rs;
int *rs_state = &rs.state;
- unsigned long flags;
size_t times_ofs;
u8 *update_bit;
@@ -236,9 +235,9 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
* Attempt to obtain the GPC lock on *both* (if there are two)
* gfn_to_pfn caches that cover the region.
*/
- read_lock_irqsave(&gpc1->lock, flags);
+ read_lock(&gpc1->lock);
while (!kvm_gpc_check(gpc1, user_len1)) {
- read_unlock_irqrestore(&gpc1->lock, flags);
+ read_unlock(&gpc1->lock);
/* When invoked from kvm_sched_out() we cannot sleep */
if (atomic)
@@ -247,7 +246,7 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
if (kvm_gpc_refresh(gpc1, user_len1))
return;
- read_lock_irqsave(&gpc1->lock, flags);
+ read_lock(&gpc1->lock);
}
/*
@@ -337,7 +336,7 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
if (!kvm_gpc_check(gpc2, user_len2)) {
read_unlock(&gpc2->lock);
- read_unlock_irqrestore(&gpc1->lock, flags);
+ read_unlock(&gpc1->lock);
/* When invoked from kvm_sched_out() we cannot sleep */
if (atomic)
@@ -399,7 +398,7 @@ static void kvm_xen_update_runstate_guest(struct kvm_vcpu *v, bool atomic)
if (user_len2)
read_unlock(&gpc2->lock);
done_1:
- read_unlock_irqrestore(&gpc1->lock, flags);
+ read_unlock(&gpc1->lock);
mark_page_dirty_in_slot(v->kvm, gpc1->memslot, gpc1->gpa >> PAGE_SHIFT);
if (user_len2)
I don't know how I feel about that. I resonate with what you say below
about "not strictly required for all users, but it's done for
simplicity". This code doesn't have enough simplicity. Handling the
runstate GPC as a special case just because we can, doesn't necessarily
fill me with joy for the amount of optimisation that it brings.
> > The generic GPC code still needs to disable interrupts when taking a
> > gpc->lock though, unless we want to add yet another flag to control
> > that behaviour.
>
> Right. Might be worth adding a comment at some point to call out that disabling
> IRQs may not be strictly required for all users, but it's done for simplicity.
> Ah, if/when we add kvm_gpc_lock(), that would be the perfect place to document
> the behavior.
Yeah. Or perhaps the kvm_gpc_lock() should go with that 'not required
for all users, but done for simplicity' angle too, and always disable
IRQs?
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
