From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:34858) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R0DAj-0000GE-Up for qemu-devel@nongnu.org; Sun, 04 Sep 2011 09:55:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R0DAi-0004cM-Hz for qemu-devel@nongnu.org; Sun, 04 Sep 2011 09:55:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:29337) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R0DAi-0004cI-8v for qemu-devel@nongnu.org; Sun, 04 Sep 2011 09:55:40 -0400 Message-ID: <4E638355.8030903@redhat.com> Date: Sun, 04 Sep 2011 16:55:33 +0300 From: Avi Kivity MIME-Version: 1.0 References: <20110826094254.GA6520@redhat.com> <4E59F359.9040506@redhat.com> <20110828114142.GC4875@redhat.com> <4E5A3E36.4010709@redhat.com> <20110828134203.GA6751@redhat.com> <4E5A485D.1020904@redhat.com> <20110904123006.GA23500@redhat.com> <4E6371DA.7070304@redhat.com> <20110904130159.GB23500@redhat.com> <4E63778A.6050002@redhat.com> <20110904134101.GA27239@redhat.com> In-Reply-To: <20110904134101.GA27239@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] pci: add standard bridge device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Kevin Wolf , Isaku Yamahata , qemu-devel@nongnu.org On 09/04/2011 04:41 PM, Michael S. Tsirkin wrote: > On Sun, Sep 04, 2011 at 04:05:14PM +0300, Avi Kivity wrote: > > It follows naturally: > > OK, so it seems the following is more or less what you suggest? > I'm not sure I create/destroy subregions properly. > Both the alias and the subregion get the same start value? Yes (so addresses are not shifted). > Is the region name for debugging only? For everything except RAM regions (there, the name is also used for save/restore). > When does priority matter? In case of overlap? Yes. In this case, since overlap resolution is not defined by the spec, the actual priority does not matter. > @@ -135,6 +135,75 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type) > return limit; > } > > +static pcibus_t pci_bridge_get_size(const PCIDevice *bridge, uint8_t type) > +{ > + return pci_bridge_get_limit(bridge, type)>= > + pci_bridge_get_base(bridge, type) ? > + pci_bridge_get_limit(bridge, type) - > + pci_bridge_get_base(bridge, type) + 1 : 0; > +} Correct but unreadable. Doesn't work for limit == 2^64-1, is this a possible value? > + > +static void pci_bridge_region_init(PCIBridge *br) > +{ > + PCIBus *sec_bus =&br->sec_bus; > + PCIBus *parent = br->dev.bus; > + memory_region_init_alias(sec_bus->alias_pref_mem, "pci_bridge_pref_mem", > + sec_bus->address_space_mem, > + pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH), > + pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH)); > + memory_region_add_subregion_overlap(parent->address_space_mem, > + pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH), > + sec_bus->alias_pref_mem, 1); > + memory_region_init_alias(sec_bus->alias_mem, "pci_bridge_memory", > + sec_bus->address_space_mem, > + pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY), > + pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY)); > + memory_region_add_subregion_overlap(parent->address_space_mem, > + pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY), > + sec_bus->alias_mem, 1); > + memory_region_init_alias(sec_bus->alias_io, "pci_bridge_io", > + sec_bus->address_space_io, > + pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_IO), > + pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_SPACE_IO)); > + memory_region_add_subregion_overlap(parent->address_space_io, > + pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_IO), > + sec_bus->alias_io, 1); > +} This looks right. Might want to use pci_address_space() instead of ->address_space_mem. Don't you have to do something similar for the vga window? > + > +static void pci_bridge_region_cleanup(PCIBridge *br) > +{ > + PCIBus *sec_bus =&br->sec_bus; > + PCIBus *parent = br->dev.bus; > + memory_region_del_subregion(parent->address_space_mem, > + sec_bus->alias_pref_mem); > + memory_region_destroy(sec_bus->alias_pref_mem); > + memory_region_del_subregion(parent->address_space_mem, > + sec_bus->alias_mem); > + memory_region_destroy(sec_bus->alias_mem); > + memory_region_del_subregion(parent->address_space_io, > + sec_bus->alias_io); > + memory_region_destroy(sec_bus->alias_io); > +} This is fine too. > + > +static void pci_bridge_update_mappings(PCIBridge *br) > +{ > + /* TODO: this doesn't handle the case of one VCPU > + * updating the bridge while another accesses an unaffected > + * region. To fix we'll need new memory region APIs. */ > + pci_bridge_region_cleanup(br); > + pci_bridge_region_init(br); memory_region_transaction_{begin,commit}() (isn't 100% implemented, but at least the API is in place) > + > +#if 0 > + TODO: do we need to propagate updates to child buses? > + > + pci_for_each_device_under_bus(b, pci_bridge_update_mappings_fn); > + > + QLIST_FOREACH(child,&b->child, sibling) { > + pci_bridge_update_mappings(child); > + } > +#endif > +} Don't need this. -- error compiling committee.c: too many arguments to function