Nuclei-Software / nuclei-sdk

Nuclei RISC-V Software Development Kit
https://doc.nucleisys.com/nuclei_sdk
Apache License 2.0
117 stars 50 forks source link

Is a bug in CTEST? #56

Closed yinqingw closed 10 months ago

yinqingw commented 10 months ago

test/core/test_compiler.c:

now:

{
    uint32_t __ALIGNED(2) data_aligned1;
    uint32_t __ALIGNED(16) data[4];
    uint32_t __ALIGNED(64) data2[4];
    unsigned long p_data = (unsigned long)(&data_aligned1);
    CTEST_LOG("Aligned 2 Byte Data address 0x%x", p_data);
    ASSERT_EQUAL(p_data & (1 - 1), 0);
    p_data = (unsigned long)(&data[0]);
    CTEST_LOG("Aligned 16 Byte Data address 0x%x", p_data);
    ASSERT_EQUAL(p_data & (16 - 1), 0);
    p_data = (unsigned long)(&data2[0]);
    CTEST_LOG("Aligned 64 Byte Data address 0x%x", p_data);
    ASSERT_EQUAL(p_data & (64 - 1), 0);
}

data_aligned1 is __ALIGNED(2), but ASSERT_EQUAL(p_data & (1 - 1), 0), shoule ASSERT_EQUAL(p_data & (2 - 1), 0)?

fanghuaqi commented 10 months ago

Yes, it is a bug, I will fix it. Thanks

yinqingw commented 10 months ago

another in test/core/test_eclic.c:

void get_max_lvl_pri(uint8_t* maxpri, uint8_t* maxlvl)
{
    uint8_t nlbits = __ECLIC_GetCfgNlbits();
    uint8_t intctlbits = (uint8_t)__ECLIC_INTCTLBITS;
    *maxpri = 0;
    *maxlvl = 0;

    if (nlbits < intctlbits) {
        *maxpri = ((1 << (intctlbits - nlbits)) - 1);
    }
    if (nlbits > 0) {
        *maxlvl = (1 << nlbits) - 1;
    }
}

if nlbits > intctlbits, maxlvlwill be error value?

fanghuaqi commented 10 months ago

another in test/core/test_eclic.c:

void get_max_lvl_pri(uint8_t* maxpri, uint8_t* maxlvl)
{
    uint8_t nlbits = __ECLIC_GetCfgNlbits();
    uint8_t intctlbits = (uint8_t)__ECLIC_INTCTLBITS;
    *maxpri = 0;
    *maxlvl = 0;

    if (nlbits < intctlbits) {
        *maxpri = ((1 << (intctlbits - nlbits)) - 1);
    }
    if (nlbits > 0) {
        *maxlvl = (1 << nlbits) - 1;
    }
}

if nlbits > intctlbits, maxlvlwill be error value?

Regarding this one, nlbits should always smaller than inictlbits, this is required by hardware.

yinqingw commented 10 months ago

But I see the following code in the NMSIS/Core/Include/core_feature_eclic.h:

__STATIC_FORCEINLINE void __ECLIC_SetLevelIRQ(IRQn_Type IRQn, uint8_t lvl_abs)
{
    uint8_t nlbits = __ECLIC_GetCfgNlbits();
    uint8_t intctlbits = (uint8_t)__ECLIC_INTCTLBITS;

    if (nlbits == 0) {
        return;
    }

    if (nlbits > intctlbits) {
        nlbits = intctlbits;
    }
    uint8_t maxlvl = ((1 << nlbits) - 1);
    if (lvl_abs > maxlvl) {
        lvl_abs = maxlvl;
    }
    uint8_t lvl = lvl_abs << (ECLIC_MAX_NLBITS - nlbits);
    uint8_t cur_ctrl = __ECLIC_GetCtrlIRQ(IRQn);
    cur_ctrl = cur_ctrl << nlbits;
    cur_ctrl = cur_ctrl >> nlbits;
    __ECLIC_SetCtrlIRQ(IRQn, (cur_ctrl | lvl));
}

This means that nlbits may be greater than intctlbits?

For example, if clicintctl[i]only have two effective bits actually implemented by the hardware(i.e. inictlbits == 2). Test CTEST(eclic, lvl_pri_irq)will fail, because ECLIC_SetCfgNlbits(3);, maxlvlwill be 7, but ECLIC_SetLevelIRQ(SysTimer_IRQn, 15);will set SysTimerIRQn level to 3, so ASSERT_EQUAL(ECLIC_GetLevelIRQ(SysTimer_IRQn), maxlvl);will fail.

