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

["Jan Beulich" <jbeulich@xxxxxxxxxx>]

>>> "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