| Message ID | 20180209143937.28866-27-andre.przywara@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | New VGIC(-v2) implementation | expand |
Hi, On 09/02/18 14:39, Andre Przywara wrote: > Tell Xen whether a particular VCPU has an IRQ that needs handling > in the guest. This is used to decide whether a VCPU is runnable. > > This is based on Linux commit 90eee56c5f90, written by Eric Auger. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > xen/arch/arm/vgic/vgic.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c > index f4f2a04a60..9e7fb1edcb 100644 > --- a/xen/arch/arm/vgic/vgic.c > +++ b/xen/arch/arm/vgic/vgic.c > @@ -646,6 +646,38 @@ void gic_inject(void) > vgic_restore_state(current); > } > > +static int vgic_vcpu_pending_irq(struct vcpu *vcpu) > +{ > + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; > + struct vgic_irq *irq; > + bool pending = false; > + unsigned long flags; > + > + if ( !vcpu->domain->arch.vgic.enabled ) > + return false; > + > + spin_lock_irqsave(&vgic_cpu->ap_list_lock, flags); > + > + list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) > + { > + spin_lock(&irq->irq_lock); > + pending = irq_is_pending(irq) && irq->enabled; > + spin_unlock(&irq->irq_lock); > + > + if ( pending ) > + break; > + } > + > + spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags); > + > + return pending; > +} > + > +int gic_events_need_delivery(void) You probably want to rename that function or just expose vgic_vcpu_pending_irq(). Cheers,
On 13/02/18 16:35, Julien Grall wrote: > Hi, > > On 09/02/18 14:39, Andre Przywara wrote: >> Tell Xen whether a particular VCPU has an IRQ that needs handling >> in the guest. This is used to decide whether a VCPU is runnable. I forgot to mention one thing. This is not the main usage of this function in Xen. That function will mostly be used to check whether we need to preempt an hypercall to run interrupt. Please update the commit message accordingly. >> >> This is based on Linux commit 90eee56c5f90, written by Eric Auger. >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> --- >> xen/arch/arm/vgic/vgic.c | 32 ++++++++++++++++++++++++++++++++ >> 1 file changed, 32 insertions(+) >> >> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c >> index f4f2a04a60..9e7fb1edcb 100644 >> --- a/xen/arch/arm/vgic/vgic.c >> +++ b/xen/arch/arm/vgic/vgic.c >> @@ -646,6 +646,38 @@ void gic_inject(void) >> vgic_restore_state(current); >> } >> +static int vgic_vcpu_pending_irq(struct vcpu *vcpu) >> +{ >> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; >> + struct vgic_irq *irq; >> + bool pending = false; >> + unsigned long flags; >> + >> + if ( !vcpu->domain->arch.vgic.enabled ) >> + return false; >> + >> + spin_lock_irqsave(&vgic_cpu->ap_list_lock, flags); >> + >> + list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) >> + { >> + spin_lock(&irq->irq_lock); >> + pending = irq_is_pending(irq) && irq->enabled; >> + spin_unlock(&irq->irq_lock); >> + >> + if ( pending ) >> + break; >> + } >> + >> + spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags); >> + >> + return pending; >> +} >> + >> +int gic_events_need_delivery(void) > > You probably want to rename that function or just expose > vgic_vcpu_pending_irq(). > > Cheers, >
Hi, On 13/02/18 16:35, Julien Grall wrote: > Hi, > > On 09/02/18 14:39, Andre Przywara wrote: >> Tell Xen whether a particular VCPU has an IRQ that needs handling >> in the guest. This is used to decide whether a VCPU is runnable. >> >> This is based on Linux commit 90eee56c5f90, written by Eric Auger. >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> --- >> xen/arch/arm/vgic/vgic.c | 32 ++++++++++++++++++++++++++++++++ >> 1 file changed, 32 insertions(+) >> >> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c >> index f4f2a04a60..9e7fb1edcb 100644 >> --- a/xen/arch/arm/vgic/vgic.c >> +++ b/xen/arch/arm/vgic/vgic.c >> @@ -646,6 +646,38 @@ void gic_inject(void) >> vgic_restore_state(current); >> } >> +static int vgic_vcpu_pending_irq(struct vcpu *vcpu) >> +{ >> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; >> + struct vgic_irq *irq; >> + bool pending = false; >> + unsigned long flags; >> + >> + if ( !vcpu->domain->arch.vgic.enabled ) >> + return false; >> + >> + spin_lock_irqsave(&vgic_cpu->ap_list_lock, flags); >> + >> + list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) >> + { >> + spin_lock(&irq->irq_lock); >> + pending = irq_is_pending(irq) && irq->enabled; >> + spin_unlock(&irq->irq_lock); >> + >> + if ( pending ) >> + break; >> + } >> + >> + spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags); >> + >> + return pending; >> +} >> + >> +int gic_events_need_delivery(void) > > You probably want to rename that function or just expose > vgic_vcpu_pending_irq(). Rename to what? I need both functions: vgic_vcpu_pending_irq() is also called by vgic_kick_vcpus() (later in the series). And gic_events_need_delivery(void) is the interface that the arch code expects. Shall I rename this there? To what? Cheers, Andre.
Hi, On 02/26/2018 03:29 PM, Andre Przywara wrote: > On 13/02/18 16:35, Julien Grall wrote: >>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c >>> index f4f2a04a60..9e7fb1edcb 100644 >>> --- a/xen/arch/arm/vgic/vgic.c >>> +++ b/xen/arch/arm/vgic/vgic.c >>> @@ -646,6 +646,38 @@ void gic_inject(void) >>> vgic_restore_state(current); >>> } >>> +static int vgic_vcpu_pending_irq(struct vcpu *vcpu) >>> +{ >>> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; >>> + struct vgic_irq *irq; >>> + bool pending = false; >>> + unsigned long flags; >>> + >>> + if ( !vcpu->domain->arch.vgic.enabled ) >>> + return false; >>> + >>> + spin_lock_irqsave(&vgic_cpu->ap_list_lock, flags); >>> + >>> + list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) >>> + { >>> + spin_lock(&irq->irq_lock); >>> + pending = irq_is_pending(irq) && irq->enabled; >>> + spin_unlock(&irq->irq_lock); >>> + >>> + if ( pending ) >>> + break; >>> + } >>> + >>> + spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags); >>> + >>> + return pending; >>> +} >>> + >>> +int gic_events_need_delivery(void) >> >> You probably want to rename that function or just expose >> vgic_vcpu_pending_irq(). > > Rename to what? I need both functions: vgic_vcpu_pending_irq() is also > called by vgic_kick_vcpus() (later in the series). > And gic_events_need_delivery(void) is the interface that the arch code > expects. Shall I rename this there? To what? Let me start with it is a bit odd to have a function name 'gic_*' in the virtual GIC code. So at least renaming to vgic_events_need_delivery would be an improvement. Regarding the interface itself, it is ARM specific and not set in stone. It would not be too bad to use vgic_vcpu_pending_irq(current). Is there any reason for not doing that? Cheers,
Hi, On 26/02/18 15:55, Julien Grall wrote: > Hi, > > On 02/26/2018 03:29 PM, Andre Przywara wrote: >> On 13/02/18 16:35, Julien Grall wrote: >>>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c >>>> index f4f2a04a60..9e7fb1edcb 100644 >>>> --- a/xen/arch/arm/vgic/vgic.c >>>> +++ b/xen/arch/arm/vgic/vgic.c >>>> @@ -646,6 +646,38 @@ void gic_inject(void) >>>> vgic_restore_state(current); >>>> } >>>> +static int vgic_vcpu_pending_irq(struct vcpu *vcpu) >>>> +{ >>>> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; >>>> + struct vgic_irq *irq; >>>> + bool pending = false; >>>> + unsigned long flags; >>>> + >>>> + if ( !vcpu->domain->arch.vgic.enabled ) >>>> + return false; >>>> + >>>> + spin_lock_irqsave(&vgic_cpu->ap_list_lock, flags); >>>> + >>>> + list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) >>>> + { >>>> + spin_lock(&irq->irq_lock); >>>> + pending = irq_is_pending(irq) && irq->enabled; >>>> + spin_unlock(&irq->irq_lock); >>>> + >>>> + if ( pending ) >>>> + break; >>>> + } >>>> + >>>> + spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags); >>>> + >>>> + return pending; >>>> +} >>>> + >>>> +int gic_events_need_delivery(void) >>> >>> You probably want to rename that function or just expose >>> vgic_vcpu_pending_irq(). >> >> Rename to what? I need both functions: vgic_vcpu_pending_irq() is also >> called by vgic_kick_vcpus() (later in the series). >> And gic_events_need_delivery(void) is the interface that the arch code >> expects. Shall I rename this there? To what? > > Let me start with it is a bit odd to have a function name 'gic_*' in the > virtual GIC code. So at least renaming to vgic_events_need_delivery > would be an improvement. > > Regarding the interface itself, it is ARM specific and not set in stone. > It would not be too bad to use vgic_vcpu_pending_irq(current). Is there > any reason for not doing that? Not really, but I am a bit reluctant to change too much original Xen code, don't want to step on anyone's toes ;-) But if that's fine with you, I am OK with the renaming - though it adds yet another patch ;-) Cheers, Andre.
On 02/26/2018 04:25 PM, Andre Przywara wrote: > Hi, > > On 26/02/18 15:55, Julien Grall wrote: >> Hi, >> >> On 02/26/2018 03:29 PM, Andre Przywara wrote: >>> On 13/02/18 16:35, Julien Grall wrote: >>>>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c >>>>> index f4f2a04a60..9e7fb1edcb 100644 >>>>> --- a/xen/arch/arm/vgic/vgic.c >>>>> +++ b/xen/arch/arm/vgic/vgic.c >>>>> @@ -646,6 +646,38 @@ void gic_inject(void) >>>>> vgic_restore_state(current); >>>>> } >>>>> +static int vgic_vcpu_pending_irq(struct vcpu *vcpu) >>>>> +{ >>>>> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; >>>>> + struct vgic_irq *irq; >>>>> + bool pending = false; >>>>> + unsigned long flags; >>>>> + >>>>> + if ( !vcpu->domain->arch.vgic.enabled ) >>>>> + return false; >>>>> + >>>>> + spin_lock_irqsave(&vgic_cpu->ap_list_lock, flags); >>>>> + >>>>> + list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) >>>>> + { >>>>> + spin_lock(&irq->irq_lock); >>>>> + pending = irq_is_pending(irq) && irq->enabled; >>>>> + spin_unlock(&irq->irq_lock); >>>>> + >>>>> + if ( pending ) >>>>> + break; >>>>> + } >>>>> + >>>>> + spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags); >>>>> + >>>>> + return pending; >>>>> +} >>>>> + >>>>> +int gic_events_need_delivery(void) >>>> >>>> You probably want to rename that function or just expose >>>> vgic_vcpu_pending_irq(). >>> >>> Rename to what? I need both functions: vgic_vcpu_pending_irq() is also >>> called by vgic_kick_vcpus() (later in the series). >>> And gic_events_need_delivery(void) is the interface that the arch code >>> expects. Shall I rename this there? To what? >> >> Let me start with it is a bit odd to have a function name 'gic_*' in the >> virtual GIC code. So at least renaming to vgic_events_need_delivery >> would be an improvement. >> >> Regarding the interface itself, it is ARM specific and not set in stone. >> It would not be too bad to use vgic_vcpu_pending_irq(current). Is there >> any reason for not doing that? > > Not really, but I am a bit reluctant to change too much original Xen > code, don't want to step on anyone's toes ;-) The original code is going to get kill at some point. So better use name that makes sense in the new context. It is quite similar to the gic_inject change. > > But if that's fine with you, I am OK with the renaming - though it adds > yet another patch ;-) The end goal is a better world, so the number of patches does not matter here :). If you put them at the beginning, we can merge them right away. Cheers,
Hi, On 26/02/18 16:30, Julien Grall wrote: > > > On 02/26/2018 04:25 PM, Andre Przywara wrote: >> Hi, >> >> On 26/02/18 15:55, Julien Grall wrote: >>> Hi, >>> >>> On 02/26/2018 03:29 PM, Andre Przywara wrote: >>>> On 13/02/18 16:35, Julien Grall wrote: >>>>>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c >>>>>> index f4f2a04a60..9e7fb1edcb 100644 >>>>>> --- a/xen/arch/arm/vgic/vgic.c >>>>>> +++ b/xen/arch/arm/vgic/vgic.c >>>>>> @@ -646,6 +646,38 @@ void gic_inject(void) >>>>>> vgic_restore_state(current); >>>>>> } >>>>>> +static int vgic_vcpu_pending_irq(struct vcpu *vcpu) >>>>>> +{ >>>>>> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; >>>>>> + struct vgic_irq *irq; >>>>>> + bool pending = false; >>>>>> + unsigned long flags; >>>>>> + >>>>>> + if ( !vcpu->domain->arch.vgic.enabled ) >>>>>> + return false; >>>>>> + >>>>>> + spin_lock_irqsave(&vgic_cpu->ap_list_lock, flags); >>>>>> + >>>>>> + list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) >>>>>> + { >>>>>> + spin_lock(&irq->irq_lock); >>>>>> + pending = irq_is_pending(irq) && irq->enabled; >>>>>> + spin_unlock(&irq->irq_lock); >>>>>> + >>>>>> + if ( pending ) >>>>>> + break; >>>>>> + } >>>>>> + >>>>>> + spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags); >>>>>> + >>>>>> + return pending; >>>>>> +} >>>>>> + >>>>>> +int gic_events_need_delivery(void) >>>>> >>>>> You probably want to rename that function or just expose >>>>> vgic_vcpu_pending_irq(). >>>> >>>> Rename to what? I need both functions: vgic_vcpu_pending_irq() is also >>>> called by vgic_kick_vcpus() (later in the series). >>>> And gic_events_need_delivery(void) is the interface that the arch code >>>> expects. Shall I rename this there? To what? >>> >>> Let me start with it is a bit odd to have a function name 'gic_*' in the >>> virtual GIC code. So at least renaming to vgic_events_need_delivery >>> would be an improvement. >>> >>> Regarding the interface itself, it is ARM specific and not set in stone. >>> It would not be too bad to use vgic_vcpu_pending_irq(current). Is there >>> any reason for not doing that? The two interfaces used for that purpose are different in the two VGICs: - The old VGIC only works on the current VCPU, since it peeks into the GICH_ register to learn the priority (regardless of whether this is really needed or useful). - The new VGIC can use this function for any VCPU, and we need this functionality later on (when we iterate over all VCPUs). So we can't use a function hardwiring "current", that would break vgic_kick_vcpus() in the new VGIC. And we can't pass a VCPU parameter, that would not work for the old VGIC. So I believe having this small wrapper here is the easiest solution. I will add a patch to rename this function to vgic_pending_irq(), though, so this one here looks like: int vgic_pending_irq(void) { return vgic_vcpu_pending_irq(current); } We can clean this up when the old VGIC gets removed. Cheers, Andre. >> Not really, but I am a bit reluctant to change too much original Xen >> code, don't want to step on anyone's toes ;-) > > The original code is going to get kill at some point. So better use name > that makes sense in the new context. It is quite similar to the > gic_inject change. > >> >> But if that's fine with you, I am OK with the renaming - though it adds >> yet another patch ;-) > > The end goal is a better world, so the number of patches does not matter > here :). > > If you put them at the beginning, we can merge them right away. > > Cheers, >
On 02/03/18 13:53, Andre Przywara wrote: > Hi, Hi Andre, > On 26/02/18 16:30, Julien Grall wrote: >> >> >> On 02/26/2018 04:25 PM, Andre Przywara wrote: >>> Hi, >>> >>> On 26/02/18 15:55, Julien Grall wrote: >>>> Hi, >>>> >>>> On 02/26/2018 03:29 PM, Andre Przywara wrote: >>>>> On 13/02/18 16:35, Julien Grall wrote: >>>>>>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c >>>>>>> index f4f2a04a60..9e7fb1edcb 100644 >>>>>>> --- a/xen/arch/arm/vgic/vgic.c >>>>>>> +++ b/xen/arch/arm/vgic/vgic.c >>>>>>> @@ -646,6 +646,38 @@ void gic_inject(void) >>>>>>> vgic_restore_state(current); >>>>>>> } >>>>>>> +static int vgic_vcpu_pending_irq(struct vcpu *vcpu) >>>>>>> +{ >>>>>>> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; >>>>>>> + struct vgic_irq *irq; >>>>>>> + bool pending = false; >>>>>>> + unsigned long flags; >>>>>>> + >>>>>>> + if ( !vcpu->domain->arch.vgic.enabled ) >>>>>>> + return false; >>>>>>> + >>>>>>> + spin_lock_irqsave(&vgic_cpu->ap_list_lock, flags); >>>>>>> + >>>>>>> + list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) >>>>>>> + { >>>>>>> + spin_lock(&irq->irq_lock); >>>>>>> + pending = irq_is_pending(irq) && irq->enabled; >>>>>>> + spin_unlock(&irq->irq_lock); >>>>>>> + >>>>>>> + if ( pending ) >>>>>>> + break; >>>>>>> + } >>>>>>> + >>>>>>> + spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags); >>>>>>> + >>>>>>> + return pending; >>>>>>> +} >>>>>>> + >>>>>>> +int gic_events_need_delivery(void) >>>>>> >>>>>> You probably want to rename that function or just expose >>>>>> vgic_vcpu_pending_irq(). >>>>> >>>>> Rename to what? I need both functions: vgic_vcpu_pending_irq() is also >>>>> called by vgic_kick_vcpus() (later in the series). >>>>> And gic_events_need_delivery(void) is the interface that the arch code >>>>> expects. Shall I rename this there? To what? >>>> >>>> Let me start with it is a bit odd to have a function name 'gic_*' in the >>>> virtual GIC code. So at least renaming to vgic_events_need_delivery >>>> would be an improvement. >>>> >>>> Regarding the interface itself, it is ARM specific and not set in stone. >>>> It would not be too bad to use vgic_vcpu_pending_irq(current). Is there >>>> any reason for not doing that? > > The two interfaces used for that purpose are different in the two VGICs: > - The old VGIC only works on the current VCPU, since it peeks into the > GICH_ register to learn the priority (regardless of whether this is > really needed or useful). > - The new VGIC can use this function for any VCPU, and we need this > functionality later on (when we iterate over all VCPUs). > So we can't use a function hardwiring "current", that would break > vgic_kick_vcpus() in the new VGIC. And we can't pass a VCPU parameter, > that would not work for the old VGIC. > So I believe having this small wrapper here is the easiest solution. > I will add a patch to rename this function to vgic_pending_irq(), > though, so this one here looks like: > int vgic_pending_irq(void) > { > return vgic_vcpu_pending_irq(current); > } > > We can clean this up when the old VGIC gets removed. Likely no-one in the old vGIC are going to call that function with v != current. This would not be the only place in Xen where a vCPU is taken in parameter but effectively v can only be current. That's where ASSERT(v == current) comes into place. Cheers,
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c index f4f2a04a60..9e7fb1edcb 100644 --- a/xen/arch/arm/vgic/vgic.c +++ b/xen/arch/arm/vgic/vgic.c @@ -646,6 +646,38 @@ void gic_inject(void) vgic_restore_state(current); } +static int vgic_vcpu_pending_irq(struct vcpu *vcpu) +{ + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; + struct vgic_irq *irq; + bool pending = false; + unsigned long flags; + + if ( !vcpu->domain->arch.vgic.enabled ) + return false; + + spin_lock_irqsave(&vgic_cpu->ap_list_lock, flags); + + list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list) + { + spin_lock(&irq->irq_lock); + pending = irq_is_pending(irq) && irq->enabled; + spin_unlock(&irq->irq_lock); + + if ( pending ) + break; + } + + spin_unlock_irqrestore(&vgic_cpu->ap_list_lock, flags); + + return pending; +} + +int gic_events_need_delivery(void) +{ + return vgic_vcpu_pending_irq(current); +} + /* * Local variables: * mode: C
Tell Xen whether a particular VCPU has an IRQ that needs handling in the guest. This is used to decide whether a VCPU is runnable. This is based on Linux commit 90eee56c5f90, written by Eric Auger. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- xen/arch/arm/vgic/vgic.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)