| Message ID | 20170526173540.10066-20-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andre, On 05/26/2017 06:35 PM, Andre Przywara wrote: > For each device we allocate one struct pending_irq for each virtual > event (MSI). > Provide a helper function which returns the pointer to the appropriate > struct, to be able to find the right struct when given a virtual > deviceID/eventID pair. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/gic-v3-its.c | 59 ++++++++++++++++++++++++++++++++++++++++ > xen/include/asm-arm/gic_v3_its.h | 4 +++ > 2 files changed, 63 insertions(+) > > diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c > index aebc257..38f0840 100644 > --- a/xen/arch/arm/gic-v3-its.c > +++ b/xen/arch/arm/gic-v3-its.c > @@ -800,6 +800,65 @@ out: > return ret; > } > > +/* Must be called with the its_device_lock held. */ > +static struct its_device *get_its_device(struct domain *d, paddr_t vdoorbell, > + uint32_t vdevid) > +{ > + struct rb_node *node = d->arch.vgic.its_devices.rb_node; > + struct its_device *dev; > + > + ASSERT(spin_is_locked(&d->arch.vgic.its_devices_lock)); > + > + while (node) > + { > + int cmp; > + > + dev = rb_entry(node, struct its_device, rbnode); > + cmp = compare_its_guest_devices(dev, vdoorbell, vdevid); > + > + if ( !cmp ) > + return dev; > + > + if ( cmp > 0 ) > + node = node->rb_left; > + else > + node = node->rb_right; > + } > + > + return NULL; > +} > + > +static struct pending_irq *get_event_pending_irq(struct domain *d, > + paddr_t vdoorbell_address, > + uint32_t vdevid, > + uint32_t eventid, > + uint32_t *host_lpi) > +{ > + struct its_device *dev; > + struct pending_irq *pirq = NULL; > + > + spin_lock(&d->arch.vgic.its_devices_lock); > + dev = get_its_device(d, vdoorbell_address, vdevid); > + if ( dev && eventid < dev->eventids ) > + { > + pirq = &dev->pend_irqs[eventid]; > + if ( host_lpi ) > + *host_lpi = dev->host_lpi_blocks[eventid / LPI_BLOCK] + > + (eventid % LPI_BLOCK); > + } > + spin_unlock(&d->arch.vgic.its_devices_lock); > + > + return pirq; > +} > + > +struct pending_irq *gicv3_its_get_event_pending_irq(struct domain *d, > + paddr_t vdoorbell_address, > + uint32_t vdevid, > + uint32_t eventid) > +{ It is quite rude to ignore my question: "So you never envision someone requiring the host LPI even for debug purpose? AFAICT, there are no other way to get the host LPI if necessary. It really does not hurt to expose it and provide a wrapper. As you may know I am all in favor of more helpers over the cost of one unconditional branch (see the callback example) when it results to a better code design. But here it is not about code design, it is more about what kind of information would you need outside (see above)." > + return get_event_pending_irq(d, vdoorbell_address, vdevid, eventid, NULL); > +} > + > /* Scan the DT for any ITS nodes and create a list of host ITSes out of it. */ > void gicv3_its_dt_init(const struct dt_device_node *node) > { > diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h > index 40f4ef5..d162e89 100644 > --- a/xen/include/asm-arm/gic_v3_its.h > +++ b/xen/include/asm-arm/gic_v3_its.h > @@ -169,6 +169,10 @@ int gicv3_its_map_guest_device(struct domain *d, > int gicv3_allocate_host_lpi_block(struct domain *d, uint32_t *first_lpi); > void gicv3_free_host_lpi_block(uint32_t first_lpi); > > +struct pending_irq *gicv3_its_get_event_pending_irq(struct domain *d, > + paddr_t vdoorbell_address, > + uint32_t vdevid, > + uint32_t veventid); > #else > > static inline void gicv3_its_dt_init(const struct dt_device_node *node) > Cheers,
On 06/02/2017 05:32 PM, Julien Grall wrote: >> /* Scan the DT for any ITS nodes and create a list of host ITSes out >> of it. */ >> void gicv3_its_dt_init(const struct dt_device_node *node) >> { >> diff --git a/xen/include/asm-arm/gic_v3_its.h >> b/xen/include/asm-arm/gic_v3_its.h >> index 40f4ef5..d162e89 100644 >> --- a/xen/include/asm-arm/gic_v3_its.h >> +++ b/xen/include/asm-arm/gic_v3_its.h >> @@ -169,6 +169,10 @@ int gicv3_its_map_guest_device(struct domain *d, >> int gicv3_allocate_host_lpi_block(struct domain *d, uint32_t >> *first_lpi); >> void gicv3_free_host_lpi_block(uint32_t first_lpi); >> +struct pending_irq *gicv3_its_get_event_pending_irq(struct domain *d, >> + paddr_t >> vdoorbell_address, >> + uint32_t vdevid, >> + uint32_t veventid); NIT: Also, you fixed the declaration but not for the prototype: s/veventid/eventid/
Hi, On 02/06/17 17:32, Julien Grall wrote: > Hi Andre, > > On 05/26/2017 06:35 PM, Andre Przywara wrote: >> For each device we allocate one struct pending_irq for each virtual >> event (MSI). >> Provide a helper function which returns the pointer to the appropriate >> struct, to be able to find the right struct when given a virtual >> deviceID/eventID pair. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> xen/arch/arm/gic-v3-its.c | 59 >> ++++++++++++++++++++++++++++++++++++++++ >> xen/include/asm-arm/gic_v3_its.h | 4 +++ >> 2 files changed, 63 insertions(+) >> >> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c >> index aebc257..38f0840 100644 >> --- a/xen/arch/arm/gic-v3-its.c >> +++ b/xen/arch/arm/gic-v3-its.c >> @@ -800,6 +800,65 @@ out: >> return ret; >> } >> +/* Must be called with the its_device_lock held. */ >> +static struct its_device *get_its_device(struct domain *d, paddr_t >> vdoorbell, >> + uint32_t vdevid) >> +{ >> + struct rb_node *node = d->arch.vgic.its_devices.rb_node; >> + struct its_device *dev; >> + >> + ASSERT(spin_is_locked(&d->arch.vgic.its_devices_lock)); >> + >> + while (node) >> + { >> + int cmp; >> + >> + dev = rb_entry(node, struct its_device, rbnode); >> + cmp = compare_its_guest_devices(dev, vdoorbell, vdevid); >> + >> + if ( !cmp ) >> + return dev; >> + >> + if ( cmp > 0 ) >> + node = node->rb_left; >> + else >> + node = node->rb_right; >> + } >> + >> + return NULL; >> +} >> + >> +static struct pending_irq *get_event_pending_irq(struct domain *d, >> + paddr_t >> vdoorbell_address, >> + uint32_t vdevid, >> + uint32_t eventid, >> + uint32_t *host_lpi) >> +{ >> + struct its_device *dev; >> + struct pending_irq *pirq = NULL; >> + >> + spin_lock(&d->arch.vgic.its_devices_lock); >> + dev = get_its_device(d, vdoorbell_address, vdevid); >> + if ( dev && eventid < dev->eventids ) >> + { >> + pirq = &dev->pend_irqs[eventid]; >> + if ( host_lpi ) >> + *host_lpi = dev->host_lpi_blocks[eventid / LPI_BLOCK] + >> + (eventid % LPI_BLOCK); >> + } >> + spin_unlock(&d->arch.vgic.its_devices_lock); >> + >> + return pirq; >> +} >> + >> +struct pending_irq *gicv3_its_get_event_pending_irq(struct domain *d, >> + paddr_t >> vdoorbell_address, >> + uint32_t vdevid, >> + uint32_t eventid) >> +{ > > It is quite rude to ignore my question: > > "So you never envision someone requiring the host LPI even for debug > purpose? > > AFAICT, there are no other way to get the host LPI if necessary. It > really does not hurt to expose it and provide a wrapper. > > As you may know I am all in favor of more helpers over the cost of one > unconditional branch (see the callback example) when it results to a > better code design. > > But here it is not about code design, it is more about what kind of > information would you need outside (see above)." Sorry, I forgot to send a reply on Friday. So I am not convinced that a *potential* debug output justifies breaking the abstraction here. The host LPI is of no concern for the guest side of the emulation code. If someone is in dire need for this information, the debug output can easily be inserted into the wrapper function or this export can be done just for this debugging session. So I'd rather not do this - as the patch demonstrated ;-) Cheers, Andre. >> + return get_event_pending_irq(d, vdoorbell_address, vdevid, >> eventid, NULL); >> +} >> + >> /* Scan the DT for any ITS nodes and create a list of host ITSes out >> of it. */ >> void gicv3_its_dt_init(const struct dt_device_node *node) >> { >> diff --git a/xen/include/asm-arm/gic_v3_its.h >> b/xen/include/asm-arm/gic_v3_its.h >> index 40f4ef5..d162e89 100644 >> --- a/xen/include/asm-arm/gic_v3_its.h >> +++ b/xen/include/asm-arm/gic_v3_its.h >> @@ -169,6 +169,10 @@ int gicv3_its_map_guest_device(struct domain *d, >> int gicv3_allocate_host_lpi_block(struct domain *d, uint32_t >> *first_lpi); >> void gicv3_free_host_lpi_block(uint32_t first_lpi); >> +struct pending_irq *gicv3_its_get_event_pending_irq(struct domain *d, >> + paddr_t >> vdoorbell_address, >> + uint32_t vdevid, >> + uint32_t veventid); >> #else >> static inline void gicv3_its_dt_init(const struct dt_device_node >> *node) >> > > Cheers, >
Hi Andre, On 06/06/17 11:19, Andre Przywara wrote: > Sorry, I forgot to send a reply on Friday. > > So I am not convinced that a *potential* debug output justifies breaking > the abstraction here. The host LPI is of no concern for the guest side > of the emulation code. If someone is in dire need for this information, > the debug output can easily be inserted into the wrapper function or > this export can be done just for this debugging session. > So I'd rather not do this - as the patch demonstrated ;-) Fair enough. So: Acked-by: Julien Grall <julien.grall@arm.com> Cheers,
diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c index aebc257..38f0840 100644 --- a/xen/arch/arm/gic-v3-its.c +++ b/xen/arch/arm/gic-v3-its.c @@ -800,6 +800,65 @@ out: return ret; } +/* Must be called with the its_device_lock held. */ +static struct its_device *get_its_device(struct domain *d, paddr_t vdoorbell, + uint32_t vdevid) +{ + struct rb_node *node = d->arch.vgic.its_devices.rb_node; + struct its_device *dev; + + ASSERT(spin_is_locked(&d->arch.vgic.its_devices_lock)); + + while (node) + { + int cmp; + + dev = rb_entry(node, struct its_device, rbnode); + cmp = compare_its_guest_devices(dev, vdoorbell, vdevid); + + if ( !cmp ) + return dev; + + if ( cmp > 0 ) + node = node->rb_left; + else + node = node->rb_right; + } + + return NULL; +} + +static struct pending_irq *get_event_pending_irq(struct domain *d, + paddr_t vdoorbell_address, + uint32_t vdevid, + uint32_t eventid, + uint32_t *host_lpi) +{ + struct its_device *dev; + struct pending_irq *pirq = NULL; + + spin_lock(&d->arch.vgic.its_devices_lock); + dev = get_its_device(d, vdoorbell_address, vdevid); + if ( dev && eventid < dev->eventids ) + { + pirq = &dev->pend_irqs[eventid]; + if ( host_lpi ) + *host_lpi = dev->host_lpi_blocks[eventid / LPI_BLOCK] + + (eventid % LPI_BLOCK); + } + spin_unlock(&d->arch.vgic.its_devices_lock); + + return pirq; +} + +struct pending_irq *gicv3_its_get_event_pending_irq(struct domain *d, + paddr_t vdoorbell_address, + uint32_t vdevid, + uint32_t eventid) +{ + return get_event_pending_irq(d, vdoorbell_address, vdevid, eventid, NULL); +} + /* Scan the DT for any ITS nodes and create a list of host ITSes out of it. */ void gicv3_its_dt_init(const struct dt_device_node *node) { diff --git a/xen/include/asm-arm/gic_v3_its.h b/xen/include/asm-arm/gic_v3_its.h index 40f4ef5..d162e89 100644 --- a/xen/include/asm-arm/gic_v3_its.h +++ b/xen/include/asm-arm/gic_v3_its.h @@ -169,6 +169,10 @@ int gicv3_its_map_guest_device(struct domain *d, int gicv3_allocate_host_lpi_block(struct domain *d, uint32_t *first_lpi); void gicv3_free_host_lpi_block(uint32_t first_lpi); +struct pending_irq *gicv3_its_get_event_pending_irq(struct domain *d, + paddr_t vdoorbell_address, + uint32_t vdevid, + uint32_t veventid); #else static inline void gicv3_its_dt_init(const struct dt_device_node *node)
For each device we allocate one struct pending_irq for each virtual event (MSI). Provide a helper function which returns the pointer to the appropriate struct, to be able to find the right struct when given a virtual deviceID/eventID pair. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/gic-v3-its.c | 59 ++++++++++++++++++++++++++++++++++++++++ xen/include/asm-arm/gic_v3_its.h | 4 +++ 2 files changed, 63 insertions(+)