| Message ID | 20180209143937.28866-39-andre.przywara@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | New VGIC(-v2) implementation | expand |
Hi, On 09/02/18 14:39, Andre Przywara wrote: > The VGIC supports virtual IRQs to be connected to a hardware IRQ, so > when a guest EOIs the virtual interrupt, it affects the state of that > corresponding interrupt on the hardware side at the same time. > Implement the interface that the Xen arch/core code expects to connect > the virtual and the physical world. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > xen/arch/arm/vgic/vgic.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 63 insertions(+) > > diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c > index dc5e011fa3..8d5260a7db 100644 > --- a/xen/arch/arm/vgic/vgic.c > +++ b/xen/arch/arm/vgic/vgic.c > @@ -693,6 +693,69 @@ void vgic_kick_vcpus(struct domain *d) > } > } > > +struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu *v, > + unsigned int virq) > +{ > + struct irq_desc *desc = NULL; > + struct vgic_irq *irq = vgic_get_irq(d, v, virq); > + unsigned long flags; > + > + if ( !irq ) > + return NULL; > + > + spin_lock_irqsave(&irq->irq_lock, flags); > + if ( irq->hw ) > + desc = irq_to_desc(irq->hwintid); This is not going to work well for PPIs. We should consider to add at least an ASSERT(...) in the code to prevent bad use of it. > + spin_unlock_irqrestore(&irq->irq_lock, flags); > + > + vgic_put_irq(d, irq); > + > + return desc; > +} > + > +/* > + * was: > + * int kvm_vgic_map_phys_irq(struct vcpu *vcpu, u32 virt_irq, u32 phys_irq) > + * int kvm_vgic_unmap_phys_irq(struct vcpu *vcpu, unsigned int virt_irq) > + */ > +int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu, > + unsigned int virt_irq, struct irq_desc *desc, > + bool connect) Indentation. > +{ > + struct vgic_irq *irq = vgic_get_irq(d, vcpu, virt_irq); > + unsigned long flags; > + int ret = 0; > + > + if ( !irq ) > + return -EINVAL; > + > + spin_lock_irqsave(&irq->irq_lock, flags); > + > + if ( connect ) /* assign a mapped IRQ */ > + { > + /* The VIRQ should not be already enabled by the guest */ > + if ( !irq->hw && !irq->enabled ) > + { > + irq->hw = true; > + irq->hwintid = desc->irq; > + } > + else > + { > + ret = -EBUSY; > + } I know that it should not matter for SPIs today. But aren't you meant to get a reference on that interrupt if you connect it? > + } > + else /* remove a mapped IRQ */ > + { > + irq->hw = false; > + irq->hwintid = 0; Here you blindly remove the interrupt without been sure it was the correct physical one. We should have a check like in the current vGIC version. > + } > + > + spin_unlock_irqrestore(&irq->irq_lock, flags); > + vgic_put_irq(d, irq); > + > + return ret; > +} > + > /* > * Local variables: > * mode: C > Cheers,
Hi, On 19/02/18 12:19, Julien Grall wrote: > Hi, > > On 09/02/18 14:39, Andre Przywara wrote: >> The VGIC supports virtual IRQs to be connected to a hardware IRQ, so >> when a guest EOIs the virtual interrupt, it affects the state of that >> corresponding interrupt on the hardware side at the same time. >> Implement the interface that the Xen arch/core code expects to connect >> the virtual and the physical world. >> >> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >> --- >> xen/arch/arm/vgic/vgic.c | 63 >> ++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 63 insertions(+) >> >> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c >> index dc5e011fa3..8d5260a7db 100644 >> --- a/xen/arch/arm/vgic/vgic.c >> +++ b/xen/arch/arm/vgic/vgic.c >> @@ -693,6 +693,69 @@ void vgic_kick_vcpus(struct domain *d) >> } >> } >> +struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu >> *v, >> + unsigned int virq) >> +{ >> + struct irq_desc *desc = NULL; >> + struct vgic_irq *irq = vgic_get_irq(d, v, virq); >> + unsigned long flags; >> + >> + if ( !irq ) >> + return NULL; >> + >> + spin_lock_irqsave(&irq->irq_lock, flags); >> + if ( irq->hw ) >> + desc = irq_to_desc(irq->hwintid); > > This is not going to work well for PPIs. We should consider to add at > least an ASSERT(...) in the code to prevent bad use of it. Yeah, done. But I wonder if we eventually should extend the irq_to_desc() function to take the vCPU, since we will need it anyway once we use hardware mapped timer IRQs (PPIs) in the future. But this should not be in this series, I guess. >> + spin_unlock_irqrestore(&irq->irq_lock, flags); >> + >> + vgic_put_irq(d, irq); >> + >> + return desc; >> +} >> + >> +/* >> + * was: >> + * int kvm_vgic_map_phys_irq(struct vcpu *vcpu, u32 virt_irq, >> u32 phys_irq) >> + * int kvm_vgic_unmap_phys_irq(struct vcpu *vcpu, unsigned int >> virt_irq) >> + */ >> +int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu, >> + unsigned int virt_irq, struct irq_desc *desc, >> + bool connect) > > Indentation. > >> +{ >> + struct vgic_irq *irq = vgic_get_irq(d, vcpu, virt_irq); >> + unsigned long flags; >> + int ret = 0; >> + >> + if ( !irq ) >> + return -EINVAL; >> + >> + spin_lock_irqsave(&irq->irq_lock, flags); >> + >> + if ( connect ) /* assign a mapped IRQ */ >> + { >> + /* The VIRQ should not be already enabled by the guest */ >> + if ( !irq->hw && !irq->enabled ) >> + { >> + irq->hw = true; >> + irq->hwintid = desc->irq; >> + } >> + else >> + { >> + ret = -EBUSY; >> + } > > I know that it should not matter for SPIs today. But aren't you meant to > get a reference on that interrupt if you connect it? No, the refcount feature is strictly for the pointer to the structure, not for everything related to this virtual IRQ. We store only the virtual IRQ number in the dev_id/info members, we will get the struct vgic_irq pointer via the vIRQ number on do_IRQ(). Does that make sense? >> + } >> + else /* remove a mapped IRQ */ >> + { >> + irq->hw = false; >> + irq->hwintid = 0; > > Here you blindly remove the interrupt without been sure it was the > correct physical one. We should have a check like in the current vGIC > version. Fixed. Cheers, Andre. >> + } >> + >> + spin_unlock_irqrestore(&irq->irq_lock, flags); >> + vgic_put_irq(d, irq); >> + >> + return ret; >> +} >> + >> /* >> * Local variables: >> * mode: C >> > > Cheers, >
On 23/02/18 18:02, Andre Przywara wrote: > Hi, Hi Andre, > On 19/02/18 12:19, Julien Grall wrote: >> Hi, >> >> On 09/02/18 14:39, Andre Przywara wrote: >>> The VGIC supports virtual IRQs to be connected to a hardware IRQ, so >>> when a guest EOIs the virtual interrupt, it affects the state of that >>> corresponding interrupt on the hardware side at the same time. >>> Implement the interface that the Xen arch/core code expects to connect >>> the virtual and the physical world. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >>> --- >>> xen/arch/arm/vgic/vgic.c | 63 >>> ++++++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 63 insertions(+) >>> >>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c >>> index dc5e011fa3..8d5260a7db 100644 >>> --- a/xen/arch/arm/vgic/vgic.c >>> +++ b/xen/arch/arm/vgic/vgic.c >>> @@ -693,6 +693,69 @@ void vgic_kick_vcpus(struct domain *d) >>> } >>> } >>> +struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu >>> *v, >>> + unsigned int virq) >>> +{ >>> + struct irq_desc *desc = NULL; >>> + struct vgic_irq *irq = vgic_get_irq(d, v, virq); >>> + unsigned long flags; >>> + >>> + if ( !irq ) >>> + return NULL; >>> + >>> + spin_lock_irqsave(&irq->irq_lock, flags); >>> + if ( irq->hw ) >>> + desc = irq_to_desc(irq->hwintid); >> >> This is not going to work well for PPIs. We should consider to add at >> least an ASSERT(...) in the code to prevent bad use of it. > > Yeah, done. But I wonder if we eventually should extend the > irq_to_desc() function to take the vCPU, since we will need it anyway > once we use hardware mapped timer IRQs (PPIs) in the future. But this > should not be in this series, I guess. irq_to_desc only deal with hardware interrupt, so you mean pCPU instead of vCPU? > >>> + spin_unlock_irqrestore(&irq->irq_lock, flags); >>> + >>> + vgic_put_irq(d, irq); >>> + >>> + return desc; >>> +} >>> + >>> +/* >>> + * was: >>> + * int kvm_vgic_map_phys_irq(struct vcpu *vcpu, u32 virt_irq, >>> u32 phys_irq) >>> + * int kvm_vgic_unmap_phys_irq(struct vcpu *vcpu, unsigned int >>> virt_irq) >>> + */ >>> +int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu, >>> + unsigned int virt_irq, struct irq_desc *desc, >>> + bool connect) >> >> Indentation. >> >>> +{ >>> + struct vgic_irq *irq = vgic_get_irq(d, vcpu, virt_irq); >>> + unsigned long flags; >>> + int ret = 0; >>> + >>> + if ( !irq ) >>> + return -EINVAL; >>> + >>> + spin_lock_irqsave(&irq->irq_lock, flags); >>> + >>> + if ( connect ) /* assign a mapped IRQ */ >>> + { >>> + /* The VIRQ should not be already enabled by the guest */ >>> + if ( !irq->hw && !irq->enabled ) >>> + { >>> + irq->hw = true; >>> + irq->hwintid = desc->irq; >>> + } >>> + else >>> + { >>> + ret = -EBUSY; >>> + } >> >> I know that it should not matter for SPIs today. But aren't you meant to >> get a reference on that interrupt if you connect it? > > No, the refcount feature is strictly for the pointer to the structure, > not for everything related to this virtual IRQ. > We store only the virtual IRQ number in the dev_id/info members, we will > get the struct vgic_irq pointer via the vIRQ number on do_IRQ(). > Does that make sense? But technically you "allocate" the virtual SPI at that time, right? So this would mean you need to get a reference, otherwise it might disappear. So I am not entirely sure why the reference is not necessary here. Cheers,
Hi, On 23/02/18 18:14, Julien Grall wrote: > > > On 23/02/18 18:02, Andre Przywara wrote: >> Hi, > > Hi Andre, > >> On 19/02/18 12:19, Julien Grall wrote: >>> Hi, >>> >>> On 09/02/18 14:39, Andre Przywara wrote: >>>> The VGIC supports virtual IRQs to be connected to a hardware IRQ, so >>>> when a guest EOIs the virtual interrupt, it affects the state of that >>>> corresponding interrupt on the hardware side at the same time. >>>> Implement the interface that the Xen arch/core code expects to connect >>>> the virtual and the physical world. >>>> >>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >>>> --- >>>> xen/arch/arm/vgic/vgic.c | 63 >>>> ++++++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 63 insertions(+) >>>> >>>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c >>>> index dc5e011fa3..8d5260a7db 100644 >>>> --- a/xen/arch/arm/vgic/vgic.c >>>> +++ b/xen/arch/arm/vgic/vgic.c >>>> @@ -693,6 +693,69 @@ void vgic_kick_vcpus(struct domain *d) >>>> } >>>> } >>>> +struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu >>>> *v, >>>> + unsigned int virq) >>>> +{ >>>> + struct irq_desc *desc = NULL; >>>> + struct vgic_irq *irq = vgic_get_irq(d, v, virq); >>>> + unsigned long flags; >>>> + >>>> + if ( !irq ) >>>> + return NULL; >>>> + >>>> + spin_lock_irqsave(&irq->irq_lock, flags); >>>> + if ( irq->hw ) >>>> + desc = irq_to_desc(irq->hwintid); >>> >>> This is not going to work well for PPIs. We should consider to add at >>> least an ASSERT(...) in the code to prevent bad use of it. >> >> Yeah, done. But I wonder if we eventually should extend the >> irq_to_desc() function to take the vCPU, since we will need it anyway >> once we use hardware mapped timer IRQs (PPIs) in the future. But this >> should not be in this series, I guess. > > irq_to_desc only deal with hardware interrupt, so you mean pCPU instead > of vCPU? Yes, indeed. But I think this points to the problem of this approach: the virtual IRQ is tied to a VCPU, and we have to make sure that not only the affinity is updated on a CPU migration (as we do for SPIs), but actually the interrupt itself is changed: since CPU0/PPI9 has a different irq_desc* from, say, CPU1/PPI9. So there is more than just adding a parameter to irq_to_desc(). >>>> + spin_unlock_irqrestore(&irq->irq_lock, flags); >>>> + >>>> + vgic_put_irq(d, irq); >>>> + >>>> + return desc; >>>> +} >>>> + >>>> +/* >>>> + * was: >>>> + * int kvm_vgic_map_phys_irq(struct vcpu *vcpu, u32 virt_irq, >>>> u32 phys_irq) >>>> + * int kvm_vgic_unmap_phys_irq(struct vcpu *vcpu, unsigned int >>>> virt_irq) >>>> + */ >>>> +int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu, >>>> + unsigned int virt_irq, struct irq_desc *desc, >>>> + bool connect) >>> >>> Indentation. >>> >>>> +{ >>>> + struct vgic_irq *irq = vgic_get_irq(d, vcpu, virt_irq); >>>> + unsigned long flags; >>>> + int ret = 0; >>>> + >>>> + if ( !irq ) >>>> + return -EINVAL; >>>> + >>>> + spin_lock_irqsave(&irq->irq_lock, flags); >>>> + >>>> + if ( connect ) /* assign a mapped IRQ */ >>>> + { >>>> + /* The VIRQ should not be already enabled by the guest */ >>>> + if ( !irq->hw && !irq->enabled ) >>>> + { >>>> + irq->hw = true; >>>> + irq->hwintid = desc->irq; >>>> + } >>>> + else >>>> + { >>>> + ret = -EBUSY; >>>> + } >>> >>> I know that it should not matter for SPIs today. But aren't you meant to >>> get a reference on that interrupt if you connect it? >> >> No, the refcount feature is strictly for the pointer to the structure, >> not for everything related to this virtual IRQ. >> We store only the virtual IRQ number in the dev_id/info members, we will >> get the struct vgic_irq pointer via the vIRQ number on do_IRQ(). >> Does that make sense? > > But technically you "allocate" the virtual SPI at that time, right? So > this would mean you need to get a reference, otherwise it might disappear. We will realise that is has disappeared when vgic_get_irq() called with that virtual number returns NULL. The refcount is really just to know when you can free dynamically allocated struct vgic_irqs, so it's strictly about the *pointer* to the *memory*, not about the logical entity of that particular virtual IRQ. Actually it should not really happen that you end up with a hardware IRQ still assigned to an abandoned virtual IRQ, as you would expect to free that connection *before* disbanding the virtual IRQ. > So I am not entirely sure why the reference is not necessary here. Typically to remove a virtual IRQ, you arrange for vgic_get_irq() to return NULL on that number. Then you "wait" for the refcount to drop to zero, at which point it's safe to free the memory allocated for that vgic_irq. As mentioned, only really useful for LPIs, but it's a central property of the new VGIC architecture, because we need to have those gets/puts in virtually every function. Cheers, Andre.
On 02/26/2018 04:48 PM, Andre Przywara wrote: > Hi, > > On 23/02/18 18:14, Julien Grall wrote: >> >> >> On 23/02/18 18:02, Andre Przywara wrote: >>> Hi, >> >> Hi Andre, >> >>> On 19/02/18 12:19, Julien Grall wrote: >>>> Hi, >>>> >>>> On 09/02/18 14:39, Andre Przywara wrote: >>>>> The VGIC supports virtual IRQs to be connected to a hardware IRQ, so >>>>> when a guest EOIs the virtual interrupt, it affects the state of that >>>>> corresponding interrupt on the hardware side at the same time. >>>>> Implement the interface that the Xen arch/core code expects to connect >>>>> the virtual and the physical world. >>>>> >>>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >>>>> --- >>>>> xen/arch/arm/vgic/vgic.c | 63 >>>>> ++++++++++++++++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 63 insertions(+) >>>>> >>>>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c >>>>> index dc5e011fa3..8d5260a7db 100644 >>>>> --- a/xen/arch/arm/vgic/vgic.c >>>>> +++ b/xen/arch/arm/vgic/vgic.c >>>>> @@ -693,6 +693,69 @@ void vgic_kick_vcpus(struct domain *d) >>>>> } >>>>> } >>>>> +struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu >>>>> *v, >>>>> + unsigned int virq) >>>>> +{ >>>>> + struct irq_desc *desc = NULL; >>>>> + struct vgic_irq *irq = vgic_get_irq(d, v, virq); >>>>> + unsigned long flags; >>>>> + >>>>> + if ( !irq ) >>>>> + return NULL; >>>>> + >>>>> + spin_lock_irqsave(&irq->irq_lock, flags); >>>>> + if ( irq->hw ) >>>>> + desc = irq_to_desc(irq->hwintid); >>>> >>>> This is not going to work well for PPIs. We should consider to add at >>>> least an ASSERT(...) in the code to prevent bad use of it. >>> >>> Yeah, done. But I wonder if we eventually should extend the >>> irq_to_desc() function to take the vCPU, since we will need it anyway >>> once we use hardware mapped timer IRQs (PPIs) in the future. But this >>> should not be in this series, I guess. >> >> irq_to_desc only deal with hardware interrupt, so you mean pCPU instead >> of vCPU? > > Yes, indeed. But I think this points to the problem of this approach: > the virtual IRQ is tied to a VCPU, and we have to make sure that not > only the affinity is updated on a CPU migration (as we do for SPIs), but > actually the interrupt itself is changed: since CPU0/PPI9 has a > different irq_desc* from, say, CPU1/PPI9. > So there is more than just adding a parameter to irq_to_desc(). Change in the irq_to_desc() interface needs to be justify. The use case I have in mind for PPI is the virtual timer. In that case, you will always receive the PPI on the right pCPU. Do you really see a use case where a vCPU is running on pCPU A but the PPI is routed to pCPU B? > >>>>> + spin_unlock_irqrestore(&irq->irq_lock, flags); >>>>> + >>>>> + vgic_put_irq(d, irq); >>>>> + >>>>> + return desc; >>>>> +} >>>>> + >>>>> +/* >>>>> + * was: >>>>> + * int kvm_vgic_map_phys_irq(struct vcpu *vcpu, u32 virt_irq, >>>>> u32 phys_irq) >>>>> + * int kvm_vgic_unmap_phys_irq(struct vcpu *vcpu, unsigned int >>>>> virt_irq) >>>>> + */ >>>>> +int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu, >>>>> + unsigned int virt_irq, struct irq_desc *desc, >>>>> + bool connect) >>>> >>>> Indentation. >>>> >>>>> +{ >>>>> + struct vgic_irq *irq = vgic_get_irq(d, vcpu, virt_irq); >>>>> + unsigned long flags; >>>>> + int ret = 0; >>>>> + >>>>> + if ( !irq ) >>>>> + return -EINVAL; >>>>> + >>>>> + spin_lock_irqsave(&irq->irq_lock, flags); >>>>> + >>>>> + if ( connect ) /* assign a mapped IRQ */ >>>>> + { >>>>> + /* The VIRQ should not be already enabled by the guest */ >>>>> + if ( !irq->hw && !irq->enabled ) >>>>> + { >>>>> + irq->hw = true; >>>>> + irq->hwintid = desc->irq; >>>>> + } >>>>> + else >>>>> + { >>>>> + ret = -EBUSY; >>>>> + } >>>> >>>> I know that it should not matter for SPIs today. But aren't you meant to >>>> get a reference on that interrupt if you connect it? >>> >>> No, the refcount feature is strictly for the pointer to the structure, >>> not for everything related to this virtual IRQ. >>> We store only the virtual IRQ number in the dev_id/info members, we will >>> get the struct vgic_irq pointer via the vIRQ number on do_IRQ(). >>> Does that make sense? >> >> But technically you "allocate" the virtual SPI at that time, right? So >> this would mean you need to get a reference, otherwise it might disappear. > > We will realise that is has disappeared when vgic_get_irq() called with > that virtual number returns NULL. The refcount is really just to know > when you can free dynamically allocated struct vgic_irqs, so it's > strictly about the *pointer* to the *memory*, not about the logical > entity of that particular virtual IRQ. > Actually it should not really happen that you end up with a hardware IRQ > still assigned to an abandoned virtual IRQ, as you would expect to free > that connection *before* disbanding the virtual IRQ. > >> So I am not entirely sure why the reference is not necessary here. > > Typically to remove a virtual IRQ, you arrange for vgic_get_irq() to > return NULL on that number. Then you "wait" for the refcount to drop to > zero, at which point it's safe to free the memory allocated for that > vgic_irq. As mentioned, only really useful for LPIs, but it's a central > property of the new VGIC architecture, because we need to have those > gets/puts in virtually every function. Thank you for the explanation. Cheers,
Hi, On 26/02/18 16:57, Julien Grall wrote: > > > On 02/26/2018 04:48 PM, Andre Przywara wrote: >> Hi, >> >> On 23/02/18 18:14, Julien Grall wrote: >>> >>> >>> On 23/02/18 18:02, Andre Przywara wrote: >>>> Hi, >>> >>> Hi Andre, >>> >>>> On 19/02/18 12:19, Julien Grall wrote: >>>>> Hi, >>>>> >>>>> On 09/02/18 14:39, Andre Przywara wrote: >>>>>> The VGIC supports virtual IRQs to be connected to a hardware IRQ, so >>>>>> when a guest EOIs the virtual interrupt, it affects the state of that >>>>>> corresponding interrupt on the hardware side at the same time. >>>>>> Implement the interface that the Xen arch/core code expects to >>>>>> connect >>>>>> the virtual and the physical world. >>>>>> >>>>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >>>>>> --- >>>>>> xen/arch/arm/vgic/vgic.c | 63 >>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 63 insertions(+) >>>>>> >>>>>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c >>>>>> index dc5e011fa3..8d5260a7db 100644 >>>>>> --- a/xen/arch/arm/vgic/vgic.c >>>>>> +++ b/xen/arch/arm/vgic/vgic.c >>>>>> @@ -693,6 +693,69 @@ void vgic_kick_vcpus(struct domain *d) >>>>>> } >>>>>> } >>>>>> +struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, >>>>>> struct vcpu >>>>>> *v, >>>>>> + unsigned int virq) >>>>>> +{ >>>>>> + struct irq_desc *desc = NULL; >>>>>> + struct vgic_irq *irq = vgic_get_irq(d, v, virq); >>>>>> + unsigned long flags; >>>>>> + >>>>>> + if ( !irq ) >>>>>> + return NULL; >>>>>> + >>>>>> + spin_lock_irqsave(&irq->irq_lock, flags); >>>>>> + if ( irq->hw ) >>>>>> + desc = irq_to_desc(irq->hwintid); >>>>> >>>>> This is not going to work well for PPIs. We should consider to add at >>>>> least an ASSERT(...) in the code to prevent bad use of it. >>>> >>>> Yeah, done. But I wonder if we eventually should extend the >>>> irq_to_desc() function to take the vCPU, since we will need it anyway >>>> once we use hardware mapped timer IRQs (PPIs) in the future. But this >>>> should not be in this series, I guess. >>> >>> irq_to_desc only deal with hardware interrupt, so you mean pCPU instead >>> of vCPU? >> >> Yes, indeed. But I think this points to the problem of this approach: >> the virtual IRQ is tied to a VCPU, and we have to make sure that not >> only the affinity is updated on a CPU migration (as we do for SPIs), but >> actually the interrupt itself is changed: since CPU0/PPI9 has a >> different irq_desc* from, say, CPU1/PPI9. >> So there is more than just adding a parameter to irq_to_desc(). > > Change in the irq_to_desc() interface needs to be justify. The use case > I have in mind for PPI is the virtual timer. In that case, you will > always receive the PPI on the right pCPU. Yes, but the connection between virtual and physical IRQ is realised as a connection between struct pending_irq/vgic_irq and struct irq_desc. For an SPI this is always the same irq_desc, regardless of the affinity or running CPU. But for PPIs you would need to change the actual irq_desc pointer when changing the affinity. Not really rocket science (though it may become nasty with the locking), but needs to be implemented. > Do you really see a use case where a vCPU is running on pCPU A but the > PPI is routed to pCPU B? Not at the moment, I guess the very nature of PPIs would avoid this. The other PPIs I know about are PMUs and the VGIC. The latter is not of a concern for us yet (fortunately). PMUs should have the same local property as the arch timer. Cheers, Andre. >>>>>> + spin_unlock_irqrestore(&irq->irq_lock, flags); >>>>>> + >>>>>> + vgic_put_irq(d, irq); >>>>>> + >>>>>> + return desc; >>>>>> +} >>>>>> + >>>>>> +/* >>>>>> + * was: >>>>>> + * int kvm_vgic_map_phys_irq(struct vcpu *vcpu, u32 virt_irq, >>>>>> u32 phys_irq) >>>>>> + * int kvm_vgic_unmap_phys_irq(struct vcpu *vcpu, unsigned int >>>>>> virt_irq) >>>>>> + */ >>>>>> +int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu, >>>>>> + unsigned int virt_irq, struct irq_desc *desc, >>>>>> + bool connect) >>>>> >>>>> Indentation. >>>>> >>>>>> +{ >>>>>> + struct vgic_irq *irq = vgic_get_irq(d, vcpu, virt_irq); >>>>>> + unsigned long flags; >>>>>> + int ret = 0; >>>>>> + >>>>>> + if ( !irq ) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + spin_lock_irqsave(&irq->irq_lock, flags); >>>>>> + >>>>>> + if ( connect ) /* assign a mapped IRQ */ >>>>>> + { >>>>>> + /* The VIRQ should not be already enabled by the guest */ >>>>>> + if ( !irq->hw && !irq->enabled ) >>>>>> + { >>>>>> + irq->hw = true; >>>>>> + irq->hwintid = desc->irq; >>>>>> + } >>>>>> + else >>>>>> + { >>>>>> + ret = -EBUSY; >>>>>> + } >>>>> >>>>> I know that it should not matter for SPIs today. But aren't you >>>>> meant to >>>>> get a reference on that interrupt if you connect it? >>>> >>>> No, the refcount feature is strictly for the pointer to the structure, >>>> not for everything related to this virtual IRQ. >>>> We store only the virtual IRQ number in the dev_id/info members, we >>>> will >>>> get the struct vgic_irq pointer via the vIRQ number on do_IRQ(). >>>> Does that make sense? >>> >>> But technically you "allocate" the virtual SPI at that time, right? So >>> this would mean you need to get a reference, otherwise it might >>> disappear. >> >> We will realise that is has disappeared when vgic_get_irq() called with >> that virtual number returns NULL. The refcount is really just to know >> when you can free dynamically allocated struct vgic_irqs, so it's >> strictly about the *pointer* to the *memory*, not about the logical >> entity of that particular virtual IRQ. >> Actually it should not really happen that you end up with a hardware IRQ >> still assigned to an abandoned virtual IRQ, as you would expect to free >> that connection *before* disbanding the virtual IRQ. >> >>> So I am not entirely sure why the reference is not necessary here. >> >> Typically to remove a virtual IRQ, you arrange for vgic_get_irq() to >> return NULL on that number. Then you "wait" for the refcount to drop to >> zero, at which point it's safe to free the memory allocated for that >> vgic_irq. As mentioned, only really useful for LPIs, but it's a central >> property of the new VGIC architecture, because we need to have those >> gets/puts in virtually every function. > > Thank you for the explanation. > > Cheers, >
Hi, On 02/26/2018 05:19 PM, Andre Przywara wrote: > Hi, > > On 26/02/18 16:57, Julien Grall wrote: >> >> >> On 02/26/2018 04:48 PM, Andre Przywara wrote: >>> Hi, >>> >>> On 23/02/18 18:14, Julien Grall wrote: >>>> >>>> >>>> On 23/02/18 18:02, Andre Przywara wrote: >>>>> Hi, >>>> >>>> Hi Andre, >>>> >>>>> On 19/02/18 12:19, Julien Grall wrote: >>>>>> Hi, >>>>>> >>>>>> On 09/02/18 14:39, Andre Przywara wrote: >>>>>>> The VGIC supports virtual IRQs to be connected to a hardware IRQ, so >>>>>>> when a guest EOIs the virtual interrupt, it affects the state of that >>>>>>> corresponding interrupt on the hardware side at the same time. >>>>>>> Implement the interface that the Xen arch/core code expects to >>>>>>> connect >>>>>>> the virtual and the physical world. >>>>>>> >>>>>>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org> >>>>>>> --- >>>>>>> xen/arch/arm/vgic/vgic.c | 63 >>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>> 1 file changed, 63 insertions(+) >>>>>>> >>>>>>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c >>>>>>> index dc5e011fa3..8d5260a7db 100644 >>>>>>> --- a/xen/arch/arm/vgic/vgic.c >>>>>>> +++ b/xen/arch/arm/vgic/vgic.c >>>>>>> @@ -693,6 +693,69 @@ void vgic_kick_vcpus(struct domain *d) >>>>>>> } >>>>>>> } >>>>>>> +struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, >>>>>>> struct vcpu >>>>>>> *v, >>>>>>> + unsigned int virq) >>>>>>> +{ >>>>>>> + struct irq_desc *desc = NULL; >>>>>>> + struct vgic_irq *irq = vgic_get_irq(d, v, virq); >>>>>>> + unsigned long flags; >>>>>>> + >>>>>>> + if ( !irq ) >>>>>>> + return NULL; >>>>>>> + >>>>>>> + spin_lock_irqsave(&irq->irq_lock, flags); >>>>>>> + if ( irq->hw ) >>>>>>> + desc = irq_to_desc(irq->hwintid); >>>>>> >>>>>> This is not going to work well for PPIs. We should consider to add at >>>>>> least an ASSERT(...) in the code to prevent bad use of it. >>>>> >>>>> Yeah, done. But I wonder if we eventually should extend the >>>>> irq_to_desc() function to take the vCPU, since we will need it anyway >>>>> once we use hardware mapped timer IRQs (PPIs) in the future. But this >>>>> should not be in this series, I guess. >>>> >>>> irq_to_desc only deal with hardware interrupt, so you mean pCPU instead >>>> of vCPU? >>> >>> Yes, indeed. But I think this points to the problem of this approach: >>> the virtual IRQ is tied to a VCPU, and we have to make sure that not >>> only the affinity is updated on a CPU migration (as we do for SPIs), but >>> actually the interrupt itself is changed: since CPU0/PPI9 has a >>> different irq_desc* from, say, CPU1/PPI9. >>> So there is more than just adding a parameter to irq_to_desc(). >> >> Change in the irq_to_desc() interface needs to be justify. The use case >> I have in mind for PPI is the virtual timer. In that case, you will >> always receive the PPI on the right pCPU. > > Yes, but the connection between virtual and physical IRQ is realised as > a connection between struct pending_irq/vgic_irq and struct irq_desc. > For an SPI this is always the same irq_desc, regardless of the affinity > or running CPU. But for PPIs you would need to change the actual > irq_desc pointer when changing the affinity. Not really rocket science > (though it may become nasty with the locking), but needs to be implemented. I don't think it is a big deal. You would remove the "link" when saving the vCPU state and add the "link" when restoring. In both case, you would be on the right pCPU. Have a look at: https://lists.xenproject.org/archives/html/xen-devel/2015-11/msg00925.html Cheers,
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c index dc5e011fa3..8d5260a7db 100644 --- a/xen/arch/arm/vgic/vgic.c +++ b/xen/arch/arm/vgic/vgic.c @@ -693,6 +693,69 @@ void vgic_kick_vcpus(struct domain *d) } } +struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu *v, + unsigned int virq) +{ + struct irq_desc *desc = NULL; + struct vgic_irq *irq = vgic_get_irq(d, v, virq); + unsigned long flags; + + if ( !irq ) + return NULL; + + spin_lock_irqsave(&irq->irq_lock, flags); + if ( irq->hw ) + desc = irq_to_desc(irq->hwintid); + spin_unlock_irqrestore(&irq->irq_lock, flags); + + vgic_put_irq(d, irq); + + return desc; +} + +/* + * was: + * int kvm_vgic_map_phys_irq(struct vcpu *vcpu, u32 virt_irq, u32 phys_irq) + * int kvm_vgic_unmap_phys_irq(struct vcpu *vcpu, unsigned int virt_irq) + */ +int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu, + unsigned int virt_irq, struct irq_desc *desc, + bool connect) +{ + struct vgic_irq *irq = vgic_get_irq(d, vcpu, virt_irq); + unsigned long flags; + int ret = 0; + + if ( !irq ) + return -EINVAL; + + spin_lock_irqsave(&irq->irq_lock, flags); + + if ( connect ) /* assign a mapped IRQ */ + { + /* The VIRQ should not be already enabled by the guest */ + if ( !irq->hw && !irq->enabled ) + { + irq->hw = true; + irq->hwintid = desc->irq; + } + else + { + ret = -EBUSY; + } + } + else /* remove a mapped IRQ */ + { + irq->hw = false; + irq->hwintid = 0; + } + + spin_unlock_irqrestore(&irq->irq_lock, flags); + vgic_put_irq(d, irq); + + return ret; +} + /* * Local variables: * mode: C
The VGIC supports virtual IRQs to be connected to a hardware IRQ, so when a guest EOIs the virtual interrupt, it affects the state of that corresponding interrupt on the hardware side at the same time. Implement the interface that the Xen arch/core code expects to connect the virtual and the physical world. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- xen/arch/arm/vgic/vgic.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+)