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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

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).

Cheers,

--
Julien Grall

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