From: Julien Grall <julien@xen.org>
To: Henry Wang <xin.wang2@amd.com>, xen-devel@lists.xenproject.org
Cc: Stefano Stabellini <sstabellini@kernel.org>,
Bertrand Marquis <bertrand.marquis@arm.com>,
Michal Orzel <michal.orzel@amd.com>,
Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs
Date: Sun, 19 May 2024 12:08:09 +0100 [thread overview]
Message-ID: <9f086470-a17f-482a-ad98-814424da0ee5@xen.org> (raw)
In-Reply-To: <59e72623-00a7-4b19-9240-fb8c4982a381@amd.com>
Hi,
On 17/05/2024 07:03, Henry Wang wrote:
>
>
> On 5/16/2024 6:03 PM, Henry Wang wrote:
>> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>>
>> Currently, routing/removing physical interrupts are only allowed at
>> the domain creation/destroy time. For use cases such as dynamic device
>> tree overlay adding/removing, the routing/removing of physical IRQ to
>> running domains should be allowed.
>>
>> Removing the above-mentioned domain creation/dying check. Since this
>> will introduce interrupt state unsync issues for cases when the
>> interrupt is active or pending in the guest, therefore for these cases
>> we simply reject the operation. Do it for both new and old vGIC
>> implementations.
>>
>> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
>> Signed-off-by: Henry Wang <xin.wang2@amd.com>
>> ---
>> v2:
>> - Reject the case where the IRQ is active or pending in guest.
>> ---
>> xen/arch/arm/gic-vgic.c | 8 ++++++--
>> xen/arch/arm/gic.c | 15 ---------------
>> xen/arch/arm/vgic/vgic.c | 5 +++--
>> 3 files changed, 9 insertions(+), 19 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
>> index 56490dbc43..d1608415f8 100644
>> --- a/xen/arch/arm/gic-vgic.c
>> +++ b/xen/arch/arm/gic-vgic.c
>> @@ -444,14 +444,18 @@ int vgic_connect_hw_irq(struct domain *d, struct
>> vcpu *v, unsigned int virq,
>> {
>> /* The VIRQ should not be already enabled by the guest */
This comment needs to be updated.
>> if ( !p->desc &&
>> - !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
>> + !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
>> + !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) &&
>> + !test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
>> p->desc = desc;
>> else
>> ret = -EBUSY;
>> }
>> else
>> {
>> - if ( desc && p->desc != desc )
>> + if ( desc && p->desc != desc &&
>> + (test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
>> + test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status)) )
>
> This should be
>
> + if ( (desc && p->desc != desc) ||
> + test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ||
> + test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
Looking at gic_set_lr(), we first check p->desc, before setting
IRQ_GUEST_VISIBLE.
I can't find a common lock, so what would guarantee that p->desc is not
going to be used or IRQ_GUEST_VISIBLE set afterwards?
>> @@ -887,7 +887,8 @@ int vgic_connect_hw_irq(struct domain *d, struct
>> vcpu *vcpu,
>> }
>> else /* remove a mapped IRQ */
>> {
>> - if ( desc && irq->hwintid != desc->irq )
>> + if ( desc && irq->hwintid != desc->irq &&
>> + (irq->active || irq->pending_latch) )
>
> Same here, this should be
>
> + if ( (desc && irq->hwintid != desc->irq) ||
> + irq->active || irq->pending_latch )
I think the new vGIC doesn't have the same problem because it looks like
the IRQ lock will be taken for any access to 'irq'.
Cheers,
--
Julien Grall
next prev parent reply other threads:[~2024-05-19 11:08 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-16 10:03 [PATCH v2 0/8] Remaining patches for dynamic node programming using overlay dtbo Henry Wang
2024-05-16 10:03 ` [PATCH v2 1/8] xen/common/dt-overlay: Fix lock issue when add/remove the device Henry Wang
2024-05-19 10:04 ` Julien Grall
2024-05-16 10:03 ` [PATCH v2 2/8] tools/xl: Correct the help information and exit code of the dt-overlay command Henry Wang
2024-05-20 18:48 ` Jason Andryuk
2024-05-16 10:03 ` [PATCH v2 3/8] xen/arm, doc: Add a DT property to specify IOMMU for Dom0less domUs Henry Wang
2024-05-19 10:17 ` Julien Grall
2024-05-20 0:41 ` Henry Wang
2024-05-20 1:38 ` Henry Wang
2024-05-20 1:40 ` Henry Wang
2024-05-16 10:03 ` [PATCH v2 4/8] tools/arm: Introduce the "nr_spis" xl config entry Henry Wang
2024-05-20 19:13 ` Jason Andryuk
2024-05-20 23:19 ` Henry Wang
2024-05-16 10:03 ` [PATCH v2 5/8] xen/arm/gic: Allow routing/removing interrupt to running VMs Henry Wang
2024-05-17 6:03 ` Henry Wang
2024-05-19 11:08 ` Julien Grall [this message]
2024-05-20 1:01 ` Henry Wang
2024-05-20 9:50 ` Julien Grall
2024-05-21 2:29 ` Stefano Stabellini
2024-05-16 10:03 ` [PATCH v2 6/8] xen/arm: Add XEN_DOMCTL_dt_overlay DOMCTL and related operations Henry Wang
2024-05-16 12:31 ` Jan Beulich
2024-05-17 1:36 ` Henry Wang
2024-05-17 7:11 ` Jan Beulich
2024-05-16 10:03 ` [PATCH v2 7/8] tools: Introduce the "xl dt-overlay {attach,detach}" commands Henry Wang
2024-05-20 19:41 ` Jason Andryuk
2024-05-20 23:21 ` Henry Wang
2024-05-16 10:03 ` [PATCH v2 8/8] docs: Add device tree overlay documentation Henry Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9f086470-a17f-482a-ad98-814424da0ee5@xen.org \
--to=julien@xen.org \
--cc=Volodymyr_Babchuk@epam.com \
--cc=bertrand.marquis@arm.com \
--cc=michal.orzel@amd.com \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
--cc=xin.wang2@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).