svn commit: r329360 - in head/sys: amd64/vmm/amd contrib/dev/acpica/include

[Conrad Meyer]

Hi Anish,

Some coverity nits inline:

On Thu, Feb 15, 2018 at 9:17 PM, Anish Gupta <anish at freebsd.org> wrote:
> Author: anish
> Date: Fri Feb 16 05:17:00 2018
> New Revision: 329360
> URL: https://svnweb.freebsd.org/changeset/base/329360
>
> Log:
>   This change fixes duplicate detection of same IOMMU/AMD-Vi device for Ryzen with EFR support.
>
>   IVRS can have entry of type legacy and non-legacy present at same time for same AMD-Vi device. ivhd driver will ignore legacy if new IVHD type is present as specified in AMD-Vi specification. Earlier both of IVHD entries used and two ivhd devices were created.
>   Add support for new IVHD type 0x11 and 0x40 in ACPI. Create new struct of type acpi_ivrs_hardware_new for these new type of IVHDs. Legacy type 0x10 will continue to use acpi_ivrs_hardware.
>
>   Reviewed by:  avg
>   Approved by:  grehan
>   Differential Revision:https://reviews.freebsd.org/D13160
>
> Modified:
>   head/sys/amd64/vmm/amd/amdvi_hw.c
>   head/sys/amd64/vmm/amd/amdvi_priv.h
>   head/sys/amd64/vmm/amd/ivrs_drv.c
>   head/sys/contrib/dev/acpica/include/actbl2.h
>
> ...
> Modified: head/sys/amd64/vmm/amd/ivrs_drv.c
> ==============================================================================
> --- head/sys/amd64/vmm/amd/ivrs_drv.c   Fri Feb 16 04:59:21 2018        (r329359)
> +++ head/sys/amd64/vmm/amd/ivrs_drv.c   Fri Feb 16 05:17:00 2018        (r329360)
> ...
> @@ -196,11 +205,26 @@ ivhd_dev_parse(ACPI_IVRS_HARDWARE * ivhd, struct amdvi
>         softc->start_dev_rid = ~0;
>         softc->end_dev_rid = 0;
>
> -       /*
> -        * XXX The following actually depends on Header.Type and
> -        * is only true for 0x10.
> -        */
> -       p = (uint8_t *)ivhd + sizeof(ACPI_IVRS_HARDWARE);
> +       switch (ivhd->Header.Type) {
> +               case ACPI_IVRS_TYPE_HARDWARE_EXT1:
> +               case ACPI_IVRS_TYPE_HARDWARE_EXT2:
> +                       p = (uint8_t *)ivhd + sizeof(ACPI_IVRS_HARDWARE_NEW);
> +                       de = (ACPI_IVRS_DE_HEADER *) ((uint8_t *)ivhd +
> +                               sizeof(ACPI_IVRS_HARDWARE_NEW));
> +                       break;
> +
> +               case ACPI_IVRS_TYPE_HARDWARE:
> +                       p = (uint8_t *)ivhd + sizeof(ACPI_IVRS_HARDWARE);
> +                       de = (ACPI_IVRS_DE_HEADER *) ((uint8_t *)ivhd +
> +                               sizeof(ACPI_IVRS_HARDWARE));


Coverity points out that initializing 'de' in these cases is
pointless, as the value is never used before it is overridden in the
immediately subsequent loop.


> ...
> @@ -285,14 +309,30 @@ ivhd_dev_parse(ACPI_IVRS_HARDWARE * ivhd, struct amdvi
>         return (0);
>  }
>
> +static bool
> +ivhd_is_newer(ACPI_IVRS_HEADER *old, ACPI_IVRS_HEADER  *new)
> +{
> +       /*
> +        * Newer IVRS header type take precedence.
> +        */
> +       if ((old->DeviceId == new->DeviceId) &&
> +               (old->Type == ACPI_IVRS_TYPE_HARDWARE) &&
> +               ((new->Type == ACPI_IVRS_TYPE_HARDWARE_EXT1) ||
> +               (new->Type == ACPI_IVRS_TYPE_HARDWARE_EXT1))) {


Coverity also points out that both sides of this OR are the same,
ACPI_IVRS_TYPE_HARDWARE_EXT1.  Logically this is redundant but
probably indicates a typo?  Perhaps one should be
ACPI_IVRS_TYPE_HARDWARE_EXT2?


Thanks,
Conrad