From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web11.8443.1582727010421605641 for ; Wed, 26 Feb 2020 06:23:30 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Pk3B4C+7; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: philmd@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1582727009; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=nbw+ziacXKYjYv4eAd2W5C8dHdsEadlxoUqqgzp1bx4=; b=Pk3B4C+7c8W0W0Rxzjg+oC5MDiUyBYr+5eEkntW43GDzVCPOuWIFtVDKccNHHUbx2EADtb qXxt6LHEXzZJ6h8Lk88Tbo9M39ExAjlo3SuEyXa8Hr3KWbZ9p1T9W5A3g50MiquMYAz5B3 fToAqD84l3UhjofxZn0We2KSgfK9KlU= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-194-C8q5xaJbN4aIPELnUPrp1Q-1; Wed, 26 Feb 2020 09:23:22 -0500 X-MC-Unique: C8q5xaJbN4aIPELnUPrp1Q-1 Received: by mail-wr1-f72.google.com with SMTP id h4so1543274wrp.13 for ; Wed, 26 Feb 2020 06:23:22 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=W+n/HuubQUOVCI0NdOXsWD/CS9NAX+yfxjF6SHdixLI=; b=OjVx9Ud8AL1090BXSo7HFMud9hSTqHKxKal5e1O46GSbs+ZawACO9cuQR7u0kEkALF QBEFkcMzdH+7zoaVL51JeLoXvlGSDB0wWKSn/WzEgewfxiWWgAPfSI57wnNu/97ocH4j 1M7epKehVeTx8zjYiTjaQ7KrRUSzc2PT0bm72+G/DlcM4Dc706sEnFP0AVZiJPC5RDfb rgqKGRI0qwg4XbO135kYVUZEqQzytNGRiH0fk5NIrN4m3fUxmhqgJedb7QZKIfvhhzdC 8oxyXZ7ewRpO37l4ux49r/btpz2DOlRuoIg7oFFNTA2QSY3riYh8cReh+ZqD5r4fbUjp TiXQ== X-Gm-Message-State: APjAAAUciOO+CRtL/kXu18gO2KwA+Gl1kRiyEk7hEMHHfnSdVfy44HKr +HjS3NjxSUKHLCwfUNMnp+/aK1zOf3omalyWORx7jcHQk/sQY+l0PPVTAeV/4GTnRf6iar1IoIx uOb+GVUK9odMbLQ== X-Received: by 2002:adf:e40f:: with SMTP id g15mr5747349wrm.223.1582727000664; Wed, 26 Feb 2020 06:23:20 -0800 (PST) X-Google-Smtp-Source: APXvYqz1BWveNVB7OGmEE/SJyeCCzO5VkCY+5KrwVcpW2dQN6megREpx9Um3h0DLbm2JdyYgHtiRiQ== X-Received: by 2002:adf:e40f:: with SMTP id g15mr5747324wrm.223.1582727000364; Wed, 26 Feb 2020 06:23:20 -0800 (PST) Return-Path: Received: from [192.168.1.35] (47.red-88-21-205.staticip.rima-tde.net. [88.21.205.47]) by smtp.gmail.com with ESMTPSA id k7sm3171236wrq.12.2020.02.26.06.23.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 26 Feb 2020 06:23:19 -0800 (PST) Subject: Re: [PATCH edk2-stable202002] OvmfPkg/QemuVideoDxe: unbreak "secondary-vga" and "bochs-display" support To: Laszlo Ersek , edk2-devel-groups-io Cc: Ard Biesheuvel , Gerd Hoffmann , Jordan Justen , Marc W Chen References: <20200224171741.7494-1-lersek@redhat.com> <8bd42e8f-20cd-64c9-eba0-deeee5d370a9@redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Message-ID: Date: Wed, 26 Feb 2020 15:23:19 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable On 2/25/20 11:02 PM, Laszlo Ersek wrote: > On 02/25/20 21:51, Philippe Mathieu-Daud=C3=A9 wrote: >> Hi Laszlo, >> >> On 2/24/20 6:17 PM, Laszlo Ersek wrote: >>> In edk2 commit 333f32ec23dd, QemuVideoDxe gained support for QEMU's >>> "secondary-vga" device model (originally introduced in QEMU commit >>> 63e3e24db2e9). >>> >>> In QEMU commit 765c94290863, the "bochs-display" device was introduced, >>> which would work with QemuVideoDxe out of the box, reusing the >>> "secondary-vga" logic. >>> >>> Support for both models has been broken since edk2 commit 662bd0da7fd7. >>> Said patch ended up requiring VGA IO Ports -- i.e., at least one of >>> EFI_PCI_IO_ATTRIBUTE_VGA_IO and EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 -- even = if >>> the device wasn't actually VGA compatible. >>> >>> Restrict the IO Ports requirement to VGA compatible devices. >>> >>> Cc: Ard Biesheuvel >>> Cc: Gerd Hoffmann >>> Cc: Jordan Justen >>> Cc: Marc W Chen >>> Cc: Philippe Mathieu-Daud=C3=A9 >>> Fixes: 662bd0da7fd77e4d2cf9ef4a78015af5cad7d9db >>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2555 >>> Signed-off-by: Laszlo Ersek >>> --- >>> >>> Notes: >>> =C2=A0=C2=A0=C2=A0=C2=A0 Repo:=C2=A0=C2=A0 https://github.com/lersek/e= dk2.git >>> =C2=A0=C2=A0=C2=A0=C2=A0 Branch: vga_io_bz_2555 >>> >>> =C2=A0 OvmfPkg/QemuVideoDxe/Driver.c | 2 +- >>> =C2=A0 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/OvmfPkg/QemuVideoDxe/Driver.c >>> b/OvmfPkg/QemuVideoDxe/Driver.c >>> index 6a4a860b3c25..37bbbbe843c9 100644 >>> --- a/OvmfPkg/QemuVideoDxe/Driver.c >>> +++ b/OvmfPkg/QemuVideoDxe/Driver.c >>> @@ -292,7 +292,7 @@ QemuVideoControllerDriverStart ( >>> =C2=A0=C2=A0=C2=A0 } >>> =C2=A0 =C2=A0=C2=A0=C2=A0 SupportedVgaIo &=3D (UINT64)(EFI_PCI_IO_ATTR= IBUTE_VGA_IO | >>> EFI_PCI_IO_ATTRIBUTE_VGA_IO_16); >>> -=C2=A0 if (SupportedVgaIo =3D=3D 0) { >>> +=C2=A0 if (SupportedVgaIo =3D=3D 0 && IS_PCI_VGA (&Pci)) { >> >> I'm having hard time to understand. Before we could have a video PCI >> controller which was not VGA? >=20 > Yes, exactly. >=20 > PCI devices have a particular field in their config spaces, which is > described as follows, in "MdePkg/Include/IndustryStandard/Pci22.h": >=20 >=20 > /// > /// Common header region in PCI Configuration Space > /// Section 6.1, PCI Local Bus Specification, 2.2 > /// > typedef struct { > UINT16 VendorId; > UINT16 DeviceId; > UINT16 Command; > UINT16 Status; > UINT8 RevisionID; > UINT8 ClassCode[3]; <---------- this one > UINT8 CacheLineSize; > UINT8 LatencyTimer; > UINT8 HeaderType; > UINT8 BIST; > } PCI_DEVICE_INDEPENDENT_REGION; >=20 >=20 > In the same header file, the IS_PCI_VGA() macro compares: > - ClassCode[0] =3D=3D PCI_IF_VGA_VGA (interface) && > - ClassCode[1] =3D=3D PCI_CLASS_DISPLAY_VGA (subclass ) && > - ClassCode[2] =3D=3D PCI_CLASS_DISPLAY (class) >=20 > While the IS_PCI_DISPLAY() macro compares: > - ClassCode[2] =3D=3D PCI_CLASS_DISPLAY (class) >=20 > So IS_PCI_DISPLAY() is more generic, IS_PCI_VGA() is more specific. OK. >=20 > QEMU provides a number of video devices that satisfy IS_PCI_DISPLAY(), > but not IS_PCI_VGA(). Examples are: >=20 > - ramfb (driven by RamfbDxe) >=20 > - virtio-gpu-pci (driver by VirtioGpuDxe) >=20 > - secondary-vga and bochs-display (driven by QemuVideoDxe, and now broken= ) I think I had some misunderstanding here. >=20 >=20 > It might help if you review the following commits: >=20 > [1] 4fdb585c69d6 ("OvmfPkg/PlatformBootManagerLib: relax device class > requirement for ConOut", 2016-09-01) >=20 > [2] 70dbd16361ee ("OvmfPkg/QemuVideoDxe: Add SubClass field to > QEMU_VIDEO_CARD", 2018-05-17) >=20 > [3] 333f32ec23dd ("OvmfPkg/QemuVideoDxe: Enable DISPLAY_OTHER pci class > for qemu stdvga", 2018-05-17) >=20 > [4] 662bd0da7fd7 ("OvmfPkg/QemuVideoDxe: Shouldn't assume system in VGA > alias mode.", 2019-06-06) -- i.e., the regression. >=20 > Commit [1] is not extremely relevant here, it just demonstrates the > usage of the IS_PCI_VGA() vs. IS_PCI_DISPLAY() macros. Relaxing the > condition in "OvmfPkg/PlatformBootManagerLib" from the former to the > latter meant that OVMF would automatically pick up virtio-gpu-pci > devices in ConOut. (The practical consequence is that the UEFI console, > such as the UEFI shell, grub, etc, are multiplexed to virtio-gpu-pci > devices too.) >=20 > In commits [2] and [3], Gerd extended QemuVideoDxe to handle > "secondary-vga". Because, "secondary-vga" has PCI_CLASS_DISPLAY_OTHER > (not PCI_CLASS_DISPLAY_VGA) for ClassCode[1] (i.e., subclass). So it > won't satisfy IS_PCI_VGA(). For ClassCode[2] -- i.e., class -- > "secondary-vga" still has PCI_CLASS_DISPLAY, so it will satisfy > IS_PCI_DISPLAY(). >=20 > Some time later, Gerd added the "bochs-display" device to QEMU, which > would exercise the exact same code path in QemuVideoDxe as "secondary-vga= ". >=20 >=20 > And, finally, while reviewing commit [4], I missed that it changed two > things at once, one intentionally, and another one unintentionally. >=20 > The intentional change was that rather than hardcoding > EFI_PCI_IO_ATTRIBUTE_VGA_IO in the "EfiPciIoAttributeOperationEnable" > operation, we would first ask the device about > (EFI_PCI_IO_ATTRIBUTE_VGA_IO | EFI_PCI_IO_ATTRIBUTE_VGA_IO_16), with > EfiPciIoAttributeOperationSupported, and then pass whichever was > available to EfiPciIoAttributeOperationEnable. This change was good. >=20 > However, the following hunk: >=20 > + if (SupportedVgaIo =3D=3D 0) { > + Status =3D EFI_UNSUPPORTED; > + goto ClosePciIo; > + } >=20 > was wrong. Because, for such devices that satisfy IS_PCI_DISPLAY(), but > not IS_PCI_VGA() -- namely: "secondary-vga" and "bochs-display" -- > *neither* of EFI_PCI_IO_ATTRIBUTE_VGA_IO and > EFI_PCI_IO_ATTRIBUTE_VGA_IO_16 can be expected. So when we bail out with > EFI_UNSUPPORTED if both attributes are missing (as reported by the > device), we deny driving "secondary-vga" and "bochs-display", even > though they used to work just fine. >=20 > The present patch restricts the EFI_UNSUPPORTED branch to such devices > (i.e., to PCI display devices with actual VGA cruft) where at least one > of those attributes would be rightfully required. Thanks a lot for the full explanation. Hopefully it will help others too :) Now that I understand your patch: Reviewed-by: Philippe Mathieu-Daude >=20 >> >> What about the other IS_PCI_OLD_VGA() macro? >=20 > It seems irrelevant here -- the class, subclass and interface codes that > we need to check for in OVMF are dictated by the QEMU device models, in > the end. So unless a QEMU device advertizes PCI_CLASS_OLD for "class", > and PCI_CLASS_OLD_VGA for "subclass", the macro is irrelevant to OVMF. >=20 > Thanks! > Laszlo >=20 >> >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Status =3D EFI_UNSUPPORTED; >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto ClosePciIo; >>> =C2=A0=C2=A0=C2=A0 } >>> >>> base-commit: 1d3215fd24f47eaa4877542a59b4bbf5afc0cfe8 >>> >> >=20