Re: [PULL 26/45] ACPI: Record Generic Error Status Block(GESB) table

[Dongjiu Geng]

On 2020/6/20 1:21, Peter Maydell wrote:
> On Thu, 21 May 2020 at 16:31, Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Thu, May 21, 2020 at 02:03:36PM +0100, Peter Maydell wrote:
>>> On Thu, 14 May 2020 at 15:22, Peter Maydell <peter.maydell@linaro.org> 
>>> wrote:
>>>>
>>>> From: Dongjiu Geng <gengdongjiu@huawei.com>
>>>>
>>>> kvm_arch_on_sigbus_vcpu() error injection uses source_id as
>>>> index in etc/hardware_errors to find out Error Status Data
>>>> Block entry corresponding to error source. So supported source_id
>>>> values should be assigned here and not be changed afterwards to
>>>> make sure that guest will write error into expected Error Status
>>>> Data Block.
>>>>
>>>> Before QEMU writes a new error to ACPI table, it will check whether
>>>> previous error has been acknowledged. If not acknowledged, the new
>>>> errors will be ignored and not be recorded. For the errors section
>>>> type, QEMU simulate it to memory section error.
>>>
>>> Hi; Coverity points out (CID 1428962) that there is
>>> unreachable code in this function:
>>>
>>>> +static int acpi_ghes_record_mem_error(uint64_t error_block_address,
>>>> +                                      uint64_t error_physical_addr)
>>>> +{
>>>> +    GArray *block;
>>>> +
>>>> +    /* Memory Error Section Type */
>>>> +    const uint8_t uefi_cper_mem_sec[] =
>>>> +          UUID_LE(0xA5BC1114, 0x6F64, 0x4EDE, 0xB8, 0x63, 0x3E, 0x83, \
>>>> +                  0xED, 0x7C, 0x83, 0xB1);
>>>> +
>>>> +    /* invalid fru id: ACPI 4.0: 17.3.2.6.1 Generic Error Data,
>>>> +     * Table 17-13 Generic Error Data Entry
>>>> +     */
>>>> +    QemuUUID fru_id = {};
>>>> +    uint32_t data_length;
>>>> +
>>>> +    block = g_array_new(false, true /* clear */, 1);
>>>> +
>>>> +    /* This is the length if adding a new generic error data entry*/
>>>> +    data_length = ACPI_GHES_DATA_LENGTH + ACPI_GHES_MEM_CPER_LENGTH;
>>>
>>> Here data_length has a constant value...
>>>
>>>> +
>>>> +    /*
>>>> +     * Check whether it will run out of the preallocated memory if adding 
>>>> a new
>>>> +     * generic error data entry
>>>> +     */
>>>> +    if ((data_length + ACPI_GHES_GESB_SIZE) > 
>>>> ACPI_GHES_MAX_RAW_DATA_LENGTH) {
>>>
>>> ...but here we immediately have a runtime check which can't possibly
>>> fail because of the values of the constants involved, so this
>>> if() block is dead code.
>>>
>>>> +        error_report("Not enough memory to record new CPER!!!");
>>>> +        g_array_free(block, true);
>>>> +        return -1;
>>>> +    }
>>>
>>> What was this code trying to do? Is the initial value of
>>> data_length incorrect, or is the if() condition wrong, or
>>> should this simply have been an assert() ?
> 
>> It's just a validity check. assert will do just as well.
> 
> Would somebody like to write a patch to make it assert instead, then,
> please? That should keep Coverity happy.
  I will check the comments history and make a patch, thanks a lot.

> 
> thanks
> -- PMM
> 
> .
>