Date: Tue, 20 Feb 2018 11:57:28 -0800 From: Conrad Meyer <cem@freebsd.org> To: Anish Gupta <anish@freebsd.org> Cc: src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r329360 - in head/sys: amd64/vmm/amd contrib/dev/acpica/include Message-ID: <CAG6CVpWzTefiecmi_G7u5FCEieVQKfwR3H8mN2DjZFo0TBzw1w@mail.gmail.com> In-Reply-To: <201802160517.w1G5H1XH047278@repo.freebsd.org> References: <201802160517.w1G5H1XH047278@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Anish, Some coverity nits inline: On Thu, Feb 15, 2018 at 9:17 PM, Anish Gupta <anish@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 R= yzen 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 0x1= 0 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 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > --- 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 am= dvi > softc->start_dev_rid =3D ~0; > softc->end_dev_rid =3D 0; > > - /* > - * XXX The following actually depends on Header.Type and > - * is only true for 0x10. > - */ > - p =3D (uint8_t *)ivhd + sizeof(ACPI_IVRS_HARDWARE); > + switch (ivhd->Header.Type) { > + case ACPI_IVRS_TYPE_HARDWARE_EXT1: > + case ACPI_IVRS_TYPE_HARDWARE_EXT2: > + p =3D (uint8_t *)ivhd + sizeof(ACPI_IVRS_HARDWARE= _NEW); > + de =3D (ACPI_IVRS_DE_HEADER *) ((uint8_t *)ivhd + > + sizeof(ACPI_IVRS_HARDWARE_NEW)); > + break; > + > + case ACPI_IVRS_TYPE_HARDWARE: > + p =3D (uint8_t *)ivhd + sizeof(ACPI_IVRS_HARDWARE= ); > + de =3D (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 am= dvi > return (0); > } > > +static bool > +ivhd_is_newer(ACPI_IVRS_HEADER *old, ACPI_IVRS_HEADER *new) > +{ > + /* > + * Newer IVRS header type take precedence. > + */ > + if ((old->DeviceId =3D=3D new->DeviceId) && > + (old->Type =3D=3D ACPI_IVRS_TYPE_HARDWARE) && > + ((new->Type =3D=3D ACPI_IVRS_TYPE_HARDWARE_EXT1) || > + (new->Type =3D=3D 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAG6CVpWzTefiecmi_G7u5FCEieVQKfwR3H8mN2DjZFo0TBzw1w>