Re: [Xen-devel] [PATCH v2 07/17] arm64: vgic-v3: Add ICV_EOIR1_EL1 handler

[Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>]

On 04/02/2018 04:33 PM, Manish Jaggi wrote:
> On 03/27/2018 03:48 PM, Marc Zyngier wrote:
> > On 27/03/18 10:07, Manish Jaggi wrote:
> > > This patch is ported to xen from linux commit
> > > b6f49035b4bf6e2709f2a5fed3107f5438c1fd02
> > > KVM: arm64: vgic-v3: Add ICV_EOIR1_EL1 handler
> > >
> > > Add a handler for writing the guest's view of the ICC_EOIR1_EL1
> > > register. This involves dropping the priority of the interrupt,
> > > and deactivating it if required (EOImode == 0).
> > >
> > > Signed-off-by : Manish Jaggi <manish.jaggi@xxxxxxxxxx>
> > > ---
> > >   xen/arch/arm/arm64/vgic-v3-sr.c     | 136 
> > > ++++++++++++++++++++++++++++++++++++
> > >   xen/include/asm-arm/arm64/sysregs.h |   1 +
> > >   xen/include/asm-arm/gic_v3_defs.h   |   4 ++
> > >   3 files changed, 141 insertions(+)
> > >
> > > diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c 
> > > b/xen/arch/arm/arm64/vgic-v3-sr.c
> > > index 026d64506f..e32ec01f56 100644
> > > --- a/xen/arch/arm/arm64/vgic-v3-sr.c
> > > +++ b/xen/arch/arm/arm64/vgic-v3-sr.c
> > > @@ -33,6 +33,7 @@
> > >     #define ICC_IAR1_EL1_SPURIOUS    0x3ff
> > >   #define VGIC_MAX_SPI             1019
> > > +#define VGIC_MIN_LPI             8192
> > >     static int vgic_v3_bpr_min(void)
> > >   {
> > > @@ -482,6 +483,137 @@ static void vreg_emulate_iar(struct 
> > > cpu_user_regs *regs, const union hsr hsr)
> > >       vgic_v3_read_iar(regs, hsr);
> > >   }
> > >   +static int vgic_v3_find_active_lr(int intid, uint64_t *lr_val)
> > > +{
> > > +    int i;
> > > +    unsigned int used_lrs =  gic_get_num_lrs();
> > This is quite a departure from the existing code. KVM always allocate
> > LRs sequentially, and used_lrs represents the current upper bound.
> IIUC, Xen uses a function gic_find_unused_lr to find an unused LR.
>
> xen/arch/arm/gic.c:
> gic_raise_guest_irq
>     gic_find_unused_lr
> > Here,
> > you seem to be looking at *all* the LRs. Is that safe?
> IIUC Xen does not maintain a used_lrs, it does have an lr_mask, but 
> that is static in gic.c
> To do something like
> +for_each_set_bit(i, lr_mask, nr_lrs)
> + {
> +      u64 val = __gic_v3_get_lr(i);
> +      u8 lr_prio = (val & ICH_LR_PRIORITY_MASK) >> 
> ICH_LR_PRIORITY_SHIFT;
> +     /* Not pending in the state? */
> +     if ((val & ICH_LR_STATE) != ICH_LR_PENDING_BIT)
> +         continue;
>
>
> I need to do some jugglery to make lr_mask visible outside of 
> xen/arch/arm/gic.c
> The easiest would be to add an extern function, harder way would be to 
> add it in gic_hw_operations
> - vgic_v3_highest_priority_lr iterates is interested in used LR's 
> which sre in Pending state.
> - emulating IAR is done with interrupts disabled
> - iterating over all the LRs and finding which ones are in Pending.
Just to add I was answering for using num_lrs for used_lrs, above was 
for IAR flow.
This holds the same for EOIR flow as well.

The bigger point is unless I add some jugglery to access static value 
outside gic.c
this is the only solution.

Stefano/Andre/Julien
Please suggest if there is some better way...
> >   Are you
> > guaranteed not to have any stale state?
> I would request Stefano/Andre/Julien to comment here...
> > In any case, the change should be documented.
> >
> > > +
> > > +    for ( i = 0; i < used_lrs; i++ )
> > > +    {
> > > +        uint64_t val = gicv3_ich_read_lr(i);
> > > +
> > > +        if ( (val & ICH_LR_VIRTUAL_ID_MASK) == intid &&
> > > +            (val & ICH_LR_ACTIVE_BIT) )
> > > +        {
> > > +            *lr_val = val;
> > > +            return i;
> > > +        }
> > > +    }
> > > +
> > > +    *lr_val = ICC_IAR1_EL1_SPURIOUS;
> > > +    return -1;
> > > +}
> > Thanks,
> >
> >     M.

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