> -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Laszlo Ersek > Sent: Wednesday, June 5, 2019 8:00 PM > To: Chen, Marc W <marc.w.c...@intel.com>; devel@edk2.groups.io > Cc: Justen, Jordan L <jordan.l.jus...@intel.com>; Ard Biesheuvel > <ard.biesheu...@linaro.org>; Anthony Perard <anthony.per...@citrix.com>; > Julien Grall <julien.gr...@arm.com>; Marc-André Lureau > <marcandre.lur...@redhat.com>; Stefan Berger <stef...@linux.ibm.com> > Subject: Re: [edk2-devel] [PATCH] OvmfPkg/QemuVideoDxe: Shouldn't > assume system in VGA alias mode. > > 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.
Thanks for the feedback, yes, I mean "bitwise AND", I'll correct it in my commit message. > > > 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? Yes, we should consider both since the mReserveVgaAliases in PciBusDxe driver is default FALSE(implies that device driver can only set VGA_IO_16 to PCI_ROOT_BRIDGE), and Platform code may not return EFI_RESERVE_VGA_IO_ALIAS in GetPlatformPolicy of PciPlatformProtocol to make mReserveVgaAliases become TRUE(implies that device driver can only set VGA_IO to PCI_ROOT_BRIDGE), currently OvmfPkg doesn't have issue due to it has hard code value in PCI_ROOT_BRIDGE's attribute field, so an IO read by PciIoProtocol will be success due to RootBridgeIoCheckParameter of PciRootBridgeIo.c will always get pass result for legacy IO access. But in our design(Edk2Platform:Platform\Intel\MinPlatformPkg), we only set Supports field of PCI_ROOT_BRIDGE, and keep Attributes field of PCI_ROOT_BRIDGE as 0, and let device driver to update the attribute of PCI_ROOT_BRIDGE for IO accessing, in that case, if BIOS doesn't return EFI_RESERVE_VGA_IO_ALIAS in GetPlatformPolicy of PciPlatformProtocol, then only VGA_IO_16 can be enabled by device driver, so this QemuVideoDriver failed to do IO access in our case. I believe if you change the Attributes filed of PCI_ROOT_BRIDGE of OvmfPkg to 0, then you should be able to see the issue. > > (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? > Yes, please refer to my above explanation. > 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. Yes, please refer to my above explanation, I'll update my commit message to describe it more detail. > > > > > Signed-off-by: Marc Chen <marc.w.c...@intel.com> > > Cc: Jordan Justen <jordan.l.jus...@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> > > Cc: Anthony Perard <anthony.per...@citrix.com> > > Cc: Julien Grall <julien.gr...@arm.com> > > Cc: Marc-André Lureau <marcandre.lur...@redhat.com> > > Cc: Stefan Berger <stef...@linux.ibm.com> > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1880 > > --- > > OvmfPkg/QemuVideoDxe/Driver.c | 22 +++++++++++++++++++++- > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/OvmfPkg/QemuVideoDxe/Driver.c > b/OvmfPkg/QemuVideoDxe/Driver.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; > > > > OldTpl = gBS->RaiseTPL (TPL_CALLBACK); > > > > @@ -277,13 +278,32 @@ QemuVideoControllerDriverStart ( > > goto ClosePciIo; > > } > > > > + // > > + // Get supported PCI attributes > > + // > > + Status = Private->PciIo->Attributes ( > > + Private->PciIo, > > + EfiPciIoAttributeOperationSupported, > > + 0, > > + &Supports > > + ); > > (4) The arguments and the closing parenthesis are incorrectly aligned, > relative to the function designator. I'll update in next patch. > > > > + if (EFI_ERROR (Status)) { > > + goto ClosePciIo; > > + } > > + > > + Supports &= (UINT64)(EFI_PCI_IO_ATTRIBUTE_VGA_IO | > EFI_PCI_IO_ATTRIBUTE_VGA_IO_16); > > + if ((Supports == 0) || (Supports == (EFI_PCI_IO_ATTRIBUTE_VGA_IO | > EFI_PCI_IO_ATTRIBUTE_VGA_IO_16))) { > > + Status = 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. I'll update in next patch. > > (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*. As my above explanation, when Attributes field of PCI_ROOT_BRIDGE is 0, if BIOS doesn't return EFI_RESERVE_VGA_IO_ALIAS in GetPlatformPolicy of PciPlatformProtocol, then only VGA_IO_16 can be enabled by device driver, if BIOS return EFI_RESERVE_VGA_IO_ALIAS in GetPlatformPolicy of PciPlatformProtocol, then only VGA_IO can be enabled by device driver, you can check the PciSetDeviceAttribute of PciEnumeratorSupport.c of PciBusDxe driver for detail. > > 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 for your reply, I think your first option is good enough since PciBusDxe driver has guaranteed this exclusion. > > Thanks, > Laszlo > > > // > > // Set new PCI attributes > > // > > Status = Private->PciIo->Attributes ( > > Private->PciIo, > > EfiPciIoAttributeOperationEnable, > > - EFI_PCI_DEVICE_ENABLE | > EFI_PCI_IO_ATTRIBUTE_VGA_MEMORY | EFI_PCI_IO_ATTRIBUTE_VGA_IO, > > + EFI_PCI_DEVICE_ENABLE | > EFI_PCI_IO_ATTRIBUTE_VGA_MEMORY | Supports, > > NULL > > ); > > if (EFI_ERROR (Status)) { > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#41976): https://edk2.groups.io/g/devel/message/41976 Mute This Topic: https://groups.io/mt/31935803/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
