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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 5 Apr 2018, Julien Grall wrote:
> On 02/04/18 12:17, Manish Jaggi wrote:
> > 
> > 
> > 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...
> 
> lr_mask is already exported. So I am not sure what you need here.
> 
> However, despite the fact the variable is living in gic.c it is only used by
> the old vGIC. Newer vGIC is based on KVM, so used_lrs would be fine to use.
> 
> For the old vGIC, see below.
> 
> > > >   Are you
> > > > guaranteed not to have any stale state?
> 
> LRs are zeroed when unused and AFAICT they should always be accurate. Stefano
> can you confirm it?
> 
> So it would be ok go through all the LRs (thought with a performance hit).

Yes, in the old vgic LRs are always accurate (obvioulsy when read on the
right pcpu).

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