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

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

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

I was trying to maintain the same names as Linux for ease of reference.  You 
are right, it would be more appropriate to follow Xen directory structure here. 
 I also tried to maintain file format of mmconfig_x?? to conform to Linux as 
they are pretty much straight copies of Linux files.  Sounds like these file 
should also be converted to Xen indentation here.

>>+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?
>

Yes it should return -EINVAL as in the original Linux code.  The change to 0 
was unintentional.

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

OK, I will add the comments back in.  Where is pci_mmcfg_amd_family10h() 
defined?  I'm having trouble finding it.

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

My original intent was to keep mmconfig-shared.c and mmconfig_64.c as closely 
to Linux as possible.  However, I soon found out including everything in 
mmconfig-shared.c involved pulling in a lot of code from Linux.

As my main goal of this round is enabling ATS, I tried to limited the scope of 
mmconfig work to be somewhat manageable for now and then add more stuff to it 
as needed.  As I'm planning to work on multi-segment PCI support in Q1, this 
stuff will get revisited again.

Other than E820 checking, do you see anything else in mmconfig-shared.c need to 
be included for this round?

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