From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52088) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1deRh4-0006yr-2s for qemu-devel@nongnu.org; Sun, 06 Aug 2017 15:58:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1deRh1-0002MG-1D for qemu-devel@nongnu.org; Sun, 06 Aug 2017 15:58:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59726) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1deRh0-0002Lj-No for qemu-devel@nongnu.org; Sun, 06 Aug 2017 15:58:30 -0400 References: <1501284872-2078-1-git-send-email-zuban32s@gmail.com> <1501284872-2078-3-git-send-email-zuban32s@gmail.com> <20170731165717-mutt-send-email-mst@kernel.org> <362df6d1-b5f8-fd37-13f4-836eb5e86da7@redhat.com> <20170731215618-mutt-send-email-mst@kernel.org> <9fc4636a-eeea-74f8-3481-07f320659175@redhat.com> From: Marcel Apfelbaum Message-ID: <4aa858d9-4237-8273-de08-fab2c7410b5e@redhat.com> Date: Sun, 6 Aug 2017 22:58:17 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Bezzubikov , Laszlo Ersek Cc: "Michael S. Tsirkin" , seabios@seabios.org, Kevin OConnor , qemu-devel@nongnu.org, Gerd Hoffmann On 04/08/2017 23:47, Alexander Bezzubikov wrote: > 2017-08-04 23:28 GMT+03:00 Laszlo Ersek : >> On 08/04/17 20:59, Alexander Bezzubikov wrote: >>> 2017-08-01 20:28 GMT+03:00 Alexander Bezzubikov : >>>> 2017-08-01 16:38 GMT+03:00 Marcel Apfelbaum : >>>>> On 31/07/2017 22:01, Alexander Bezzubikov wrote: >>>>>> >>>>>> 2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin : >>>>>>> >>>>>>> On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote: >>>>>>>> >>>>>>>> 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum : >>>>>>>>> >>>>>>>>> On 31/07/2017 17:00, Michael S. Tsirkin wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Sat, Jul 29, 2017 at 02:34:31AM +0300, Aleksandr Bezzubikov wrote: >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On PCI init PCI bridge devices may need some >>>>>>>>>>> extra info about bus number to reserve, IO, memory and >>>>>>>>>>> prefetchable memory limits. QEMU can provide this >>>>>>>>>>> with special vendor-specific PCI capability. >>>>>>>>>>> >>>>>>>>>>> This capability is intended to be used only >>>>>>>>>>> for Red Hat PCI bridges, i.e. QEMU cooperation. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Aleksandr Bezzubikov >>>>>>>>>>> --- >>>>>>>>>>> src/fw/dev-pci.h | 62 >>>>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>>>>> 1 file changed, 62 insertions(+) >>>>>>>>>>> create mode 100644 src/fw/dev-pci.h >>>>>>>>>>> >>>>>>>>>>> diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h >>>>>>>>>>> new file mode 100644 >>>>>>>>>>> index 0000000..fbd49ed >>>>>>>>>>> --- /dev/null >>>>>>>>>>> +++ b/src/fw/dev-pci.h >>>>>>>>>>> @@ -0,0 +1,62 @@ >>>>>>>>>>> +#ifndef _PCI_CAP_H >>>>>>>>>>> +#define _PCI_CAP_H >>>>>>>>>>> + >>>>>>>>>>> +#include "types.h" >>>>>>>>>>> + >>>>>>>>>>> +/* >>>>>>>>>>> + >>>>>>>>>>> +QEMU-specific vendor(Red Hat)-specific capability. >>>>>>>>>>> +It's intended to provide some hints for firmware to init PCI >>>>>>>>>>> devices. >>>>>>>>>>> + >>>>>>>>>>> +Its is shown below: >>>>>>>>>>> + >>>>>>>>>>> +Header: >>>>>>>>>>> + >>>>>>>>>>> +u8 id; Standard PCI Capability Header field >>>>>>>>>>> +u8 next; Standard PCI Capability Header field >>>>>>>>>>> +u8 len; Standard PCI Capability Header field >>>>>>>>>>> +u8 type; Red Hat vendor-specific capability type: >>>>>>>>>>> + now only REDHAT_QEMU_CAP 1 exists >>>>>>>>>>> +Data: >>>>>>>>>>> + >>>>>>>>>>> +u16 non_prefetchable_16; non-prefetchable memory limit >>>>>>>>>>> + >>>>>>>>>>> +u8 bus_res; minimum bus number to reserve; >>>>>>>>>>> + this is necessary for PCI Express Root Ports >>>>>>>>>>> + to support PCIE-to-PCI bridge hotplug >>>>>>>>>>> + >>>>>>>>>>> +u8 io_8; IO limit in case of 8-bit limit value >>>>>>>>>>> +u32 io_32; IO limit in case of 16-bit limit value >>>>>>>>>>> + io_8 and io_16 are mutually exclusive, in other words, >>>>>>>>>>> + they can't be non-zero simultaneously >>>>>>>>>>> + >>>>>>>>>>> +u32 prefetchable_32; non-prefetchable memory limit >>>>>>>>>>> + in case of 32-bit limit value >>>>>>>>>>> +u64 prefetchable_64; non-prefetchable memory limit >>>>>>>>>>> + in case of 64-bit limit value >>>>>>>>>>> + prefetachable_32 and prefetchable_64 >>>>>>>>>>> are >>>>>>>>>>> + mutually exclusive, in other words, >>>>>>>>>>> + they can't be non-zero simultaneously >>>>>>>>>>> +If any field in Data section is 0, >>>>>>>>>>> +it means that such kind of reservation >>>>>>>>>>> +is not needed. >>>>>>>> >>>>>>>> >>>>>>>> I really don't like this 'mutually exclusive' fields approach because >>>>>>>> IMHO it increases confusion level when undertanding this capability >>>>>>>> structure. >>>>>>>> But - if we came to consensus on that, then IO fields should be used >>>>>>>> in the same way, >>>>>>>> because as I understand, this 'mutual exclusivity' serves to distinguish >>>>>>>> cases >>>>>>>> when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT >>>>>>>> registers. >>>>>>>> And this is how both IO and PREFETCHABLE works, isn't it? >>>>>>> >>>>>>> >>>>>>> I would just use simeple 64 bit registers. PCI spec has an ugly format >>>>>>> with fields spread all over the place but that is because of >>>>>>> compatibility concerns. It makes not sense to spend cycles just >>>>>>> to be similarly messy. >>>>>> >>>>>> >>>>>> Then I suggest to use exactly one field of a maximum possible size >>>>>> for each reserving object, and get rid of mutually exclusive fields. >>>>>> Then it can be something like that (order and names can be changed): >>>>>> u8 bus; >>>>>> u16 non_pref; >>>>>> u32 io; >>>>>> u64 pref; >>>>>> >>>>> >>>>> I think Michael suggested: >>>>> >>>>> u64 bus_res; >>>>> u64 mem_res; >>>>> u64 io_res; >>>>> u64 mem_pref_res; >>>>> >>>>> OR: >>>>> u32 bus_res; >>>>> u32 mem_res; >>>>> u32 io_res; >>>>> u64 mem_pref_res; >>>>> >>>>> >>>>> We can use 0XFFF..F as "not-set" value "merging" Gerd's and Michael's >>>>> requests. >>>> >>>> Let's dwell on the second option (with -1 as 'ignore' sign), if no new >>>> objections. >>>> >>> >>> BTW, talking about limit values provided in the capability - do we >>> want to completely override >>> existing PCI resources allocation mechanism being used in SeaBIOS, I >>> mean, to assign capability >>> values hardly, not taking into consideration any existing checks, >>> or somehow make this process soft (not an obvious way, can lead to >>> another big discussion)? >>> >>> In other words, how do we plan to use IO/MEM/PREF limits provided in >>> this capability >>> in application to the PCIE Root Port, what result is this supposed to achieve? >> >> I think Gerd spoke about this earlier: when determining a given kind of >> aperture for a given bridge, pick the maximum of: >> - the actual cumulative need of the devices behind the bridge, and >> - the hint for the given kind of aperture. >> >> So basically, do the same thing as before, but if the hint is larger, >> grow the aperture to that. > > Looks like current SeaBIOS resource allocation implementation is incorrect. > E.g. let's look at IO for pcie-root-port (and ioh3420 too) - we have 2 different > pci_region_entry objects, where one of them have bar = -1 (that > respresent a bridge > region as it as in the comment there) and size = 0. This leads to the > next situation after > the whole BIOS init: root port's IO has base=0x5000,size=0x4FFF. > And then Linux reconfigures this values itself. > Should the size of the pci_region be checked for emptiness before creation or > this isn't an error for some reason? I am not aware of such issue, however there is a patch that prevents SeaBIOS to allocate IO range for PCIe Root Ports if the device attached to it does not require IO ports. (or if a Root Port is empty) Can you provide more info? (a debug log) Or a patch that could fix the issue you see? So we can better understand the problem. Thanks, Marcel > >> >> This is how I recall the discussion anyway. >> >> Thanks >> Laszlo > > >