Xen devel - Dec 2008 - [PATCH][VTD] pci mmcfg patch for x86-64 - version 2

Fixes made in version 2:

1) Use PML4[257] for ioremap of PCI mmcfg.  As full 16-bit segment support would
require 44-bits.  Since each slot only has 39-bits, we support 2048 PCI segments
for now.  This can be easily expanded if deemed necessary in the future.

2) Integrated PCI mmcfg access with existing PCI config interface for x86_64. 
Use MMCFG interface if offset is greater than 256.

Signed-off-by: Allen Kay <allen.m.kay@intel.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> "Kay, Allen M" <allen.m.kay@intel.com> 08.12.08
23:33 >>>
>+#define PCI_MCFG_VIRT_START     (PML4_ADDR(257))
>+#define PCI_MCFG_VIRT_END       (RDWR_MPT_VIRT_START + PML4_ENTRY_BYTES)
The latter line is certainly wrong.
>+static void __iomem * __init mcfg_ioremap(struct acpi_mcfg_allocation *cfg)
>+{
>+	void __iomem *addr;
>+	unsigned long virt;
>+	unsigned long mfn;
>+	unsigned long size, nr_mfn;
>+
>+        /* see asm-x86/config.h, only 2048 PCI segments are supported) */
>+        BUG_ON(cfg->pci_segment >= 2048);
This seems an inappropriate use of BUG_ON() - should either return NULL
here or map just the first 2048 segments (and check in other places
accordingly). Also, I''d say you shouldn''t have a literal 2048
here, but rather
calculate the number from PCI_MCFG_VIRT_END.
>+	virt = PCI_MCFG_VIRT_START + (cfg->pci_segment * (1 << 22)) +
>+               (cfg->start_bus_number * (1 << 20));
Isn''t the first (segment) shift value supposed to be 28? Also, you have
to
use 1UL there, otherwise the multiplication will get truncated to 32 bits.
>+	mfn = cfg->address >> PAGE_SHIFT;
>+        size = (cfg->end_bus_number - cfg->start_bus_number) <<
20;
Since end_bus_number is the last valid number, you need to add 1 here
I would think.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

I see that all the special pci mmcfg stuff has been put under the
x86_64 tree.  However, most of it is generic x86.  Why not make the
code mostly generic x86 and let the x86_32 specific part fail
gracefully (as in: mmcfg not configured properly)?  Support for x86_32
can then later be added without requiring code duplication or
restructuring.

	eSk


[Allen M Kay]
> Fixes made in version 2:
> 1) Use PML4[257] for ioremap of PCI mmcfg.  As full 16-bit segment
>    support would require 44-bits.  Since each slot only has 39-bits,
>    we support 2048 PCI segments for now.  This can be easily
>    expanded if deemed necessary in the future.
> 2) Integrated PCI mmcfg access with existing PCI config interface
>    for x86_64.  Use MMCFG interface if offset is greater than 256.
> Signed-off-by: Allen Kay <allen.m.kay@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Fixed.  Will include in the next revision.  Thanks!

Allen 
>-----Original Message-----
>From: Jan Beulich [mailto:jbeulich@novell.com] 
>Sent: Tuesday, December 09, 2008 2:50 AM
>To: Kay, Allen M
>Cc: Keir Fraser; Han, Weidong; xen-devel@lists.xensource.com; 
>Espen Skoglund
>Subject: Re: [PATCH][VTD] pci mmcfg patch for x86-64 - version 2
>
>>>> "Kay, Allen M" <allen.m.kay@intel.com> 08.12.08
23:33 >>>
>>+#define PCI_MCFG_VIRT_START     (PML4_ADDR(257))
>>+#define PCI_MCFG_VIRT_END       (RDWR_MPT_VIRT_START + 
>PML4_ENTRY_BYTES)
>
>The latter line is certainly wrong.
>
>>+static void __iomem * __init mcfg_ioremap(struct 
>acpi_mcfg_allocation *cfg)
>>+{
>>+	void __iomem *addr;
>>+	unsigned long virt;
>>+	unsigned long mfn;
>>+	unsigned long size, nr_mfn;
>>+
>>+        /* see asm-x86/config.h, only 2048 PCI segments are 
>supported) */
>>+        BUG_ON(cfg->pci_segment >= 2048);
>
>This seems an inappropriate use of BUG_ON() - should either return NULL
>here or map just the first 2048 segments (and check in other places
>accordingly). Also, I''d say you shouldn''t have a literal
2048
>here, but rather
>calculate the number from PCI_MCFG_VIRT_END.
>
>>+	virt = PCI_MCFG_VIRT_START + (cfg->pci_segment * (1 << 22)) +
>>+               (cfg->start_bus_number * (1 << 20));
>
>Isn''t the first (segment) shift value supposed to be 28? Also, 
>you have to
>use 1UL there, otherwise the multiplication will get truncated 
>to 32 bits.
>
>>+	mfn = cfg->address >> PAGE_SHIFT;
>>+        size = (cfg->end_bus_number - cfg->start_bus_number)
<< 20;
>
>Since end_bus_number is the last valid number, you need to add 1 here
>I would think.
>
>Jan
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

I have moved the files into arch/x86 directory, added mmconfig_32.c but with
pci_mmcfg_read/write() stubbed out - returning appropriate error values.

I will send out 3rd version after I fix a minor issue. 

Allen 
>-----Original Message-----
>From: Espen Skoglund [mailto:espen.skoglund@netronome.com] 
>Sent: Tuesday, December 09, 2008 4:41 AM
>To: Kay, Allen M
>Cc: xen-devel@lists.xensource.com; Han, Weidong; Espen 
>Skoglund; Keir Fraser
>Subject: Re: [Xen-devel] [PATCH][VTD] pci mmcfg patch for 
>x86-64 - version 2
>
>I see that all the special pci mmcfg stuff has been put under the
>x86_64 tree.  However, most of it is generic x86.  Why not make the
>code mostly generic x86 and let the x86_32 specific part fail
>gracefully (as in: mmcfg not configured properly)?  Support for x86_32
>can then later be added without requiring code duplication or
>restructuring.
>
>        eSk
>
>
>[Allen M Kay]
>> Fixes made in version 2:
>> 1) Use PML4[257] for ioremap of PCI mmcfg.  As full 16-bit segment
>>    support would require 44-bits.  Since each slot only has 39-bits,
>>    we support 2048 PCI segments for now.  This can be easily
>>    expanded if deemed necessary in the future.
>
>> 2) Integrated PCI mmcfg access with existing PCI config interface
>>    for x86_64.  Use MMCFG interface if offset is greater than 256.
>
>> Signed-off-by: Allen Kay <allen.m.kay@intel.com>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel