From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Wed, 05 Jun 2019 05:00:35 -0700 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0C30C30832C9; Wed, 5 Jun 2019 12:00:24 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-4.ams2.redhat.com [10.36.117.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6E72519741; Wed, 5 Jun 2019 12:00:08 +0000 (UTC) Subject: Re: [PATCH] OvmfPkg/QemuVideoDxe: Shouldn't assume system in VGA alias mode. To: Marc W Chen , devel@edk2.groups.io Cc: Jordan Justen , Ard Biesheuvel , Anthony Perard , Julien Grall , =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= , Stefan Berger References: <20190605111410.73564-1-marc.w.chen@intel.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 5 Jun 2019 14:00:07 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190605111410.73564-1-marc.w.chen@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.44]); Wed, 05 Jun 2019 12:00:34 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 06/05/19 13:14, Marc W Chen wrote: > Query the supported attributes firstly, then AND (&&) both > VGA_IO and VGA_IO_16. I don't think you mean "logical AND" above. And if you mean "bitwise AND", then "&&" is incorrect. > Since the supported attributes should > only have VGA_IO or VGA_IO_16 set, the result of AND (&&) is > either VGA_IO or IO_16. Then the result can be passed to > PciIo->Attributes() to set the attributes. I don't understand what the problem is. In fact, do we have two problems? Such as, (1) We don't consider VGA_IO_16, only VGA_IO. Are you saying that we should consider both, and (at the same time) make sure that at most one is set? (2) We don't check whether VGA_IO* is supported, we just go ahead and enable them. Are you saying that we should check before we enable? Furthermore, (3) did you encounter an practical problem with the current code? If so, can you describe it in the commit message please? The subject line helps a little (it implies that using VGA_IO over VGA_IO_16 assumes "VGA alias mode"), but it should be described in more detail please. >=20 > Signed-off-by: Marc Chen > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Cc: Anthony Perard > Cc: Julien Grall > Cc: Marc-Andr=C3=A9 Lureau > Cc: Stefan Berger > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1880 > --- > OvmfPkg/QemuVideoDxe/Driver.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) >=20 > diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Drive= r.c > index e8a613ef33..ba9210f24b 100644 > --- a/OvmfPkg/QemuVideoDxe/Driver.c > +++ b/OvmfPkg/QemuVideoDxe/Driver.c > @@ -201,6 +201,7 @@ QemuVideoControllerDriverStart ( > PCI_TYPE00 Pci; > QEMU_VIDEO_CARD *Card; > EFI_PCI_IO_PROTOCOL *ChildPciIo; > + UINT64 Supports; > =20 > OldTpl =3D gBS->RaiseTPL (TPL_CALLBACK); > =20 > @@ -277,13 +278,32 @@ QemuVideoControllerDriverStart ( > goto ClosePciIo; > } > =20 > + // > + // Get supported PCI attributes > + // > + Status =3D Private->PciIo->Attributes ( > + Private->PciIo, > + EfiPciIoAttributeOperationSupported, > + 0, > + &Supports > + ); (4) The arguments and the closing parenthesis are incorrectly aligned, relative to the function designator. > + if (EFI_ERROR (Status)) { > + goto ClosePciIo; > + } > + > + Supports &=3D (UINT64)(EFI_PCI_IO_ATTRIBUTE_VGA_IO | EFI_PCI_IO_ATTR= IBUTE_VGA_IO_16); > + if ((Supports =3D=3D 0) || (Supports =3D=3D (EFI_PCI_IO_ATTRIBUTE_VG= A_IO | EFI_PCI_IO_ATTRIBUTE_VGA_IO_16))) { > + Status =3D EFI_UNSUPPORTED; > + goto ClosePciIo; > + } > + I have two comments on this: (5) If we need a variable for tracking just these two bits, then it should be named something more to-the-point than just "Supports". For example, "SupportedVgaIo" or similar. (6) I'm not convinced this logic is correct, according to the UEFI spec. The spec seems to suggest that (VGA_IO | VGA_IO_16) should not be *enabled* at the same time. However, it doesn't seem to require that a PciIo protocol instance report at most one of these attributes as *supported*. The spec says, near VGA_IO_16, "This bit may not be combined with EFI_PCI_IO_ATTRIBUTE_VGA_IO [...]". I'm unsure whether this applies to both EfiPciIoAttributeOperationSupported and EfiPciIoAttributeOperationEnable. I think we have two options here: - if the spec guarantees this exclusion, then we can either drop the "both set" check, or even ASSERT that it never happens - if the spec does not guarantee the exclusion, then we should pick VGA_IO_16 first, and VGA_IO second (in this priority order). I think the last option is the most robust one. Thanks, Laszlo > // > // Set new PCI attributes > // > Status =3D Private->PciIo->Attributes ( > Private->PciIo, > EfiPciIoAttributeOperationEnable, > - EFI_PCI_DEVICE_ENABLE | EFI_PCI_IO_ATTRIBU= TE_VGA_MEMORY | EFI_PCI_IO_ATTRIBUTE_VGA_IO, > + EFI_PCI_DEVICE_ENABLE | EFI_PCI_IO_ATTRIBU= TE_VGA_MEMORY | Supports, > NULL > ); > if (EFI_ERROR (Status)) { >=20