>>> "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));
by
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?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home