Re: [Xen-devel] [PATCH v3 19/39] ARM: new VGIC: Add ENABLE registers handlers

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 28 Mar 2018, Andre Przywara wrote:
> Hi,
> 
> On 27/03/18 22:06, Stefano Stabellini wrote:
> > On Wed, 21 Mar 2018, Andre Przywara wrote:
> >> As the enable register handlers are shared between the v2 and v3
> >> emulation, their implementation goes into vgic-mmio.c, to be easily
> >> referenced from the v3 emulation as well later.
> >> This introduces a vgic_sync_hardware_irq() function, which updates the
> >> physical side of a hardware mapped virtual IRQ.
> >> Because the existing locking order between vgic_irq->irq_lock and
> >> irq_desc->lock dictates so, we drop the irq_lock and retake them in the
> >> proper order.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> >> Reviewed-by: Julien Grall <julien.grall@xxxxxxx>
> >> ---
> >> Changelog v2 ... v3:
> >> - fix indentation
> >> - fix wording in comment
> >> - add Reviewed-by:
> >>
> >> Changelog v1 ... v2:
> >> - ASSERT on h/w IRQ and vIRQ staying in sync
> >>
> >>  xen/arch/arm/vgic/vgic-mmio-v2.c |   4 +-
> >>  xen/arch/arm/vgic/vgic-mmio.c    | 117 
> >> +++++++++++++++++++++++++++++++++++++++
> >>  xen/arch/arm/vgic/vgic-mmio.h    |  11 ++++
> >>  xen/arch/arm/vgic/vgic.c         |  40 +++++++++++++
> >>  xen/arch/arm/vgic/vgic.h         |   3 +
> >>  5 files changed, 173 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c 
> >> b/xen/arch/arm/vgic/vgic-mmio-v2.c
> >> index 43c1ab5906..7efd1c4eb4 100644
> >> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c
> >> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
> >> @@ -89,10 +89,10 @@ static const struct vgic_register_region 
> >> vgic_v2_dist_registers[] = {
> >>          vgic_mmio_read_rao, vgic_mmio_write_wi, 1,
> >>          VGIC_ACCESS_32bit),
> >>      REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISENABLER,
> >> -        vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> >> +        vgic_mmio_read_enable, vgic_mmio_write_senable, 1,
> >>          VGIC_ACCESS_32bit),
> >>      REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICENABLER,
> >> -        vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> >> +        vgic_mmio_read_enable, vgic_mmio_write_cenable, 1,
> >>          VGIC_ACCESS_32bit),
> >>      REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISPENDR,
> >>          vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
> >> diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c
> >> index a03e8d88b9..f219b7c509 100644
> >> --- a/xen/arch/arm/vgic/vgic-mmio.c
> >> +++ b/xen/arch/arm/vgic/vgic-mmio.c
> >> @@ -39,6 +39,123 @@ void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t 
> >> addr,
> >>      /* Ignore */
> >>  }
> >>  
> >> +/*
> >> + * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the 
> >> value
> >> + * of the enabled bit, so there is only one function for both here.
> >> + */
> >> +unsigned long vgic_mmio_read_enable(struct vcpu *vcpu,
> >> +                                    paddr_t addr, unsigned int len)
> >> +{
> >> +    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1);
> >> +    uint32_t value = 0;
> >> +    unsigned int i;
> >> +
> >> +    /* Loop over all IRQs affected by this read */
> >> +    for ( i = 0; i < len * 8; i++ )
> >> +    {
> >> +        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + 
> >> i);
> >> +
> >> +        if ( irq->enabled )
> >> +            value |= (1U << i);
> > 
> > Don't we need to take the irq->irq_lock before reading irq->enabled?
> 
> Not really. A boolean has no illegal state, so we can't read any
> intermediate values.
> 
> If you think about concurrent writes: That is even racy on real
> hardware, and normally you expect a sane driver to take a lock around
> every distributor access (cf. spin_lock(&gicv2.lock)).
> Keep in mind that only a guest can change the enabled state.
> 
> So the rationale behind those unlocked reads is:
> As long as it doesn't harm the hypervisor, we don't care too much about
> being 100% correct in a situation that is out of spec anyway.
> We discussed this issue also with Julien before:
> https://lists.xen.org/archives/html/xen-devel/2018-02/msg02148.html

OK, I buy the argument.

Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

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