From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44639) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YkZin-0007P6-6d for qemu-devel@nongnu.org; Tue, 21 Apr 2015 11:04:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YkZig-0006hi-Fj for qemu-devel@nongnu.org; Tue, 21 Apr 2015 11:04:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46205) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YkZig-0006hX-7X for qemu-devel@nongnu.org; Tue, 21 Apr 2015 11:04:14 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 7245C8E79D for ; Tue, 21 Apr 2015 15:04:13 +0000 (UTC) Message-ID: <1429628650.21164.24.camel@nilsson.home.kraxel.org> From: Gerd Hoffmann Date: Tue, 21 Apr 2015 17:04:10 +0200 In-Reply-To: <55365C33.2090101@redhat.com> References: <1429521560-2743-1-git-send-email-kraxel@redhat.com> <1429521560-2743-5-git-send-email-kraxel@redhat.com> <55365C33.2090101@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, mst@redhat.com > > +static const MemoryRegionOps tseg_blackhole_ops = { > > + .read = tseg_blackhole_read, > > + .write = tseg_blackhole_write, > > + .endianness = DEVICE_NATIVE_ENDIAN, > > + .valid.min_access_size = 1, > > + .valid.max_access_size = 4, > > + .impl.min_access_size = 4, > > + .impl.max_access_size = 4, > > + .endianness = DEVICE_LITTLE_ENDIAN, > > +}; > > (a) you've specified .endianness twice (with inconsistent initializers > at that, although the value returned by the accessor happens to be > unaffected). Oops, cut+paste bug, will fix (/me wonders why gcc didn't throw an error on that one). > (b) Why are 8-byte accesses forbidden? ... just a matter of setting .valid.max_access_size = 8, can do that of course. > > + > > /* PCIe MMCFG */ > > static void mch_update_pciexbar(MCHPCIState *mch) > > { > > @@ -267,6 +289,7 @@ static void mch_update_smram(MCHPCIState *mch) > > { > > PCIDevice *pd = PCI_DEVICE(mch); > > bool h_smrame = (pd->config[MCH_HOST_BRIDGE_ESMRAMC] & MCH_HOST_BRIDGE_ESMRAMC_H_SMRAME); > > + uint32_t tseg_size; > > > > /* implement SMRAM.D_LCK */ > > if (pd->config[MCH_HOST_BRIDGE_SMRAM] & MCH_HOST_BRIDGE_SMRAM_D_LCK) { > > @@ -296,6 +319,32 @@ static void mch_update_smram(MCHPCIState *mch) > > memory_region_set_enabled(&mch->high_smram, false); > > } > > > > + if (pd->config[MCH_HOST_BRIDGE_ESMRAMC] & MCH_HOST_BRIDGE_ESMRAMC_T_EN) { > > + switch (pd->config[MCH_HOST_BRIDGE_ESMRAMC] & > > + MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_MASK) { > > + case MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_1MB: > > + tseg_size = 1024 * 1024; > > + break; > > + case MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_2MB: > > + tseg_size = 1024 * 1024 * 2; > > + break; > > + case MCH_HOST_BRIDGE_ESMRAMC_TSEG_SZ_8MB: > > + tseg_size = 1024 * 1024 * 8; > > + break; > > + default: > > + tseg_size = 0; > > + break; > > + } > > + } else { > > + tseg_size = 0; > > + } > > - so, is the guest supposed to select the tseg size by writing this > register? Correct. > - I assume this register is not reprogrammable once SMRAM is locked -- > is that correct? Correct. > - can the guest somehow use this facility to detect, dynamically, the > presence of this feature? (For now I'm thinking about a static build > flag for OVMF that would enable SMM support, but I'm 99% sure Jordan > will object and ask for a dynamic feature detection.) Hmm. I think if we merge all the smm / smram / tseg patches in one go this should work. Without patches reading the ESMRAMC register returns zero. With the patches the three cache-disable bits are hardcoded to '1'. This should work for detecting tseg support. You also have to test for smm support. The current protocol for this is that seabios checks whenever smm is already initialized (see *_apmc_smm_setup() in seabios/src/fw/smm.c) and if so it skips smm initialization. Right now qemu sets the bit on reset when running on kvm, so seabios doesn't try to use smm. On tcg the bit is clear after reset and seabios actually uses smm mode. > > + memory_region_set_enabled(&mch->tseg_blackhole, tseg_size); > > + memory_region_set_size(&mch->tseg_blackhole, tseg_size); > > + memory_region_add_subregion_overlap(mch->system_memory, > > + mch->below_4g_mem_size - tseg_size, > > + &mch->tseg_blackhole, 1); > > So, does tseg overlay the last few MB of the RAM below 4GB? (Ie. right > before the PCI hole starts?) tseg is just normal ram (yes, located at the end of memory), but (once tseg is enabled) only cpus in smm mode are allowed to access it. Likewise busmaster dma access is rejected, so non-smm code can't use the sata controller to access this indirectly. > > + memory_region_init_io(&mch->tseg_blackhole, OBJECT(mch), > > + &tseg_blackhole_ops, NULL, > > + "tseg-blackhole", 0); > > Okay, this answers one of the above, tseg_blackhole is never uninitialized. > > > + memory_region_set_enabled(&mch->tseg_blackhole, false); > > + memory_region_add_subregion_overlap(mch->system_memory, > > + mch->below_4g_mem_size, > > + &mch->tseg_blackhole, 1); > > The address (2nd argument) is inconsistent with the one in > mch_update_smram() -- this function call places the tseg black hole at > the beginning of the 32-bit PCI hole, not just below it. It's a zero-length hole here, therefore it actually is the same. But I didn't bother to use "mch->below_4g_mem_size - 0" as start address to make that more clear ;) > (a) place the permanent PEI RAM so that it ends at the top of > "below_4g_mem_size"; PublishPeiMemory() > (b) create one of the memory HOBs, QemuInitializeRam(), > (c) create the MMIO HOB that describes the 32-bit PCI hole, > MemMapInitialization() > > If the last 1 / 2 / 8 MB of the low RAM can't be used as general purpose > RAM, then (a) is affected (it should simply be sunk by the tseg size of > choice); (b) is affected similarly (just decrease the top of the memory > HOB); Yes. cheers, Gerd