| Message ID | 20180305160415.16760-29-andre.przywara@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | New VGIC(-v2) implementation | expand |
Hi Andre, On 05/03/18 16:03, Andre Przywara wrote: > The new VGIC implementation centers around a struct vgic_irq instance > per virtual IRQ. > Provide a function to retrieve the right instance for a given IRQ > number and (in case of private interrupts) the right VCPU. > This also includes the corresponding put function, which does nothing > for private interrupts and SPIs, but handles the ref-counting for LPIs. > > This is based on Linux commit 64a959d66e47, written by Christoffer Dall. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > Changelog RFC ... v1: > - add kernel-doc comments to exported functions > - adapt to previous changes (new_vgic.h, arch_vcpu member name) > - use ASSERT_UNREACHABLE > > xen/arch/arm/vgic/vgic.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++ > xen/arch/arm/vgic/vgic.h | 41 ++++++++++++++++ > 2 files changed, 165 insertions(+) > create mode 100644 xen/arch/arm/vgic/vgic.c > create mode 100644 xen/arch/arm/vgic/vgic.h > > diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c > new file mode 100644 > index 0000000000..ace30f78d0 > --- /dev/null > +++ b/xen/arch/arm/vgic/vgic.c > @@ -0,0 +1,124 @@ > +/* > + * Copyright (C) 2015, 2016 ARM Ltd. > + * Imported from Linux ("new" KVM VGIC) and heavily adapted to Xen. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <asm/bug.h> > +#include <asm/new_vgic.h> > +#include <xen/sched.h> > + > +#include "vgic.h" > + > +/* > + * Iterate over the VM's list of mapped LPIs to find the one with a > + * matching interrupt ID and return a reference to the IRQ structure. > + */ > +static struct vgic_irq *vgic_get_lpi(struct domain *d, u32 intid) > +{ > + struct vgic_dist *dist = &d->arch.vgic; > + struct vgic_irq *irq = NULL; > + > + spin_lock(&dist->lpi_list_lock); > + > + list_for_each_entry( irq, &dist->lpi_list_head, lpi_list ) I am still not a big fan of the list solution. Strictly speaking nobody is populating that list and likely going to be too slow in Xen case (I am thinking for the hardware domain). So I think I would prefer to see the LPI related code disappear for this cut. This could easily be added back as they are standalone. > + { > + if ( irq->intid != intid ) > + continue; > + > + /* > + * This increases the refcount, the caller is expected to > + * call vgic_put_irq() later once it's finished with the IRQ. > + */ > + vgic_get_irq_kref(irq); > + goto out_unlock; > + } > + irq = NULL; > + > +out_unlock: > + spin_unlock(&dist->lpi_list_lock); > + > + return irq; > +} > + > +/** > + * vgic_get_irq() - obtain a reference to a virtual IRQ > + * @d: The domain the virtual IRQ belongs to. > + * @vcpu: For private IRQs (SGIs, PPIs) the virtual CPU this IRQ > + * is associated with. Will be ignored for SPIs and LPIs. > + * @intid: The virtual IRQ number. > + * > + * This looks up the virtual interrupt ID to get the corresponding > + * struct vgic_irq. It also increases the refcount, so any caller is expected > + * to call vgic_put_irq() once it's finished with this IRQ. > + * > + * Return: The pointer to the requested struct vgic_irq. > + */ > +struct vgic_irq *vgic_get_irq(struct domain *d, struct vcpu *vcpu, > + u32 intid) > +{ > + /* SGIs and PPIs */ > + if ( intid <= VGIC_MAX_PRIVATE ) > + return &vcpu->arch.vgic.private_irqs[intid]; > + > + /* SPIs */ > + if ( intid <= VGIC_MAX_SPI ) > + return &d->arch.vgic.spis[intid - VGIC_NR_PRIVATE_IRQS]; > + > + /* LPIs */ > + if ( intid >= VGIC_MIN_LPI ) > + return vgic_get_lpi(d, intid); > + > + ASSERT_UNREACHABLE(); You need to keep the return NULL here as ASSERT_UNREACHABLE() is turned to a nop on non-debug build. My point with ASSERT is to avoid potential flaw in the code and a way to flood Xen console with the WARN in non-debug build. > +} > + > +/** > + * vgic_put_irq() - drop the reference to a virtual IRQ > + * @d: The domain the virtual IRQ belongs to. > + * @irq: The pointer to struct vgic_irq, as obtained from vgic_get_irq(). > + * > + * This drops the reference to a virtual IRQ. It decreases the refcount > + * of the pointer, so dynamic IRQs can be freed when no longer needed. > + * This should always be called after a vgic_get_irq(), though the reference > + * can be deliberately held for longer periods, if needed. > + */ > +void vgic_put_irq(struct domain *d, struct vgic_irq *irq) > +{ > + struct vgic_dist *dist = &d->arch.vgic; > + > + if ( irq->intid < VGIC_MIN_LPI ) > + return; > + > + spin_lock(&dist->lpi_list_lock); > + if ( !atomic_dec_and_test(&irq->refcount) ) > + { > + spin_unlock(&dist->lpi_list_lock); > + return; > + }; > + > + list_del(&irq->lpi_list); > + dist->lpi_list_count--; > + spin_unlock(&dist->lpi_list_lock); > + > + xfree(irq); > +} > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */ > diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h > new file mode 100644 > index 0000000000..a3befd386b > --- /dev/null > +++ b/xen/arch/arm/vgic/vgic.h > @@ -0,0 +1,41 @@ > +/* > + * Copyright (C) 2015, 2016 ARM Ltd. > + * Imported from Linux ("new" KVM VGIC) and heavily adapted to Xen. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > +#ifndef __XEN_ARM_VGIC_VGIC_H__ > +#define __XEN_ARM_VGIC_VGIC_H__ > + > +struct vgic_irq *vgic_get_irq(struct domain *d, struct vcpu *vcpu, > + u32 intid); > +void vgic_put_irq(struct domain *d, struct vgic_irq *irq); > + > +static inline void vgic_get_irq_kref(struct vgic_irq *irq) > +{ > + if ( irq->intid < VGIC_MIN_LPI ) > + return; > + > + atomic_inc(&irq->refcount); > +} > + > +#endif > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */ > Cheers,
Hi, On 06/03/18 18:13, Julien Grall wrote: > Hi Andre, > > On 05/03/18 16:03, Andre Przywara wrote: >> The new VGIC implementation centers around a struct vgic_irq instance >> per virtual IRQ. >> Provide a function to retrieve the right instance for a given IRQ >> number and (in case of private interrupts) the right VCPU. >> This also includes the corresponding put function, which does nothing >> for private interrupts and SPIs, but handles the ref-counting for LPIs. >> >> This is based on Linux commit 64a959d66e47, written by Christoffer Dall. >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> --- >> Changelog RFC ... v1: >> - add kernel-doc comments to exported functions >> - adapt to previous changes (new_vgic.h, arch_vcpu member name) >> - use ASSERT_UNREACHABLE >> >> xen/arch/arm/vgic/vgic.c | 124 >> +++++++++++++++++++++++++++++++++++++++++++++++ >> xen/arch/arm/vgic/vgic.h | 41 ++++++++++++++++ >> 2 files changed, 165 insertions(+) >> create mode 100644 xen/arch/arm/vgic/vgic.c >> create mode 100644 xen/arch/arm/vgic/vgic.h >> >> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c >> new file mode 100644 >> index 0000000000..ace30f78d0 >> --- /dev/null >> +++ b/xen/arch/arm/vgic/vgic.c >> @@ -0,0 +1,124 @@ >> +/* >> + * Copyright (C) 2015, 2016 ARM Ltd. >> + * Imported from Linux ("new" KVM VGIC) and heavily adapted to Xen. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program. If not, see <http://www.gnu.org/licenses/>. >> + */ >> + >> +#include <asm/bug.h> >> +#include <asm/new_vgic.h> >> +#include <xen/sched.h> >> + >> +#include "vgic.h" >> + >> +/* >> + * Iterate over the VM's list of mapped LPIs to find the one with a >> + * matching interrupt ID and return a reference to the IRQ structure. >> + */ >> +static struct vgic_irq *vgic_get_lpi(struct domain *d, u32 intid) >> +{ >> + struct vgic_dist *dist = &d->arch.vgic; >> + struct vgic_irq *irq = NULL; >> + >> + spin_lock(&dist->lpi_list_lock); >> + >> + list_for_each_entry( irq, &dist->lpi_list_head, lpi_list ) > > I am still not a big fan of the list solution. Strictly speaking nobody > is populating that list and likely going to be too slow in Xen case (I > am thinking for the hardware domain). So I think I would prefer to see > the LPI related code disappear for this cut. This could easily be added > back as they are standalone. I was thinking about that, but dismissed the idea: Considering LPIs as first class citizens is a crucial property of the new VGIC and actually the main driver for its creation: the refcounting is solely in for that purpose, and the per-IRQ data structure and its lock are mainly driven by it. So removing the LPI support completely will make the refcounting look very awkward (since useless and unused). Consequently one would need to remove that as well. In the worst case we run into issues with unused functions, like vgic_get_irq_kref(). I already removed a lot of code from the KVM VGIC, which I feel we will need back one day. So I really like to keep this one in, and be it just for documentation how the refcounting is supposed to be used. I will add a comment to this respect. And I believe replacing the list_for_each_entry() with something more sophisticated is the least of our problems later on. Cheers, Andre.
On 03/19/2018 05:32 PM, Andre Przywara wrote: > Hi, Hi, > On 06/03/18 18:13, Julien Grall wrote: >> Hi Andre, >> >> On 05/03/18 16:03, Andre Przywara wrote: >>> The new VGIC implementation centers around a struct vgic_irq instance >>> per virtual IRQ. >>> Provide a function to retrieve the right instance for a given IRQ >>> number and (in case of private interrupts) the right VCPU. >>> This also includes the corresponding put function, which does nothing >>> for private interrupts and SPIs, but handles the ref-counting for LPIs. >>> >>> This is based on Linux commit 64a959d66e47, written by Christoffer Dall. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >>> --- >>> Changelog RFC ... v1: >>> - add kernel-doc comments to exported functions >>> - adapt to previous changes (new_vgic.h, arch_vcpu member name) >>> - use ASSERT_UNREACHABLE >>> >>> xen/arch/arm/vgic/vgic.c | 124 >>> +++++++++++++++++++++++++++++++++++++++++++++++ >>> xen/arch/arm/vgic/vgic.h | 41 ++++++++++++++++ >>> 2 files changed, 165 insertions(+) >>> create mode 100644 xen/arch/arm/vgic/vgic.c >>> create mode 100644 xen/arch/arm/vgic/vgic.h >>> >>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c >>> new file mode 100644 >>> index 0000000000..ace30f78d0 >>> --- /dev/null >>> +++ b/xen/arch/arm/vgic/vgic.c >>> @@ -0,0 +1,124 @@ >>> +/* >>> + * Copyright (C) 2015, 2016 ARM Ltd. >>> + * Imported from Linux ("new" KVM VGIC) and heavily adapted to Xen. >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License version 2 as >>> + * published by the Free Software Foundation. >>> + * >>> + * This program is distributed in the hope that it will be useful, >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>> + * GNU General Public License for more details. >>> + * >>> + * You should have received a copy of the GNU General Public License >>> + * along with this program. If not, see <http://www.gnu.org/licenses/>. >>> + */ >>> + >>> +#include <asm/bug.h> >>> +#include <asm/new_vgic.h> >>> +#include <xen/sched.h> >>> + >>> +#include "vgic.h" >>> + >>> +/* >>> + * Iterate over the VM's list of mapped LPIs to find the one with a >>> + * matching interrupt ID and return a reference to the IRQ structure. >>> + */ >>> +static struct vgic_irq *vgic_get_lpi(struct domain *d, u32 intid) >>> +{ >>> + struct vgic_dist *dist = &d->arch.vgic; >>> + struct vgic_irq *irq = NULL; >>> + >>> + spin_lock(&dist->lpi_list_lock); >>> + >>> + list_for_each_entry( irq, &dist->lpi_list_head, lpi_list ) >> >> I am still not a big fan of the list solution. Strictly speaking nobody >> is populating that list and likely going to be too slow in Xen case (I >> am thinking for the hardware domain). So I think I would prefer to see >> the LPI related code disappear for this cut. This could easily be added >> back as they are standalone. > > I was thinking about that, but dismissed the idea: > Considering LPIs as first class citizens is a crucial property of the > new VGIC and actually the main driver for its creation: the refcounting > is solely in for that purpose, and the per-IRQ data structure and its > lock are mainly driven by it. So removing the LPI support completely > will make the refcounting look very awkward (since useless and unused). > Consequently one would need to remove that as well. In the worst case we > run into issues with unused functions, like vgic_get_irq_kref(). > > I already removed a lot of code from the KVM VGIC, which I feel we will > need back one day. So I really like to keep this one in, and be it just > for documentation how the refcounting is supposed to be used. > I will add a comment to this respect. > And I believe replacing the list_for_each_entry() with something more > sophisticated is the least of our problems later on. Using a list for LPIs means the search is O(n). That search will be done each time you want to get the LPI information. While in guest you may have limited LPIs, this will not be the case for the hardware domain. Indeed that domain has access to all devices and likely going to use a lot of MSIs. So I still can't understand why you dismiss that problem. To be honest, I don't mind to keep the refcounting around. What I don't want to see in Xen is using a data structure that we know will not fit Xen (even for well-behave guests). You may remove a lot of code, and re-add the code later but that's the price to convert KVM to Xen code. It makes much more sense to get all the LPIs infrastructure together. And to be clear, what I am asking for is implementing vgic_get_lpi with return NULL + a comment. Cheers,
Hi, On 19/03/18 21:53, Julien Grall wrote: > > > On 03/19/2018 05:32 PM, Andre Przywara wrote: >> Hi, > > Hi, > >> On 06/03/18 18:13, Julien Grall wrote: >>> Hi Andre, >>> >>> On 05/03/18 16:03, Andre Przywara wrote: >>>> The new VGIC implementation centers around a struct vgic_irq instance >>>> per virtual IRQ. >>>> Provide a function to retrieve the right instance for a given IRQ >>>> number and (in case of private interrupts) the right VCPU. >>>> This also includes the corresponding put function, which does nothing >>>> for private interrupts and SPIs, but handles the ref-counting for LPIs. >>>> >>>> This is based on Linux commit 64a959d66e47, written by Christoffer >>>> Dall. >>>> >>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >>>> --- >>>> Changelog RFC ... v1: >>>> - add kernel-doc comments to exported functions >>>> - adapt to previous changes (new_vgic.h, arch_vcpu member name) >>>> - use ASSERT_UNREACHABLE >>>> >>>> xen/arch/arm/vgic/vgic.c | 124 >>>> +++++++++++++++++++++++++++++++++++++++++++++++ >>>> xen/arch/arm/vgic/vgic.h | 41 ++++++++++++++++ >>>> 2 files changed, 165 insertions(+) >>>> create mode 100644 xen/arch/arm/vgic/vgic.c >>>> create mode 100644 xen/arch/arm/vgic/vgic.h >>>> >>>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c >>>> new file mode 100644 >>>> index 0000000000..ace30f78d0 >>>> --- /dev/null >>>> +++ b/xen/arch/arm/vgic/vgic.c >>>> @@ -0,0 +1,124 @@ >>>> +/* >>>> + * Copyright (C) 2015, 2016 ARM Ltd. >>>> + * Imported from Linux ("new" KVM VGIC) and heavily adapted to Xen. >>>> + * >>>> + * This program is free software; you can redistribute it and/or >>>> modify >>>> + * it under the terms of the GNU General Public License version 2 as >>>> + * published by the Free Software Foundation. >>>> + * >>>> + * This program is distributed in the hope that it will be useful, >>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>> + * GNU General Public License for more details. >>>> + * >>>> + * You should have received a copy of the GNU General Public License >>>> + * along with this program. If not, see >>>> <http://www.gnu.org/licenses/>. >>>> + */ >>>> + >>>> +#include <asm/bug.h> >>>> +#include <asm/new_vgic.h> >>>> +#include <xen/sched.h> >>>> + >>>> +#include "vgic.h" >>>> + >>>> +/* >>>> + * Iterate over the VM's list of mapped LPIs to find the one with a >>>> + * matching interrupt ID and return a reference to the IRQ structure. >>>> + */ >>>> +static struct vgic_irq *vgic_get_lpi(struct domain *d, u32 intid) >>>> +{ >>>> + struct vgic_dist *dist = &d->arch.vgic; >>>> + struct vgic_irq *irq = NULL; >>>> + >>>> + spin_lock(&dist->lpi_list_lock); >>>> + >>>> + list_for_each_entry( irq, &dist->lpi_list_head, lpi_list ) >>> >>> I am still not a big fan of the list solution. Strictly speaking nobody >>> is populating that list and likely going to be too slow in Xen case (I >>> am thinking for the hardware domain). So I think I would prefer to see >>> the LPI related code disappear for this cut. This could easily be added >>> back as they are standalone. >> >> I was thinking about that, but dismissed the idea: >> Considering LPIs as first class citizens is a crucial property of the >> new VGIC and actually the main driver for its creation: the refcounting >> is solely in for that purpose, and the per-IRQ data structure and its >> lock are mainly driven by it. So removing the LPI support completely >> will make the refcounting look very awkward (since useless and unused). >> Consequently one would need to remove that as well. In the worst case we >> run into issues with unused functions, like vgic_get_irq_kref(). >> >> I already removed a lot of code from the KVM VGIC, which I feel we will >> need back one day. So I really like to keep this one in, and be it just >> for documentation how the refcounting is supposed to be used. >> I will add a comment to this respect. >> And I believe replacing the list_for_each_entry() with something more >> sophisticated is the least of our problems later on. > > Using a list for LPIs means the search is O(n). That search will be done > each time you want to get the LPI information. > > While in guest you may have limited LPIs, this will not be the case for > the hardware domain. Indeed that domain has access to all devices and > likely going to use a lot of MSIs. > > So I still can't understand why you dismiss that problem. I think you got me wrong. I am happy with eventually using another data structure, but want to keep the current list stub in as documentation, as it motivates a crucial parts of the VGIC, namely refcounting and the possible dynamic allocation of struct vgic_irq's. If that would be left out, anyone reading the code would seriously scratch their head why we have all this code, possibly removing it. Actually the list we are talking about consists of a root pointer and this single "list_for_each_entry()" call in vgic_get_lpi(). That's it. There is no code to populate the list at the moment. So I consider this more *documentation* on how it's supposed to be done, especially the refcounting, allocation, and where to initialise things. And as I mentioned before, a list is a good example, because it's easy to understand: You immediately understand what list_for_each_entry() means, and the code shows where to take and drop the locks and how to refcount. Whoever implements LPI support, can just grep for lpi_list_head and replace the respective code with something more appropriate. I added comments to that regard. > To be honest, I don't mind to keep the refcounting around. What I don't > want to see in Xen is using a data structure that we know will not fit > Xen (even for well-behave guests). As said, no one is really using this data structure ;-) But there is good code to keep in: the root pointer in the struct, where the data structure should be initialized and freed, how the refcounting is supposed to be done and why we have vgic_put_irq() and what it should do. > You may remove a lot of code, and re-add the code later but that's the > price to convert KVM to Xen code. It makes much more sense to get all > the LPIs infrastructure together. Re-adding code is not trivial, due to the changes required to make it work in Xen. For instance I ported the refcounting to Xen. So I prefer to not throw away the work I already did. > And to be clear, what I am asking for is implementing vgic_get_lpi with > return NULL + a comment. It does that already: lpi_list_head is always empty, so it will skip the loop and eventually return NULL. This also tells the reader how to handle non-allocated LPIs. I am happy to add a comment saying that a list is not the right data structure. Cheers, Andre.
Hi, Sorry for the formatting. On 20 Mar 2018 19:01, "Andre Przywara" <andre.przywara@linaro.org> wrote: I am happy to add a comment saying that a list is not the right data structure. Fair enough. I won't argue more, let's keep the list and add a comment. I do want to see that series merged in Xen 4.11. Cheers, Cheers, Andre. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel <div dir="auto"><div><br><div class="gmail_extra">Hi,</div><div class="gmail_extra" dir="auto"><br></div><div class="gmail_extra" dir="auto">Sorry for the formatting.</div><div class="gmail_extra" dir="auto"><br></div><div class="gmail_extra" dir="auto"><div class="gmail_quote" dir="auto">On 20 Mar 2018 19:01, "Andre Przywara" <<a href="mailto:andre.przywara@linaro.org">andre.przywara@linaro.org</a>> wrote:<blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> I am happy to add a comment saying that a list is not the right data<br> structure.<br></blockquote></div></div></div><div dir="auto"><br></div><div dir="auto">Fair enough. I won't argue more, let's keep the list and add a comment. I do want to see that series merged in Xen 4.11.</div><div dir="auto"><br></div><div dir="auto">Cheers,</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> Cheers,<br> Andre.<br> <div class="elided-text"><br> ______________________________<wbr>_________________<br> Xen-devel mailing list<br> <a href="mailto:Xen-devel@lists.xenproject.org">Xen-devel@lists.xenproject.org</a><br> <a href="https://lists.xenproject.org/mailman/listinfo/xen-devel" rel="noreferrer" target="_blank">https://lists.xenproject.org/<wbr>mailman/listinfo/xen-devel</a></div></blockquote></div><br></div></div></div>
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c new file mode 100644 index 0000000000..ace30f78d0 --- /dev/null +++ b/xen/arch/arm/vgic/vgic.c @@ -0,0 +1,124 @@ +/* + * Copyright (C) 2015, 2016 ARM Ltd. + * Imported from Linux ("new" KVM VGIC) and heavily adapted to Xen. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <asm/bug.h> +#include <asm/new_vgic.h> +#include <xen/sched.h> + +#include "vgic.h" + +/* + * Iterate over the VM's list of mapped LPIs to find the one with a + * matching interrupt ID and return a reference to the IRQ structure. + */ +static struct vgic_irq *vgic_get_lpi(struct domain *d, u32 intid) +{ + struct vgic_dist *dist = &d->arch.vgic; + struct vgic_irq *irq = NULL; + + spin_lock(&dist->lpi_list_lock); + + list_for_each_entry( irq, &dist->lpi_list_head, lpi_list ) + { + if ( irq->intid != intid ) + continue; + + /* + * This increases the refcount, the caller is expected to + * call vgic_put_irq() later once it's finished with the IRQ. + */ + vgic_get_irq_kref(irq); + goto out_unlock; + } + irq = NULL; + +out_unlock: + spin_unlock(&dist->lpi_list_lock); + + return irq; +} + +/** + * vgic_get_irq() - obtain a reference to a virtual IRQ + * @d: The domain the virtual IRQ belongs to. + * @vcpu: For private IRQs (SGIs, PPIs) the virtual CPU this IRQ + * is associated with. Will be ignored for SPIs and LPIs. + * @intid: The virtual IRQ number. + * + * This looks up the virtual interrupt ID to get the corresponding + * struct vgic_irq. It also increases the refcount, so any caller is expected + * to call vgic_put_irq() once it's finished with this IRQ. + * + * Return: The pointer to the requested struct vgic_irq. + */ +struct vgic_irq *vgic_get_irq(struct domain *d, struct vcpu *vcpu, + u32 intid) +{ + /* SGIs and PPIs */ + if ( intid <= VGIC_MAX_PRIVATE ) + return &vcpu->arch.vgic.private_irqs[intid]; + + /* SPIs */ + if ( intid <= VGIC_MAX_SPI ) + return &d->arch.vgic.spis[intid - VGIC_NR_PRIVATE_IRQS]; + + /* LPIs */ + if ( intid >= VGIC_MIN_LPI ) + return vgic_get_lpi(d, intid); + + ASSERT_UNREACHABLE(); +} + +/** + * vgic_put_irq() - drop the reference to a virtual IRQ + * @d: The domain the virtual IRQ belongs to. + * @irq: The pointer to struct vgic_irq, as obtained from vgic_get_irq(). + * + * This drops the reference to a virtual IRQ. It decreases the refcount + * of the pointer, so dynamic IRQs can be freed when no longer needed. + * This should always be called after a vgic_get_irq(), though the reference + * can be deliberately held for longer periods, if needed. + */ +void vgic_put_irq(struct domain *d, struct vgic_irq *irq) +{ + struct vgic_dist *dist = &d->arch.vgic; + + if ( irq->intid < VGIC_MIN_LPI ) + return; + + spin_lock(&dist->lpi_list_lock); + if ( !atomic_dec_and_test(&irq->refcount) ) + { + spin_unlock(&dist->lpi_list_lock); + return; + }; + + list_del(&irq->lpi_list); + dist->lpi_list_count--; + spin_unlock(&dist->lpi_list_lock); + + xfree(irq); +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h new file mode 100644 index 0000000000..a3befd386b --- /dev/null +++ b/xen/arch/arm/vgic/vgic.h @@ -0,0 +1,41 @@ +/* + * Copyright (C) 2015, 2016 ARM Ltd. + * Imported from Linux ("new" KVM VGIC) and heavily adapted to Xen. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ +#ifndef __XEN_ARM_VGIC_VGIC_H__ +#define __XEN_ARM_VGIC_VGIC_H__ + +struct vgic_irq *vgic_get_irq(struct domain *d, struct vcpu *vcpu, + u32 intid); +void vgic_put_irq(struct domain *d, struct vgic_irq *irq); + +static inline void vgic_get_irq_kref(struct vgic_irq *irq) +{ + if ( irq->intid < VGIC_MIN_LPI ) + return; + + atomic_inc(&irq->refcount); +} + +#endif + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */
The new VGIC implementation centers around a struct vgic_irq instance per virtual IRQ. Provide a function to retrieve the right instance for a given IRQ number and (in case of private interrupts) the right VCPU. This also includes the corresponding put function, which does nothing for private interrupts and SPIs, but handles the ref-counting for LPIs. This is based on Linux commit 64a959d66e47, written by Christoffer Dall. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- Changelog RFC ... v1: - add kernel-doc comments to exported functions - adapt to previous changes (new_vgic.h, arch_vcpu member name) - use ASSERT_UNREACHABLE xen/arch/arm/vgic/vgic.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++ xen/arch/arm/vgic/vgic.h | 41 ++++++++++++++++ 2 files changed, 165 insertions(+) create mode 100644 xen/arch/arm/vgic/vgic.c create mode 100644 xen/arch/arm/vgic/vgic.h