* [PATCH] OvmfPkg/QemuVideoDxe: Shouldn't assume system in VGA alias mode. @ 2019-06-05 11:14 Marc W Chen 2019-06-05 12:00 ` Laszlo Ersek 0 siblings, 1 reply; 5+ messages in thread From: Marc W Chen @ 2019-06-05 11:14 UTC (permalink / raw) To: devel Cc: Marc Chen, Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Anthony Perard, Julien Grall, Marc-André Lureau, Stefan Berger Query the supported attributes firstly, then AND (&&) both VGA_IO and VGA_IO_16. 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. Signed-off-by: Marc Chen <marc.w.chen@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Anthony Perard <anthony.perard@citrix.com> Cc: Julien Grall <julien.grall@arm.com> Cc: Marc-André Lureau <marcandre.lureau@redhat.com> Cc: Stefan Berger <stefanb@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 + ); + 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; + } + // // 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)) { -- 2.16.2.windows.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] OvmfPkg/QemuVideoDxe: Shouldn't assume system in VGA alias mode. 2019-06-05 11:14 [PATCH] OvmfPkg/QemuVideoDxe: Shouldn't assume system in VGA alias mode Marc W Chen @ 2019-06-05 12:00 ` Laszlo Ersek 2019-06-05 17:15 ` [edk2-devel] " Laszlo Ersek 2019-06-06 4:18 ` Marc W Chen 0 siblings, 2 replies; 5+ messages in thread From: Laszlo Ersek @ 2019-06-05 12:00 UTC (permalink / raw) To: Marc W Chen, devel Cc: Jordan Justen, Ard Biesheuvel, Anthony Perard, Julien Grall, Marc-André Lureau, Stefan Berger 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. > > Signed-off-by: Marc Chen <marc.w.chen@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Anthony Perard <anthony.perard@citrix.com> > Cc: Julien Grall <julien.grall@arm.com> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com> > Cc: Stefan Berger <stefanb@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. > + 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. (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 = 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)) { > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH] OvmfPkg/QemuVideoDxe: Shouldn't assume system in VGA alias mode. 2019-06-05 12:00 ` Laszlo Ersek @ 2019-06-05 17:15 ` Laszlo Ersek 2019-06-06 4:18 ` Marc W Chen 1 sibling, 0 replies; 5+ messages in thread From: Laszlo Ersek @ 2019-06-05 17:15 UTC (permalink / raw) To: Marc W Chen, devel Cc: Jordan Justen, Ard Biesheuvel, Anthony Perard, Julien Grall, Marc-André Lureau, Stefan Berger On 06/05/19 14:00, Laszlo Ersek wrote: > 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. On second thought, you do mean "logical AND" above, it's just that the wording is a bit difficult to understand (to me anyway). So please ignore this part of my review (but please answer the rest of the questions). Thanks Laszlo ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH] OvmfPkg/QemuVideoDxe: Shouldn't assume system in VGA alias mode. 2019-06-05 12:00 ` Laszlo Ersek 2019-06-05 17:15 ` [edk2-devel] " Laszlo Ersek @ 2019-06-06 4:18 ` Marc W Chen 1 sibling, 0 replies; 5+ messages in thread From: Marc W Chen @ 2019-06-06 4:18 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com Cc: Justen, Jordan L, Ard Biesheuvel, Anthony Perard, Julien Grall, Marc-André Lureau, Stefan Berger > -----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.chen@intel.com>; devel@edk2.groups.io > Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel > <ard.biesheuvel@linaro.org>; Anthony Perard <anthony.perard@citrix.com>; > Julien Grall <julien.grall@arm.com>; Marc-André Lureau > <marcandre.lureau@redhat.com>; Stefan Berger <stefanb@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.chen@intel.com> > > Cc: Jordan Justen <jordan.l.justen@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Cc: Anthony Perard <anthony.perard@citrix.com> > > Cc: Julien Grall <julien.grall@arm.com> > > Cc: Marc-André Lureau <marcandre.lureau@redhat.com> > > Cc: Stefan Berger <stefanb@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)) { > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] OvmfPkg/QemuVideoDxe: Shouldn't assume system in VGA alias mode. @ 2019-06-05 10:55 Marc W Chen 0 siblings, 0 replies; 5+ messages in thread From: Marc W Chen @ 2019-06-05 10:55 UTC (permalink / raw) To: devel Cc: Marc Chen, Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Anthony Perard, Julien Grall, Marc-André Lureau, Stefan Berger Query the supported attributes firstly, then AND (&&) both VGA_IO and VGA_IO_16. 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. Signed-off-by: Marc Chen <marc.w.chen@intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Anthony Perard <anthony.perard@citrix.com> Cc: Julien Grall <julien.grall@arm.com> Cc: Marc-André Lureau <marcandre.lureau@redhat.com> Cc: Stefan Berger <stefanb@linux.ibm.com> --- 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 + ); + 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; + } + // // 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)) { -- 2.16.2.windows.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-06-06 4:18 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-06-05 11:14 [PATCH] OvmfPkg/QemuVideoDxe: Shouldn't assume system in VGA alias mode Marc W Chen 2019-06-05 12:00 ` Laszlo Ersek 2019-06-05 17:15 ` [edk2-devel] " Laszlo Ersek 2019-06-06 4:18 ` Marc W Chen -- strict thread matches above, loose matches on Subject: below -- 2019-06-05 10:55 Marc W Chen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox