From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@infradead.org header.s=merlin.20170209 header.b=d4Cpo7J4; spf=none, err=permanent DNS error (domain: merlin.srs.infradead.org, ip: 205.233.59.134, mailfrom: batv+d60b494a64d95ca09138+5779+infradead.org+dwmw2@merlin.srs.infradead.org) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by groups.io with SMTP; Thu, 20 Jun 2019 06:59:00 -0700 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=merlin.20170209; h=Mime-Version:Content-Type:References: In-Reply-To:Date:Cc:To:From:Subject:Message-ID:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=46PASFEq9sAyEimWp0nNo+CTBNITPSxAxfIM+0GQc9c=; b=d4Cpo7J4V2C6RKdQ/uTI70e9t EUZXB7f+0Nchekw68k8WkJNkWvRoh7/Vce9haSP/c078IOh6SjGPiGKWYJhZ1D3uUs8yCIwKozAfI bHltAorl3nfhOpwTYBhmWsgV1Fp4BbRcgtsXcXA6+LvLhDZt5rWv2+C1MLCQDFFprSpMiMsD12A07 4EdwZ5m1xzpgAok32Trrzbbz9YsteBKH7enjQqOjmgxHYh7depN0Jo1qr+76mwyXnh6azxpetEOY4 21nokzoopwO2LtAt+MM9Oj1FabbDpWT7mJTPf+qb69wMjmfx8Nu2yka74us9rMISsJmksngPOkBp8 tS1UAiSkA==; Received: from 54-240-197-228.amazon.com ([54.240.197.228] helo=u3832b3a9db3152.ant.amazon.com) by merlin.infradead.org with esmtpsa (Exim 4.92 #3 (Red Hat Linux)) id 1hdxaZ-0007pD-8L; Thu, 20 Jun 2019 13:58:55 +0000 Message-ID: <42f4314e3545b698dc5294a494a43afdcc205722.camel@infradead.org> Subject: Re: [edk2] [PATCH] OvmfPkg: prevent 64-bit MMIO BAR degradation if there is no CSM From: "David Woodhouse" To: Laszlo Ersek Cc: "devel@edk2.groups.io" , Ray Ni , Jordan Justen , Ard Biesheuvel Date: Thu, 20 Jun 2019 14:58:53 +0100 In-Reply-To: <28b2a68d-ad2f-1137-d356-227ee43db295@redhat.com> References: <1463609573-16626-1-git-send-email-lersek@redhat.com> <9c690717587ff1ccd9a8ec8bd3d741e6c86f8bb3.camel@infradead.org> <01a87fee-5a17-d027-39fd-2a2e0a36787b@redhat.com> <28b2a68d-ad2f-1137-d356-227ee43db295@redhat.com> X-Mailer: Evolution 3.28.5-0ubuntu0.18.04.1 Mime-Version: 1.0 X-SRS-Rewrite: SMTP reverse-path rewritten from by merlin.infradead.org. See http://www.infradead.org/rpr.html X-Groupsio-MsgNum: 42639 Content-Type: multipart/signed; micalg="sha-256"; protocol="application/x-pkcs7-signature"; boundary="=-qjWGj8RFrdi5w/Db3i1n" --=-qjWGj8RFrdi5w/Db3i1n Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 2019-06-20 at 14:57 +0200, Laszlo Ersek wrote: > (updating Ray's email) >=20 > On 06/20/19 00:19, David Woodhouse wrote: > >=20 > > > the driver is thoroughly commented. See especially the > > > DriverInitialize() function. Can you determine which one(s) of those > > > statements doesn't / don't hold any longer? > > >=20 > > > Or maybe IncompatiblePciDeviceSupportDxe works as before, but commit > > > 065ae7d717f9 ("MdeModulePkg/PciBusDxe: make OPROM BAR degradation > > > configurable", 2016-09-26) made a difference? (Adding Ard.) > > >=20 > > > I'm just guessing of course; a bisection could prove more effective. > >=20 > > I think I worked it out. The problem is that the nvme controller doesn'= t > > have a ROM so it wasn't triggering the downgrade to 32-bit in the first > > place. > >=20 > > By hacking IncompatiblePciDeviceSupportDxe to always return configurati= on > > with 32+bit "granularity" I can boot. That does it for *all* devices, o= f > > course... but I don't get the PCI class; only device/vendor IDs. >=20 > Doesn't this imply that we have a more generic problem in PciBusDxe? Potentially so, yes. Although it's not clear the PciBusDxe itself should be making such platform-dependent decisions. Calling out to something like IncompatiblePciDeviceSupportDxe seems like the right decision =E2=80=94 but having an a priori default behaviour based on the presence or otherwise of an OpROM doesn't. Just let the platform code decide. > AIUI, the current aperture degradation (for BAR allocation purposes) is > meant to allow the device's own (specific) legacy option ROM to access > the BARs. If the device has no option ROM, this particular goal falls > away. (And OVMF's driver basically implements an override, in case the > oprom does exist, but "legacy" is whole-sale unsupported by the platform.= ) >=20 > If the generic (device independent) CSM code -- *any* generic CSM in > fact -- is expected to access BARs, then the logic in PciBusDxe is both > too lax and too restrictive at the same time. It's too lax due to > factors discussed previously (see the paragraph above, and commit > 065ae7d717f9), and too restrictive because it misses the "CSM per se > needs BAR access" case. Right. AFAICT the criterion "has OpROM" was never really correct. It should really have been "will be driven by legacy code". Things that are natively supported by the CSM =E2=80=94 including IDE, SATA= , VirtIO and NVMe 'disks' =E2=80=94 all fall into that category whether they = have OpROMs or not. I think we've mostly just got away with it until now because they haven't had 64-bit capable BARs. > For working this around in OVMF, we'd have to change the OVMF driver's > behavior from "force 64-bit, or keep silent", to "force 64-bit, or force > 32-bit". This looks like a kludge that entirely supplants the PciBusDxe > logic. So I'm not a fan of it. Hm, I think the PciBusDxe logic *should* be "supplanted". It never made sense for PciBusDxe to take a guess about what the platform might want, then let the platform 'correct' it. But of course it would be nice if PciBusDxe made a callout to platform code and gave it all the information that might be needed =E2=80=94 like th= e PCI class, and the presence or otherwise of an OpROM.=20 > That said, if you can patch > IncompatiblePciDeviceSupportDxe such that the change only affect the > CSM_ENABLE build of OVMF observably, I guess I can live with it. That's simple enough; we make CheckDevice() always return a configuration, but with the granularity set to 32 if there's a CSM and (as is currently the case) with it set to 64 if there's not. Really, there should be a CSM call to say "do you want this device?", and then the real trigger for degrading to 32-bit should be whether it has an OpRom *or* that call returns true. For now I've resorted to building with--pcd gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size=3D0 =E2=80=94 if we switch from SeaBIOS to OVMF+SeaBIOS then I don't want random 32-bit guests breaking because their BARs are suddenly unreachable, whether the device in question is used by the BIOS or not. --=-qjWGj8RFrdi5w/Db3i1n Content-Type: application/x-pkcs7-signature; name="smime.p7s" Content-Disposition: attachment; filename="smime.p7s" Content-Transfer-Encoding: base64 MIAGCSqGSIb3DQEHAqCAMIACAQExDzANBglghkgBZQMEAgEFADCABgkqhkiG9w0BBwEAAKCCECow ggUcMIIEBKADAgECAhEA4rtJSHkq7AnpxKUY8ZlYZjANBgkqhkiG9w0BAQsFADCBlzELMAkGA1UE BhMCR0IxGzAZBgNVBAgTEkdyZWF0ZXIgTWFuY2hlc3RlcjEQMA4GA1UEBxMHU2FsZm9yZDEaMBgG A1UEChMRQ09NT0RPIENBIExpbWl0ZWQxPTA7BgNVBAMTNENPTU9ETyBSU0EgQ2xpZW50IEF1dGhl bnRpY2F0aW9uIGFuZCBTZWN1cmUgRW1haWwgQ0EwHhcNMTkwMTAyMDAwMDAwWhcNMjIwMTAxMjM1 OTU5WjAkMSIwIAYJKoZIhvcNAQkBFhNkd213MkBpbmZyYWRlYWQub3JnMIIBIjANBgkqhkiG9w0B AQEFAAOCAQ8AMIIBCgKCAQEAsv3wObLTCbUA7GJqKj9vHGf+Fa+tpkO+ZRVve9EpNsMsfXhvFpb8 RgL8vD+L133wK6csYoDU7zKiAo92FMUWaY1Hy6HqvVr9oevfTV3xhB5rQO1RHJoAfkvhy+wpjo7Q cXuzkOpibq2YurVStHAiGqAOMGMXhcVGqPuGhcVcVzVUjsvEzAV9Po9K2rpZ52FE4rDkpDK1pBK+ uOAyOkgIg/cD8Kugav5tyapydeWMZRJQH1vMQ6OVT24CyAn2yXm2NgTQMS1mpzStP2ioPtTnszIQ Ih7ASVzhV6csHb8Yrkx8mgllOyrt9Y2kWRRJFm/FPRNEurOeNV6lnYAXOymVJwIDAQABo4IB0zCC Ac8wHwYDVR0jBBgwFoAUgq9sjPjF/pZhfOgfPStxSF7Ei8AwHQYDVR0OBBYEFLfuNf820LvaT4AK xrGK3EKx1DE7MA4GA1UdDwEB/wQEAwIFoDAMBgNVHRMBAf8EAjAAMB0GA1UdJQQWMBQGCCsGAQUF BwMEBggrBgEFBQcDAjBGBgNVHSAEPzA9MDsGDCsGAQQBsjEBAgEDBTArMCkGCCsGAQUFBwIBFh1o dHRwczovL3NlY3VyZS5jb21vZG8ubmV0L0NQUzBaBgNVHR8EUzBRME+gTaBLhklodHRwOi8vY3Js LmNvbW9kb2NhLmNvbS9DT01PRE9SU0FDbGllbnRBdXRoZW50aWNhdGlvbmFuZFNlY3VyZUVtYWls Q0EuY3JsMIGLBggrBgEFBQcBAQR/MH0wVQYIKwYBBQUHMAKGSWh0dHA6Ly9jcnQuY29tb2RvY2Eu Y29tL0NPTU9ET1JTQUNsaWVudEF1dGhlbnRpY2F0aW9uYW5kU2VjdXJlRW1haWxDQS5jcnQwJAYI KwYBBQUHMAGGGGh0dHA6Ly9vY3NwLmNvbW9kb2NhLmNvbTAeBgNVHREEFzAVgRNkd213MkBpbmZy YWRlYWQub3JnMA0GCSqGSIb3DQEBCwUAA4IBAQALbSykFusvvVkSIWttcEeifOGGKs7Wx2f5f45b nv2ghcxK5URjUvCnJhg+soxOMoQLG6+nbhzzb2rLTdRVGbvjZH0fOOzq0LShq0EXsqnJbbuwJhK+ PnBtqX5O23PMHutP1l88AtVN+Rb72oSvnD+dK6708JqqUx2MAFLMevrhJRXLjKb2Mm+/8XBpEw+B 7DisN4TMlLB/d55WnT9UPNHmQ+3KFL7QrTO8hYExkU849g58Dn3Nw3oCbMUgny81ocrLlB2Z5fFG Qu1AdNiBA+kg/UxzyJZpFbKfCITd5yX49bOriL692aMVDyqUvh8fP+T99PqorH4cIJP6OxSTdxKM MIIFHDCCBASgAwIBAgIRAOK7SUh5KuwJ6cSlGPGZWGYwDQYJKoZIhvcNAQELBQAwgZcxCzAJBgNV BAYTAkdCMRswGQYDVQQIExJHcmVhdGVyIE1hbmNoZXN0ZXIxEDAOBgNVBAcTB1NhbGZvcmQxGjAY BgNVBAoTEUNPTU9ETyBDQSBMaW1pdGVkMT0wOwYDVQQDEzRDT01PRE8gUlNBIENsaWVudCBBdXRo ZW50aWNhdGlvbiBhbmQgU2VjdXJlIEVtYWlsIENBMB4XDTE5MDEwMjAwMDAwMFoXDTIyMDEwMTIz NTk1OVowJDEiMCAGCSqGSIb3DQEJARYTZHdtdzJAaW5mcmFkZWFkLm9yZzCCASIwDQYJKoZIhvcN AQEBBQADggEPADCCAQoCggEBALL98Dmy0wm1AOxiaio/bxxn/hWvraZDvmUVb3vRKTbDLH14bxaW /EYC/Lw/i9d98CunLGKA1O8yogKPdhTFFmmNR8uh6r1a/aHr301d8YQea0DtURyaAH5L4cvsKY6O 0HF7s5DqYm6tmLq1UrRwIhqgDjBjF4XFRqj7hoXFXFc1VI7LxMwFfT6PStq6WedhROKw5KQytaQS vrjgMjpICIP3A/CroGr+bcmqcnXljGUSUB9bzEOjlU9uAsgJ9sl5tjYE0DEtZqc0rT9oqD7U57My ECIewElc4VenLB2/GK5MfJoJZTsq7fWNpFkUSRZvxT0TRLqznjVepZ2AFzsplScCAwEAAaOCAdMw ggHPMB8GA1UdIwQYMBaAFIKvbIz4xf6WYXzoHz0rcUhexIvAMB0GA1UdDgQWBBS37jX/NtC72k+A CsaxitxCsdQxOzAOBgNVHQ8BAf8EBAMCBaAwDAYDVR0TAQH/BAIwADAdBgNVHSUEFjAUBggrBgEF BQcDBAYIKwYBBQUHAwIwRgYDVR0gBD8wPTA7BgwrBgEEAbIxAQIBAwUwKzApBggrBgEFBQcCARYd aHR0cHM6Ly9zZWN1cmUuY29tb2RvLm5ldC9DUFMwWgYDVR0fBFMwUTBPoE2gS4ZJaHR0cDovL2Ny bC5jb21vZG9jYS5jb20vQ09NT0RPUlNBQ2xpZW50QXV0aGVudGljYXRpb25hbmRTZWN1cmVFbWFp bENBLmNybDCBiwYIKwYBBQUHAQEEfzB9MFUGCCsGAQUFBzAChklodHRwOi8vY3J0LmNvbW9kb2Nh LmNvbS9DT01PRE9SU0FDbGllbnRBdXRoZW50aWNhdGlvbmFuZFNlY3VyZUVtYWlsQ0EuY3J0MCQG CCsGAQUFBzABhhhodHRwOi8vb2NzcC5jb21vZG9jYS5jb20wHgYDVR0RBBcwFYETZHdtdzJAaW5m cmFkZWFkLm9yZzANBgkqhkiG9w0BAQsFAAOCAQEAC20spBbrL71ZEiFrbXBHonzhhirO1sdn+X+O W579oIXMSuVEY1LwpyYYPrKMTjKECxuvp24c829qy03UVRm742R9Hzjs6tC0oatBF7KpyW27sCYS vj5wbal+TttzzB7rT9ZfPALVTfkW+9qEr5w/nSuu9PCaqlMdjABSzHr64SUVy4ym9jJvv/FwaRMP gew4rDeEzJSwf3eeVp0/VDzR5kPtyhS+0K0zvIWBMZFPOPYOfA59zcN6AmzFIJ8vNaHKy5QdmeXx RkLtQHTYgQPpIP1Mc8iWaRWynwiE3ecl+PWzq4i+vdmjFQ8qlL4fHz/k/fT6qKx+HCCT+jsUk3cS jDCCBeYwggPOoAMCAQICEGqb4Tg7/ytrnwHV2binUlYwDQYJKoZIhvcNAQEMBQAwgYUxCzAJBgNV BAYTAkdCMRswGQYDVQQIExJHcmVhdGVyIE1hbmNoZXN0ZXIxEDAOBgNVBAcTB1NhbGZvcmQxGjAY BgNVBAoTEUNPTU9ETyBDQSBMaW1pdGVkMSswKQYDVQQDEyJDT01PRE8gUlNBIENlcnRpZmljYXRp b24gQXV0aG9yaXR5MB4XDTEzMDExMDAwMDAwMFoXDTI4MDEwOTIzNTk1OVowgZcxCzAJBgNVBAYT AkdCMRswGQYDVQQIExJHcmVhdGVyIE1hbmNoZXN0ZXIxEDAOBgNVBAcTB1NhbGZvcmQxGjAYBgNV BAoTEUNPTU9ETyBDQSBMaW1pdGVkMT0wOwYDVQQDEzRDT01PRE8gUlNBIENsaWVudCBBdXRoZW50 aWNhdGlvbiBhbmQgU2VjdXJlIEVtYWlsIENBMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKC AQEAvrOeV6wodnVAFsc4A5jTxhh2IVDzJXkLTLWg0X06WD6cpzEup/Y0dtmEatrQPTRI5Or1u6zf +bGBSyD9aH95dDSmeny1nxdlYCeXIoymMv6pQHJGNcIDpFDIMypVpVSRsivlJTRENf+RKwrB6vcf WlP8dSsE3Rfywq09N0ZfxcBa39V0wsGtkGWC+eQKiz4pBZYKjrc5NOpG9qrxpZxyb4o4yNNwTqza aPpGRqXB7IMjtf7tTmU2jqPMLxFNe1VXj9XB1rHvbRikw8lBoNoSWY66nJN/VCJv5ym6Q0mdCbDK CMPybTjoNCQuelc0IAaO4nLUXk0BOSxSxt8kCvsUtQIDAQABo4IBPDCCATgwHwYDVR0jBBgwFoAU u69+Aj36pvE8hI6t7jiY7NkyMtQwHQYDVR0OBBYEFIKvbIz4xf6WYXzoHz0rcUhexIvAMA4GA1Ud DwEB/wQEAwIBhjASBgNVHRMBAf8ECDAGAQH/AgEAMBEGA1UdIAQKMAgwBgYEVR0gADBMBgNVHR8E RTBDMEGgP6A9hjtodHRwOi8vY3JsLmNvbW9kb2NhLmNvbS9DT01PRE9SU0FDZXJ0aWZpY2F0aW9u QXV0aG9yaXR5LmNybDBxBggrBgEFBQcBAQRlMGMwOwYIKwYBBQUHMAKGL2h0dHA6Ly9jcnQuY29t b2RvY2EuY29tL0NPTU9ET1JTQUFkZFRydXN0Q0EuY3J0MCQGCCsGAQUFBzABhhhodHRwOi8vb2Nz cC5jb21vZG9jYS5jb20wDQYJKoZIhvcNAQEMBQADggIBAHhcsoEoNE887l9Wzp+XVuyPomsX9vP2 SQgG1NgvNc3fQP7TcePo7EIMERoh42awGGsma65u/ITse2hKZHzT0CBxhuhb6txM1n/y78e/4ZOs 0j8CGpfb+SJA3GaBQ+394k+z3ZByWPQedXLL1OdK8aRINTsjk/H5Ns77zwbjOKkDamxlpZ4TKSDM KVmU/PUWNMKSTvtlenlxBhh7ETrN543j/Q6qqgCWgWuMAXijnRglp9fyadqGOncjZjaaSOGTTFB+ E2pvOUtY+hPebuPtTbq7vODqzCM6ryEhNhzf+enm0zlpXK7q332nXttNtjv7VFNYG+I31gnMrwfH M5tdhYF/8v5UY5g2xANPECTQdu9vWPoqNSGDt87b3gXb1AiGGaI06vzgkejL580ul+9hz9D0S0U4 jkhJiA7EuTecP/CFtR72uYRBcunwwH3fciPjviDDAI9SnC/2aPY8ydehzuZutLbZdRJ5PDEJM/1t yZR2niOYihZ+FCbtf3D9mB12D4ln9icgc7CwaxpNSCPt8i/GqK2HsOgkL3VYnwtx7cJUmpvVdZ4o gnzgXtgtdk3ShrtOS1iAN2ZBXFiRmjVzmehoMof06r1xub+85hFQzVxZx5/bRaTKTlL8YXLI8nAb R9HWdFqzcOoB/hxfEyIQpx9/s81rgzdEZOofSlZHynoSMYIDyjCCA8YCAQEwga0wgZcxCzAJBgNV BAYTAkdCMRswGQYDVQQIExJHcmVhdGVyIE1hbmNoZXN0ZXIxEDAOBgNVBAcTB1NhbGZvcmQxGjAY BgNVBAoTEUNPTU9ETyBDQSBMaW1pdGVkMT0wOwYDVQQDEzRDT01PRE8gUlNBIENsaWVudCBBdXRo ZW50aWNhdGlvbiBhbmQgU2VjdXJlIEVtYWlsIENBAhEA4rtJSHkq7AnpxKUY8ZlYZjANBglghkgB ZQMEAgEFAKCCAe0wGAYJKoZIhvcNAQkDMQsGCSqGSIb3DQEHATAcBgkqhkiG9w0BCQUxDxcNMTkw NjIwMTM1ODUzWjAvBgkqhkiG9w0BCQQxIgQgXzmkSB2A3VtmPDnzV14OL0A+W0/N0elPpXHOXYmN Zoowgb4GCSsGAQQBgjcQBDGBsDCBrTCBlzELMAkGA1UEBhMCR0IxGzAZBgNVBAgTEkdyZWF0ZXIg TWFuY2hlc3RlcjEQMA4GA1UEBxMHU2FsZm9yZDEaMBgGA1UEChMRQ09NT0RPIENBIExpbWl0ZWQx PTA7BgNVBAMTNENPTU9ETyBSU0EgQ2xpZW50IEF1dGhlbnRpY2F0aW9uIGFuZCBTZWN1cmUgRW1h aWwgQ0ECEQDiu0lIeSrsCenEpRjxmVhmMIHABgsqhkiG9w0BCRACCzGBsKCBrTCBlzELMAkGA1UE BhMCR0IxGzAZBgNVBAgTEkdyZWF0ZXIgTWFuY2hlc3RlcjEQMA4GA1UEBxMHU2FsZm9yZDEaMBgG A1UEChMRQ09NT0RPIENBIExpbWl0ZWQxPTA7BgNVBAMTNENPTU9ETyBSU0EgQ2xpZW50IEF1dGhl bnRpY2F0aW9uIGFuZCBTZWN1cmUgRW1haWwgQ0ECEQDiu0lIeSrsCenEpRjxmVhmMA0GCSqGSIb3 DQEBAQUABIIBAFKvDJDFkNZ5X/Se5s0NoM236P/DpHhgXyDes611e1lBDG08vfx4g9cT51gO/w4C awlstUWFTj4tU8jp2BT5BngalPIAMFJXF82716g5c/D0EVehUGdPWHm1yqvRB/qIIHL9fphvBrjk J0yYzI1PG4AM0KQttoBwSlXlBDwUZU18Nm5CtABeRnrMt1taLIlj/W2DVGk2PJvX6+zWG/czTpLd 8oQyMDi4NtBay/RkR9GtCnH68kdLVhYZX7SAVvq41h0TYcbCPVm1dRk1hnzIoEi5XjVzxFfeEtVh 4qvHDQ/bZ/3LQUUNiniCMQDOEbbuedISIQhjEl8WyBKTVFXTzJUAAAAAAAA= --=-qjWGj8RFrdi5w/Db3i1n--