From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60145) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YkZ0X-0001OC-Ow for qemu-devel@nongnu.org; Tue, 21 Apr 2015 10:18:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YkZ0S-0007cH-G1 for qemu-devel@nongnu.org; Tue, 21 Apr 2015 10:18:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47222) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YkZ0S-0007aj-6p for qemu-devel@nongnu.org; Tue, 21 Apr 2015 10:18:32 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id E140F8E6E4 for ; Tue, 21 Apr 2015 14:18:31 +0000 (UTC) Message-ID: <55365C33.2090101@redhat.com> Date: Tue, 21 Apr 2015 16:18:27 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <1429521560-2743-1-git-send-email-kraxel@redhat.com> <1429521560-2743-5-git-send-email-kraxel@redhat.com> In-Reply-To: <1429521560-2743-5-git-send-email-kraxel@redhat.com> Content-Type: text/plain; charset=windows-1252 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: Gerd Hoffmann Cc: pbonzini@redhat.com, qemu-devel@nongnu.org, mst@redhat.com In general, commit messages for the series would be appreciated by the uninitiated :) Then, On 04/20/15 11:19, Gerd Hoffmann wrote: > route access to tseg into nowhere when enabled, > for both cpus and busmaster dma. > > Signed-off-by: Gerd Hoffmann > --- > hw/pci-host/q35.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++ > include/hw/pci-host/q35.h | 1 + > 2 files changed, 58 insertions(+) > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > index f0d840c..412ff0a 100644 > --- a/hw/pci-host/q35.c > +++ b/hw/pci-host/q35.c > @@ -198,6 +198,28 @@ static const TypeInfo q35_host_info = { > * MCH D0:F0 > */ > > +static uint64_t tseg_blackhole_read(void *ptr, hwaddr reg, unsigned size) > +{ > + return 0xffffffff; > +} > + > +static void tseg_blackhole_write(void *opaque, hwaddr addr, uint64_t val, > + unsigned width) > +{ > + /* nothing */ > +} > + > +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). (b) Why are 8-byte accesses forbidden? ... Hm, actually, the above doesn't forbid it, it just causes qemu to split up the access. Should be fine. > + > /* 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? - I assume this register is not reprogrammable once SMRAM is locked -- is that 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.) > + memory_region_del_subregion(mch->system_memory, &mch->tseg_blackhole); Hmm. I think this API is for something else. It is not for hole punching. ... Ah, okay, you *are* removing the previously configured blackhole region (ie. you're not hole punching), I see. But, does this work if tseg_blackhole is still all zeroes? (Ie. this is the first invocation.) > + 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?) > + > memory_region_transaction_commit(); > } > > @@ -443,6 +492,14 @@ static void mch_realize(PCIDevice *d, Error **errp) > object_property_add_alias(qdev_get_machine(), "smram", > OBJECT(&mch->smram), ".", NULL); > > + 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. (OTOH it doesn't really matter, because the region is disabled. Still, it would be easier to understand.) ... I'm thinking loud about what this means for the OVMF memory space map... The central function we use in PlatformPei is GetSystemMemorySizeBelow4gb(), which reads the CMOS (0x34/0x35). That gives us "below_4g_mem_size" in OVMF. Currently that value is used for three purposes: (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); (c) is not affected (the start of the low PCI hole remains unchanged). Okay... > + > init_pam(DEVICE(mch), mch->ram_memory, mch->system_memory, > mch->pci_address_space, &mch->pam_regions[0], > PAM_BIOS_BASE, PAM_BIOS_SIZE); > diff --git a/include/hw/pci-host/q35.h b/include/hw/pci-host/q35.h > index 61bfe7e..ba64c70 100644 > --- a/include/hw/pci-host/q35.h > +++ b/include/hw/pci-host/q35.h > @@ -55,6 +55,7 @@ typedef struct MCHPCIState { > PAMMemoryRegion pam_regions[13]; > MemoryRegion smram_region, open_high_smram; > MemoryRegion smram, low_smram, high_smram; > + MemoryRegion tseg_blackhole; > PcPciInfo pci_info; > ram_addr_t below_4g_mem_size; > ram_addr_t above_4g_mem_size; > Thanks! Laszlo