2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin <m...@redhat.com>: > On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote: >> 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum <mar...@redhat.com>: >> > 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 <zuban...@gmail.com> >> >>> --- >> >>> 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; > > >> >> >> >> >> > >> > Hi Michael, >> > >> >> We also want a way to say "no hint for this type". >> >> >> >> One way to achive this would be to have instead multiple >> >> vendor specific capabilities, one for each of >> >> bus#/io/mem/prefetch. 0 would mean do not reserve anything, >> >> absence of capability would mean "no info, up to firmware". >> >> >> > >> > First version of the series was implemented exactly like you propose, >> > however Gerd preferred only one capability with multiple fields. >> > >> > I personally like the simplicity of vendor cap per io/mem/bus, >> > even if it is on the expense of the limited PCI Config space. >> > >> >> Personally I agree with Marcel since all this fields express >> reservations of some objects. >> >> > We need a consensus here :) >> > >> >> Absolutely :) >> >> > Thanks, >> > Marcel >> > >> > >> >> >> >>> + >> >>> +*/ >> >>> + >> >>> +/* Offset of vendor-specific capability type field */ >> >>> +#define PCI_CAP_VNDR_SPEC_TYPE 3 >> >> >> >> >> >> This is a QEMU specific thing. Please name it as such. >> >> >> >>> + >> >>> +/* List of valid Red Hat vendor-specific capability types */ >> >>> +#define REDHAT_CAP_TYPE_QEMU 1 >> >>> + >> >>> + >> >>> +/* Offsets of QEMU capability fields */ >> >>> +#define QEMU_PCI_CAP_NON_PREF 4 >> >>> +#define QEMU_PCI_CAP_BUS_RES 6 >> >>> +#define QEMU_PCI_CAP_IO_8 7 >> >>> +#define QEMU_PCI_CAP_IO_32 8 >> >>> +#define QEMU_PCI_CAP_PREF_32 12 >> >>> +#define QEMU_PCI_CAP_PREF_64 16 >> >>> +#define QEMU_PCI_CAP_SIZE 24 >> >>> + >> >>> +#endif /* _PCI_CAP_H */ >> >>> -- >> >>> 2.7.4 >> > >> > >> >> >> >> -- >> Alexander Bezzubikov -- Alexander Bezzubikov