Re: [edk2-devel] [PATCH] OvmfPkg/QemuVideoDxe: Shouldn't assume system in VGA alias mode.

["Marc W Chen" <marc.w.chen@intel.com>]

> -----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)) {
> >
> 
> 
>