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

[Peter Maydell]

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.

thanks
-- PMM