[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [XEN PATCH v3 07/11] xen: arm: vgic: allow delivery of PPIs to guests





On 28/11/2019 01:07, Stefano Stabellini wrote:
On Wed, 27 Nov 2019, Julien Grall wrote:
On Tue, 26 Nov 2019, 23:18 Stefano Stabellini, <sstabellini@xxxxxxxxxx> wrote:
       On Fri, 15 Nov 2019, Stewart Hildebrand wrote:
       > Allow vgic_get_hw_irq_desc to be called with a vcpu argument.
       >
       > Use vcpu argument in vgic_connect_hw_irq.
       >
       > vgic_connect_hw_irq is called for PPIs and SPIs, not SGIs. Enforce with
       > ASSERTs.
       >
       > Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxxxxxxxxxx>
       >
       > ---
       > v3: new patch
       >
       > ---
       > Note: I have only modified the old vgic to allow delivery of PPIs.
       > ---
       >  xen/arch/arm/gic-vgic.c | 24 ++++++++++++++++--------
       >  xen/arch/arm/vgic.c     |  6 +++---
       >  2 files changed, 19 insertions(+), 11 deletions(-)
       >
       > diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
       > index 98c021f1a8..2c66a8fa92 100644
       > --- a/xen/arch/arm/gic-vgic.c
       > +++ b/xen/arch/arm/gic-vgic.c
       > @@ -418,7 +418,7 @@ struct irq_desc *vgic_get_hw_irq_desc(struct 
domain *d, struct vcpu *v,
       >  {
       >      struct pending_irq *p;
       >
       > -    ASSERT(!v && virq >= 32);
       > +    ASSERT((!v && (virq >= 32)) || (!d && v && (virq >= 16) && (virq 
< 32)));

       I don't think !d is necessary for this to work as intended so I would
       limit the ASSERT to

         ASSERT((!v && (virq >= 32)) || (v && (virq >= 16) && (virq < 32)));

       the caller can always pass v->domain

But then you have the risk to run into d != v->domain. So at least with the 
ASSERT you document the expectation.

Yes, that was not my intention.

It makes sense in certain scenarios for v to be NULL. What I was trying
to say is that when v is not-NULL, then also d should be not-NULL for
consistency. I don't think it makes sense to pass v corresponding to
vcpu1 of domain2 and d == NULL, right?

While I usually like consistency, 'd' is only used to find 'v' if it is NULL. So I really see limited reason to impose the caller to set 'd' in this case.


I don't know if you want to add a (d == v->domain) check to the ASSERT
as it is pretty busy already. I am OK either way.


--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.