Re: [Qemu-devel] [Seabios] [PATCH 0/6] 64bit PCI BARs allocations (take 2)
On Mon, Mar 05, 2012 at 07:03:08PM +1300, Alexey Korolev wrote: On Thu, Mar 01, 2012 at 06:50:43PM +1300, Alexey Korolev wrote: In the 6th patch we clear old code. Given the churn in this area, I don't want to commit patches that do wholesale code replacement. I'd prefer to see each patch independently add some functionality and perform its related cleanup. I understand your point, will rework. Would it be reasonable if I send one patch series for redesign of existing implementation and another one for 64bit support? Sending two patch series would be preferable. One series that converts the existing functionality to your new data structures, and one series that adds new capabilities. Also, since Gerd has some patches pending in this area, we should figure out which direction makes sense. Can you explain on how this 64bit support is different from the support proposed by Gerd? Ah it's a difficult thing, I don't want to criticise. You'll hate me :). I think Gerd's implementation is about saving existing approach and having 64bit BARs support with incremental sort of changes. It is reasonable, but causes the code to be bulky, and adding extra types (PCI_REGION_TYPE_PREFMEM64) is a bit misleading. I agree that PCI_REGION_TYPE_PREFMEM64 doesn't make sense to me. 3. About RAM use (may not be important), but lets count: FYI - ram use doesn't really matter. In any reasonable config there will be multiple megabytes of ram available. The ram allocated with malloc_tmp is not reserved beyond the BIOS init phase. -Kevin
Re: [Qemu-devel] [Seabios] [PATCH 0/6] 64bit PCI BARs allocations (take 2)
On Thu, Mar 01, 2012 at 06:50:43PM +1300, Alexey Korolev wrote: Hi, This patch series enables 64bit BAR support in seabios. It has a bit different approach for resources accounting, We did this because we wanted: a) Provide 64bit bar support for PCI BARs and bridges with 64bit memory window. b) Allow migration to 64bit bit ranges if we did not fit into 32bit range c) Keep implementation simple. Hrmm. By my count, this would be the third rewrite of the PCI bar initialization in the last 14 months. [...] The patch series includes 6 patches. In the 1st patch we introduce new structures. Patch 1 does not look like it will compile independently. There is no point in breaking up patches if each part doesn't compile. In the 2nd patch we introduce support functions for basic hlist operations, plus modify service functions to support 64bits address ranges. Note: I've seen similar hlist operations in post memory manager and stack location operations, it makes sense to move them to a header file. In the 3rd patch a new function to fill pci_region structures with entries, and discover topology is added. In the 4th patch we define address range for pci_region structure, migrate entries to 64bits address range if necessary, and program PCI BAR addresses and bridge regions. In the 6th patch we clear old code. Given the churn in this area, I don't want to commit patches that do wholesale code replacement. I'd prefer to see each patch independently add some functionality and perform its related cleanup. Also, since Gerd has some patches pending in this area, we should figure out which direction makes sense. Can you explain on how this 64bit support is different from the support proposed by Gerd? Thanks, -Kevin
Re: [Qemu-devel] [Seabios] [PATCH 0/6] 64bit PCI BARs allocations (take 2)
On Thu, Mar 01, 2012 at 06:50:43PM +1300, Alexey Korolev wrote:
Hi,
This patch series enables 64bit BAR support in seabios.
It has a bit different approach for resources accounting, We did this
because we wanted:
a) Provide 64bit bar support for PCI BARs and bridges with 64bit memory
window.
b) Allow migration to 64bit bit ranges if we did not fit into 32bit
range
c) Keep implementation simple.
Hrmm. By my count, this would be the third rewrite of the PCI bar
initialization in the last 14 months.
Correct, it could be called a rewrite
[...]
The patch series includes 6 patches.
In the 1st patch we introduce new structures.
Patch 1 does not look like it will compile independently. There is no
point in breaking up patches if each part doesn't compile.
Sorry about this. In my case it was difficult to do as the changes are
extensive and I wanted to explain how they work.
Probably the best approach would be splitting into two series.
In the 2nd patch we introduce support functions for basic hlist
operations, plus modify service functions to support 64bits address
ranges.
Note: I've seen similar hlist operations in post memory manager
and stack location operations, it makes sense to move
them to a header file.
In the 3rd patch a new function to fill pci_region structures with
entries, and discover topology is added.
In the 4th patch we define address range for pci_region structure,
migrate entries to 64bits address range if necessary, and program PCI
BAR addresses and bridge regions.
In the 6th patch we clear old code.
Given the churn in this area, I don't want to commit patches that do
wholesale code replacement. I'd prefer to see each patch
independently add some functionality and perform its related cleanup.
I understand your point, will rework.
Would it be reasonable if I send one patch series for redesign of existing
implementation
and another one for 64bit support?
Also, since Gerd has some patches pending in this area, we should
figure out which direction makes sense. Can you explain on how this
64bit support is different from the support proposed by Gerd?
Ah it's a difficult thing, I don't want to criticise. You'll hate me :).
I think Gerd's implementation is about saving existing approach and having
64bit BARs support
with incremental sort of changes. It is reasonable, but causes the code to be
bulky, and adding extra types
(PCI_REGION_TYPE_PREFMEM64) is a bit misleading.
1. Gerd's implementaton creates extra new region types:
static const char *region_type_name[] = {
[ PCI_REGION_TYPE_IO ]= io,
[ PCI_REGION_TYPE_MEM ] = mem,
[ PCI_REGION_TYPE_PREFMEM ] = prefmem,
[ PCI_REGION_TYPE_PREFMEM64 ] = prefmem64,
};
Note: if we are going to support 64bit PCI non prefetchable BARs on root bus
we have to add an
extra type PCI_REGION_TYPE_MEM64. In reality there are just 3 types
(PCI_REGION_TYPE_IO,
PCI_REGION_TYPE_MEM,PCI_REGION_TYPE_PREFMEM), so adding new types makes code
bulkier and less strait-forward.
In my implementation there is no need to create extra new region types, we are
still using standard types.
2. If we going to use existing approach and want to support 64bit bridges and
bars with size over 4GB
we need to extend arrays and rewrite all bus sizing functions:
struct {
/* pci region stats */
u32 count[32 - PCI_MEM_INDEX_SHIFT];
-u32 sum, max;
+u64 sum, max;
/* seconday bus region sizes */
-u32 size;
+u64 size;
/* pci region assignments */
-u32 bases[32 - PCI_MEM_INDEX_SHIFT];
-u32 base;
+u64 bases[32 - PCI_MEM_INDEX_SHIFT];
+u64 base;
} r[PCI_REGION_TYPE_COUNT];
In other words instead of 32 for bases and count arrays it must be (40 or
39). Plus we have to rewrite index to size, size to index,
size round up and so on
functions to make them 64bit capable.
My implementation does this without adding much code, we just declare size of
each entry.
3. About RAM use (may not be important), but lets count:
5 region types * (40 -PCI_MEM_INDEX_SHIFT ) * (sizeof(u64) + sizeof(u32)) =
1680 bytes for each bus.
if we don' bother about PCI_REGION_TYPE_MEM64 it will be 1344 bytes for each
bus. I didn't count other members so the size will be even more.
Plus we allocate 20 bytes * PCI_NUM_REGIONS = 140bytes for each pci device.
In my approach we are using just 16bytes for each region, so 48bytes per
bus, and 48bytes for each entry.
In conclusion:
Gerd's approach:
src/pci.h | 14 ++-
src/pciinit.c | 272 +
2 files changed, 206 insertions(+), 82 deletions(-)
+ Less marginal changes in code
- Bulkier code
- Doesn't support PCI bars and bridges with size over 4GB.
- Doesn't support 64bit non-prefetchable BARs on root bus.
My approach:
src/pci.h |6 -
src/pciinit.c | 509
[Qemu-devel] [Seabios] [PATCH 0/6] 64bit PCI BARs allocations (take 2)
Hi, This patch series enables 64bit BAR support in seabios. It has a bit different approach for resources accounting, We did this because we wanted: a) Provide 64bit bar support for PCI BARs and bridges with 64bit memory window. b) Allow migration to 64bit bit ranges if we did not fit into 32bit range c) Keep implementation simple. There are still have two main passes to enumerate resources and map devices, but structures are changed. We introduced two new structures: pci_region and pci_region_entry. The pci_region structure includes a list of pci_region_entries. Each pci_region_entry could be a PCI bar or a downstream PCI region (bridge). Each entry has a set of attributes: type (IO, MEM, PREFMEM), is64bit (if address can be over 4GB), size, base address, PCI device owner, and a pointer to the pci_region it belongs to. In the first pass we fill the pci_regions with entries and discover topology. In the second pass we try assigning memory addresses to pci_regions. If there is not enough space available the 64bit entries of root regions will be migrated to 64bit bit ranges and then we try assigning memory addresses again. Then each entry of each region will be mapped. The patch series includes 6 patches. In the 1st patch we introduce new structures. In the 2nd patch we introduce support functions for basic hlist operations, plus modify service functions to support 64bits address ranges. Note: I've seen similar hlist operations in post memory manager and stack location operations, it makes sense to move them to a header file. In the 3rd patch a new function to fill pci_region structures with entries, and discover topology is added. In the 4th patch we define address range for pci_region structure, migrate entries to 64bits address range if necessary, and program PCI BAR addresses and bridge regions. In the 6th patch we clear old code. And last patch is proposed by Michael Tsirkin, it contains changes in acpi-dsdt.dsl file those are necessary to support 64bit BARs in Windows. src/acpi-dsdt.dsl |7 + src/acpi-dsdt.hex | 72 ++-- src/config.h |2 + src/pci.h |6 - src/pciinit.c | 509 +--- 5 files changed, 352 insertions(+), 244 deletions(-) Note: At the moment there are three issues related to support of 64bit BARs in qemu (head of master branch). It's very likely they will be fixed in next qemu release. The 1st one is described here (this issue causing problems if 64bit BAR is mapped below 4GB and Linux guest OS version is 2.6.27): http://www.mail-archive.com/qemu-devel@nongnu.org/msg94522.html The 2nd one is just a typo in i440fx init code (this issue is causing problems if somebody is going to access 64bit PCI memory - memory will be inaccessible): http://www.mail-archive.com/qemu-devel@nongnu.org/msg99423.html The 3nd issue is related to a case of HUGE PCI bars when BAR size is 4GB and over. Qemu for some reasons reports zero size in this case. New seabios should handle huge bars well. I've sent the patches for the first two issues. If they are all applied problems except the huge BARs issue should gone.
