From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.51874.1673519768613922111 for ; Thu, 12 Jan 2023 02:36:08 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=c/M6GKGb; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673519767; 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=EqYEARS1vOEt8ua2+c+RYUGmb/15xj+9Y0Xdlqforvg=; b=c/M6GKGbhwKQW05vdgNI+0nnJefTf206bFAiFW/n3H8mU3c6+1GZ9oRDANoE2ESsi3lqxI MoqZ2RbVNbY3ixYLwpcUHpUs03ePt6TFGkOdhDFlmS2W8Q+KkIL6eGm2ivKj5p/LxYIQhh JVdLb5Am6FI0hF1tP039XURtxVAkGME= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-654-lOn9BmTtOF6sp0txJOS8NA-1; Thu, 12 Jan 2023 05:36:06 -0500 X-MC-Unique: lOn9BmTtOF6sp0txJOS8NA-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 0CF842802E23; Thu, 12 Jan 2023 10:36:06 +0000 (UTC) Received: from [10.39.192.93] (unknown [10.39.192.93]) by smtp.corp.redhat.com (Postfix) with ESMTPS id AEEF0492B00; Thu, 12 Jan 2023 10:36:04 +0000 (UTC) Message-ID: Date: Thu, 12 Jan 2023 11:36:03 +0100 MIME-Version: 1.0 Subject: Re: [PATCH v2 4/4] OvmfPkg/PlatformInitLib: Add PlatformReservationConflictCB To: Gerd Hoffmann Cc: devel@edk2.groups.io, Pawel Polawski , Jiewen Yao , Oliver Steffen , Jordan Justen , Ard Biesheuvel References: <20230110082123.159521-1-kraxel@redhat.com> <20230110082123.159521-5-kraxel@redhat.com> <20230111082647.d3bg3er5ofjpszuc@sirius.home.kraxel.org> From: "Laszlo Ersek" In-Reply-To: <20230111082647.d3bg3er5ofjpszuc@sirius.home.kraxel.org> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 1/11/23 09:26, Gerd Hoffmann wrote: > Hi, > >>> +/** >>> + Check whenever the 64bit PCI MMIO window overlaps with a reservation >>> + from qemu. If so move down the MMIO window to resolve the conflict. >>> + >>> + This happens on (virtal) AMD machines with 1TB address space, >>> + because the AMD IOMMU uses an address window just below 1GB. >> >> (3) Same two typos, I think, as in the commit message. > > Duplicated by the power of cut + paste. > >>> + NewBase = (PlatformInfoHob->PcdPciMmio64Base - >>> + PlatformInfoHob->PcdPciMmio64Size); >> >> (6) This appears a typo; we'll want >> >> NewBase + PcdPciMmio64Size == E820Entry->BaseAddr > > But then NewBase is not aligned. The assignment above moves it down > while maintaining the existing alignment. Ah, good point. I totally missed the idea here. The starting predicate is, apparently, that the base is aligned by size ("naturally aligned"), so by subtracting size from the aperture, we get a new aperture that conforms to both properties below: - still naturally aligned (original predicate preserved), - empty intersection with the original aperture (because the new aperture will end where the old one just started). Now, here's one thought: just because the new (sunken) aperture does not intersect the pre-move aperture, is it guaranteed to also steer clear of the E820 Entry either? I don't think that's certain. If the E820 Entry overlaps the bottom of the original aperture, but goes "deeper" than that, then it will overlap the top of the sunken aperture too. So I think we might want to do a two step process here, in case the original aperture overlaps the E820 Entry: - push down the aperture (same size) so its exclusive end equal the inclusive start of the E820 Entry - align *down* (truncate) the base of the aperture, while keeping its size unchanged. Something like: NewBase = E820Entry->BaseAddr - PcdPciMmio64Size; ASSERT (PcdPciMmio64Size != 0); ASSERT ((PcdPciMmio64Size & (PcdPciMmio64Size - 1)) == 0); NewBase = NewBase & ~(PcdPciMmio64Size - 1); > >> (9) Do we need any other checks or maybe assertions that we're only >> conflicting with a reserved area, and/or that the subtraction for >> NewBase does not underflow? >> >> I don't think we can "armor" this very well, I'm just pondering if there >> are any egregious misunderstandings between QEMU and the firmware that >> we might want to catch here. If not, that's OK of course. > > Yes, it's hard to design something which can handle all reservations > qemu might do in the future correctly. And, yes, the code above works > because we know the qemu reservation is smaller than the mmio window, so > moving down to the next naturally aligned address actually solves the > conflict. Not only smaller, but also *not encompassing* the lower boundary of the MMIO window. Anyway, up to you -- if you want to stick with the logic shown in this patch, I'm OK, but then please add some comments, or maybe even some ASSERTs. Thanks! Laszlo