[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Xen-devel] Re: [PATCH][VTD] pci mmcfg patch for x86-64 - version 3

>>> "Kay, Allen M" <allen.m.kay@xxxxxxxxx> 10.12.08 04:14 >>>
>This is version 3 of mmconfig patch that addresses all of the feedbacks I 
>received from Jan and Espen yesterday. 

Thanks. Two more (new) items, though:

>--- a/xen/arch/x86/Makefile    Tue Dec 09 16:28:02 2008 +0000
>+++ b/xen/arch/x86/Makefile    Tue Dec 09 06:23:29 2008 -0800
>@@ -54,6 +54,9 @@ obj-y += crash.o
> obj-y += crash.o
> obj-y += tboot.o
> obj-y += hpet.o
>+obj-y += mmconfig-shared.o
>+obj-$(CONFIG_X86_64) += mmconfig_64.o
>+obj-$(CONFIG_X86_32) += mmconfig_32.o

These should now really go into xen/arch/x86/x86_{32,64}/ rather than
adding _{32,64} suffixes.

>+int pci_mmcfg_read(unsigned int seg, unsigned int bus,
>+                        unsigned int devfn, int reg, int len, u32 *value)
>+      *value = -1;
>+      return 0;

Shouldn't you return -EINVAL (or -ENOSYS, but an error in any case) here?

And one thing I didn't notice before:

>+/* used by mmcfg */
>+static inline unsigned char mmio_config_readb(void __iomem *pos)

This is preceded by a rather important comment regarding AMD Fam10
CPUs in Linux. Without that comment, no-one will easily understand why
you must use eax/ax/al here. I'm also surprised you didn't copy over
pci_mmcfg_amd_fam10h() from Linux...

And as far as I'm concerned I would not exactly follow Linux in how the
assembly is written, e.g. replace

+       asm volatile("movb (%1),%%al" : "=a" (val) : "r" (pos));


    asm volatile("movb (%1),%0" : "=a" (val) : "r" (pos));

(and use proper Xen-style indentation).

Oh, and one more thing I remembered just now: Linux verifies the mmcfg
values it gets from ACPI against the E820 map - shouldn't Xen do this too?


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.