CTEST(eclic, lvl_pri_irq)
{
    uint8_t oldnlbits = __ECLIC_GetCfgNlbits();
    uint8_t maxlvl, maxpri;
    uint8_t lvlnew, prinew;

    get_max_lvl_pri(&maxpri, &maxlvl);

    if (maxlvl == 0) {
        lvlnew = 0;
    } else {
        lvlnew = rand() % maxlvl;
    }
    if (maxpri == 0) {
        prinew = 0;
    } else {
        prinew = rand() % maxpri;
    }

    ECLIC_SetPriorityIRQ(SysTimer_IRQn, prinew);
    ASSERT_EQUAL(ECLIC_GetPriorityIRQ(SysTimer_IRQn), prinew);

    ECLIC_SetLevelIRQ(SysTimer_IRQn, lvlnew);
    ASSERT_EQUAL(ECLIC_GetLevelIRQ(SysTimer_IRQn), lvlnew);

    ECLIC_SetPriorityIRQ(SysTimer_IRQn, 255);
    ASSERT_EQUAL(ECLIC_GetPriorityIRQ(SysTimer_IRQn), maxpri);

    ECLIC_SetLevelIRQ(SysTimer_IRQn, 255);
    ASSERT_EQUAL(ECLIC_GetLevelIRQ(SysTimer_IRQn), maxlvl);

    ECLIC_SetCfgNlbits(3);
    get_max_lvl_pri(&maxpri, &maxlvl);
    ECLIC_SetLevelIRQ(SysTimer_IRQn, 15);
    ASSERT_EQUAL(ECLIC_GetLevelIRQ(SysTimer_IRQn), maxlvl);
    ECLIC_SetPriorityIRQ(SysTimer_IRQn, 31);
    ASSERT_EQUAL(ECLIC_GetPriorityIRQ(SysTimer_IRQn), maxpri);
    ECLIC_SetCfgNlbits(2);
    get_max_lvl_pri(&maxpri, &maxlvl);
    ECLIC_SetLevelIRQ(SysTimer_IRQn, 2);
    ECLIC_SetPriorityIRQ(SysTimer_IRQn, 5);
    ASSERT_EQUAL(ECLIC_GetLevelIRQ(SysTimer_IRQn) <= maxlvl, 1);
    ASSERT_EQUAL(ECLIC_GetPriorityIRQ(SysTimer_IRQn) <= maxpri, 1);

    ECLIC_SetCfgNlbits(oldnlbits);
    ASSERT_EQUAL(ECLIC_GetCfgNlbits(), oldnlbits);
}
fanghuaqi commented 10 months ago

This means that nlbits may be greater than intctlbits?

No, it will not

image

Test CTEST(eclic, lvl_pri_irq)will fail, because ECLIC_SetCfgNlbits(3);, maxlvlwill be 7, but ECLIC_SetLevelIRQ(SysTimer_IRQn, 15);will set SysTimerIRQn level to 3, so ASSERT_EQUAL(ECLIC_GetLevelIRQ(SysTimer_IRQn), maxlvl);will fail.

uint8_t maxlvl = ((1 << nlbits) - 1); if (lvl_abs > maxlvl) { lvl_abs = maxlvl; } lvl_abs will never above maxlvl, controlled by here

yinqingw commented 10 months ago

For first one, now my CLICINTCTLBITS == 2implemented by the hardware, but my nlbits could be set to 3(ECLIC_SetCfgNlbits(3);in functionCTEST(eclic, lvl_pri_irq)). In the case, nlbits will be greater than intctlbits, and CTEST(eclic, lvl_pri_irq)) will fail.

I think it's possible to add a protection in the function void get_max_lvl_pri(uint8_t* maxpri, uint8_t* maxlvl) as follow:

void get_max_lvl_pri(uint8_t* maxpri, uint8_t* maxlvl)
{
    uint8_t nlbits = __ECLIC_GetCfgNlbits();
    uint8_t intctlbits = (uint8_t)__ECLIC_INTCTLBITS;
    *maxpri = 0;
    *maxlvl = 0;

    if (nlbits < intctlbits) {
        *maxpri = ((1 << (intctlbits - nlbits)) - 1);
        *maxlvl = (1 << nlbits) - 1;
    }
    else{
        *maxlvl = (1 << intctlbits) - 1;
    }
}
fanghuaqi commented 10 months ago

For first one, now my CLICINTCTLBITS == 2 implemented by the hardware, but my nlbits could be set to 3(ECLIC_SetCfgNlbits(3);

Okay, checked with hw, you can write nlbits bigger than CLICINTCTLBITS, so I will add protection code to protect nlbits, thank you.

yinqingw commented 10 months ago

ok, Thanks for your patient reply.

fanghuaqi commented 10 months ago

Hi @yinqingw , fixed it in the develop branch, but you need gcc13 toolchain to test it, see #53

fanghuaqi commented 10 months ago

Close it since it is fixed in e96376be1a2a73b7df27fd9f3c620e8a6b5a89f5