From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40729) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1daSDN-0003to-IA for qemu-devel@nongnu.org; Wed, 26 Jul 2017 15:43:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1daSDL-0000gk-Ev for qemu-devel@nongnu.org; Wed, 26 Jul 2017 15:43:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46516) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1daSDL-0000gA-5L for qemu-devel@nongnu.org; Wed, 26 Jul 2017 15:43:23 -0400 Date: Wed, 26 Jul 2017 22:43:09 +0300 From: "Michael S. Tsirkin" Message-ID: <20170726215625-mutt-send-email-mst@kernel.org> References: <1500761743-1669-1-git-send-email-zuban32s@gmail.com> <1500761743-1669-5-git-send-email-zuban32s@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1500761743-1669-5-git-send-email-zuban32s@gmail.com> Subject: Re: [Qemu-devel] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aleksandr Bezzubikov Cc: qemu-devel@nongnu.org, imammedo@redhat.com, pbonzini@redhat.com, rth@twiddle.net, ehabkost@redhat.com, marcel@redhat.com, kraxel@redhat.com, seabios@seabios.org On Sun, Jul 23, 2017 at 01:15:41AM +0300, Aleksandr Bezzubikov wrote: > On PCI init PCI bridges may need some > extra info about bus number to reserve, IO, memory and > prefetchable memory limits. QEMU can provide this > with special with a special > vendor-specific PCI capability. > > Sizes of limits match ones from > PCI Type 1 Configuration Space Header, > number of buses to reserve occupies only 1 byte > since it is the size of Subordinate Bus Number register. > > Signed-off-by: Aleksandr Bezzubikov > --- > hw/pci/pci_bridge.c | 27 +++++++++++++++++++++++++++ > include/hw/pci/pci_bridge.h | 18 ++++++++++++++++++ > 2 files changed, 45 insertions(+) > > diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c > index 720119b..8ec6c2c 100644 > --- a/hw/pci/pci_bridge.c > +++ b/hw/pci/pci_bridge.c > @@ -408,6 +408,33 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name, > br->bus_name = bus_name; > } > > + > +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset, help? should be qemu_cap_init? > + uint8_t bus_reserve, uint32_t io_limit, > + uint16_t mem_limit, uint64_t pref_limit, > + Error **errp) > +{ > + size_t cap_len = sizeof(PCIBridgeQemuCap); > + PCIBridgeQemuCap cap; This leaks info to guest. You want to init all fields here: cap = { .len = .... }; > + > + cap.len = cap_len; > + cap.bus_res = bus_reserve; > + cap.io_lim = io_limit & 0xFF; > + cap.io_lim_upper = io_limit >> 8 & 0xFFFF; > + cap.mem_lim = mem_limit; > + cap.pref_lim = pref_limit & 0xFFFF; > + cap.pref_lim_upper = pref_limit >> 16 & 0xFFFFFFFF; Please use pci_set_word etc or cpu_to_leXX. I think it's easiest to replace struct with a set of macros then pci_set_word does the work for you. > + > + int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, > + cap_offset, cap_len, errp); > + if (offset < 0) { > + return offset; > + } > + > + memcpy(dev->config + offset + 2, (char *)&cap + 2, cap_len - 2); +2 is yacky. See how virtio does it: memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len, cap->cap_len - PCI_CAP_FLAGS); > + return 0; > +} > + > static const TypeInfo pci_bridge_type_info = { > .name = TYPE_PCI_BRIDGE, > .parent = TYPE_PCI_DEVICE, > diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h > index ff7cbaa..c9f642c 100644 > --- a/include/hw/pci/pci_bridge.h > +++ b/include/hw/pci/pci_bridge.h > @@ -67,4 +67,22 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name, > #define PCI_BRIDGE_CTL_DISCARD_STATUS 0x400 /* Discard timer status */ > #define PCI_BRIDGE_CTL_DISCARD_SERR 0x800 /* Discard timer SERR# enable */ > > +typedef struct PCIBridgeQemuCap { > + uint8_t id; /* Standard PCI capability header field */ > + uint8_t next; /* Standard PCI capability header field */ > + uint8_t len; /* Standard PCI vendor-specific capability header field */ > + uint8_t bus_res; > + uint32_t pref_lim_upper; Big endian? Ugh. > + uint16_t pref_lim; > + uint16_t mem_lim; I'd say we need 64 bit for memory. > + uint16_t io_lim_upper; > + uint8_t io_lim; > + uint8_t padding; IMHO each type should have a special "don't care" flag that would mean "I don't know". > +} PCIBridgeQemuCap; You don't really need this struct in the header. And pls document all fields. > + > +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset, > + uint8_t bus_reserve, uint32_t io_limit, > + uint16_t mem_limit, uint64_t pref_limit, > + Error **errp); > + > #endif /* QEMU_PCI_BRIDGE_H */ > -- > 2.7.4