[Xen-devel] RE: [PATCH][VTD] pci mmcfg patch for x86-64 - version 2

["Kay, Allen M" <allen.m.kay@xxxxxxxxx>]

Fixed.  Will include in the next revision.  Thanks!

Allen 

>-----Original Message-----
>From: Jan Beulich [mailto:jbeulich@xxxxxxxxxx] 
>Sent: Tuesday, December 09, 2008 2:50 AM
>To: Kay, Allen M
>Cc: Keir Fraser; Han, Weidong; xen-devel@xxxxxxxxxxxxxxxxxxx; 
>Espen Skoglund
>Subject: Re: [PATCH][VTD] pci mmcfg patch for x86-64 - version 2
>
>>>> "Kay, Allen M" <allen.m.kay@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel