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

[Ayan Kumar Halder <ayankuma@amd.com>]

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@amd.com>
>> ---
>> 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