Re: [RFC PATCH] xen/arm: Track coverage gap due to architecture limitations

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Michal,

On 09/07/2025 14:38, Orzel, Michal wrote:
> On 09/07/2025 15:12, Ayan Kumar Halder wrote:
> > There are a number of places where Xen triggers a BUG() due to 'impossible'
> > conditions. One of these impossible condition is when gicv3_info.nr_lrs is
> > equal to 0 or greater than 16.
> >
> > Add a OFT marker in the code and link it to document explaining why this
> > condition is impossible to be covered while running Xen on a platform. As a
> > consequence, explain the architectural limitation that Xen relies on.
> >
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > ---
> > While running Xen with coverage, there will be some parts of code which cannot
> > be covered as long as we run Xen on a supported platform. If we have link the
> > code with the specific explanation, this will provide some context for the
> > missing coverage. Also if the relevant part of the code is ever modified, the
> > dev can update the explanation.
> >
> >   docs/fusa/coverage_gaps.rst | 17 +++++++++++++++++
> >   xen/arch/arm/gic-v3.c       |  4 ++++
> >   2 files changed, 21 insertions(+)
> >   create mode 100644 docs/fusa/coverage_gaps.rst
> >
> > diff --git a/docs/fusa/coverage_gaps.rst b/docs/fusa/coverage_gaps.rst
> > new file mode 100644
> > index 0000000000..3cb877a8ee
> > --- /dev/null
> > +++ b/docs/fusa/coverage_gaps.rst
> > @@ -0,0 +1,17 @@
> > +
> > +Coverage gap
> > +============
> > +
> > +No support for zero or greater than 16 LRs
> > +------------------------------------------
> > +
> > +`CovGapExp~unexp_err~1`
> I think the tags need to be named better. 'unexp_err' does not mean a lot when
> seen in a code.
While writing this, I was thinking that this is an unexpected error. Can 
I name this
`CovGapExp~gicv3_lr~1`, so we can correlate this with the component ?

> > +
> > +Description:
> > +gicv3_info.nr_lrs is populated by reading ICH_VTR_EL2.ListRegs. The supported
> > +number of list resgister is 1 - 16. Thus, any value outside of the range is
> s/resgister/registers
Ack
> > +currently unsupported by Xen.
> Having a description written this way is misleading as it gives a false
> impression that this is Xen limitation. It is not. ListRegs bitfield is 4 bits
> width, therefore it's a GIC limit.
I agree. Should I say ?

...... currently unsupported by GICv3 (Refer Arm IHI 0069H.bID041224, 
Sec 12.4.9 ICH_VTR_EL2).
Thus, we know eactly the source of truth. If Arm specs change in future 
and Xen is updated, then we can update with reference to the latest doc.
- Ayan

> > +
> > +Needs:
> > + - CovGap
> > +
> > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> > index cd3e1acf79..eca4d76f5a 100644
> > --- a/xen/arch/arm/gic-v3.c
> > +++ b/xen/arch/arm/gic-v3.c
> > @@ -109,6 +109,7 @@ static inline void gicv3_save_lrs(struct vcpu *v)
> >       case 1:
> >            v->arch.gic.v3.lr[0] = READ_SYSREG_LR(0);
> >            break;
> > +    /* [CovGap~~1->CovGapExp~unexp_err~1] */
> >       default:
> >            BUG();
> >       }
> > @@ -171,6 +172,7 @@ static inline void gicv3_restore_lrs(const struct vcpu *v)
> >       case 1:
> >           WRITE_SYSREG_LR(v->arch.gic.v3.lr[0], 0);
> >           break;
> > +    /* [CovGap~~1->CovGapExp~unexp_err~1] */
> >       default:
> >            BUG();
> >       }
> > @@ -196,6 +198,7 @@ static uint64_t gicv3_ich_read_lr(int lr)
> >       case 13: return READ_SYSREG_LR(13);
> >       case 14: return READ_SYSREG_LR(14);
> >       case 15: return READ_SYSREG_LR(15);
> > +    /* [CovGap~~1->CovGapExp~unexp_err~1] */
> >       default:
> >           BUG();
> >       }
> > @@ -253,6 +256,7 @@ static void gicv3_ich_write_lr(int lr, uint64_t val)
> >       case 15:
> >           WRITE_SYSREG_LR(val, 15);
> >           break;
> > +    /* [CovGap~~1->CovGapExp~unexp_err~1] */
> >       default:
> >           return;
> >       }~Michal