Xen devel - Dec 2008 - [PATCH][VTD] pci mmcfg patch for x86-64 - version 3

This is version 3 of mmconfig patch that addresses all of the feedbacks I
received from Jan and Espen yesterday.

Signed-off-by: Allen Kay <allen.m.kay@intel.com>

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

>>> "Kay, Allen M" <allen.m.kay@intel.com> 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@lists.xensource.com
http://lists.xensource.com/xen-devel

[Allen M Kay]
> This is version 3 of mmconfig patch that addresses all of the
> feedbacks I received from Jan and Espen yesterday.
> Signed-off-by: Allen Kay <allen.m.kay@intel.com>
Some comments inline.

	eSk


===============================================================
> diff -r 6595393a3d28 xen/arch/x86/mmconfig_32.c
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/xen/arch/x86/mmconfig_32.c	Tue Dec 09 06:42:04 2008 -0800
> +int pci_mmcfg_read(unsigned int seg, unsigned int bus,
> +			  unsigned int devfn, int reg, int len, u32 *value)
> +{
> +	*value = -1;
> +	return 0;
> +}
> +
> +int pci_mmcfg_write(unsigned int seg, unsigned int bus,
> +			   unsigned int devfn, int reg, int len, u32 value)
> +{
> +	return -EINVAL;
> +}
These two can be removed (see further down).
> +int __init pci_mmcfg_arch_init(void)
> +{
> +	return 1;
> +}
Should return 0.  And add a dummy pci_dev_base() function.

> diff -r 6595393a3d28 xen/arch/x86/mmconfig_64.c
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/xen/arch/x86/mmconfig_64.c	Tue Dec 09 06:42:13 2008 -0800
Put following two functions in generic x86.
> +int pci_mmcfg_read(unsigned int seg, unsigned int bus,
> +			  unsigned int devfn, int reg, int len, u32 *value)
> +{
> +	char __iomem *addr;
> +
> +	/* Why do we have this when nobody checks it. How about a BUG()!? -AK */
> +	if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) {
> +err:		*value = -1;
> +		return -EINVAL;
> +	}
Also fail if (pci_probe & PCI_PROBE_MMCONF) == 0.  Or alternatively do
this check in pci_dev_base().
> +	addr = pci_dev_base(seg, bus, devfn);
> +	if (!addr)
> +		goto err;
> +
> +	switch (len) {
> +	case 1:
> +		*value = mmio_config_readb(addr + reg);
> +		break;
> +	case 2:
> +		*value = mmio_config_readw(addr + reg);
> +		break;
> +	case 4:
> +		*value = mmio_config_readl(addr + reg);
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +int pci_mmcfg_write(unsigned int seg, unsigned int bus,
> +			   unsigned int devfn, int reg, int len, u32 value)
> +{
> +	char __iomem *addr;
> +
> +	/* Why do we have this when nobody checks it. How about a BUG()!? -AK */
> +	if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095)))
> +		return -EINVAL;
Also fail if (pci_probe & PCI_PROBE_MMCONF) == 0.
> +	addr = pci_dev_base(seg, bus, devfn);
> +	if (!addr)
> +		return -EINVAL;
> +
> +	switch (len) {
> +	case 1:
> +		mmio_config_writeb(addr + reg, value);
> +		break;
> +	case 2:
> +		mmio_config_writew(addr + reg, value);
> +		break;
> +	case 4:
> +		mmio_config_writel(addr + reg, value);
> +		break;
> +	}
> +
> +	return 0;
> +}

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

>
>>--- 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@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> "Kay, Allen M" <allen.m.kay@intel.com> 12.12.08
03:47 >>>
>>>+/* 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.
The name really is pci_mmcfg_amd_fam10h(), and it''s in
2.6.27''s
arch/x86/pci/mmconfig-shared.c, right along with the other host bridge
probe functions.
>>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.
Understandable. But this particular check may save us from dying on systems
where Linux works. And it shouldn''t be that much code that needs adding
here if you restrict this to the E820 check; I''m not certain how
significant
the ACPI check is (which would require Dom0 assistance in my opinion, as
it requires the ACPI interpreter to be available).
>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.
Is that going to come through Linux again, or is this a Xen-only plan?
>Other than E820 checking, do you see anything else in mmconfig-shared.c need
to be included for this round?
I didn''t notice anything.

Jan


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

> >>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.
> 
> Understandable. But this particular check may save us from dying on systems
> where Linux works. And it shouldn''t be that much code that needs
adding
My recollection of this was that the check was added for buggy pre-release
Intel boxes (the address that was coded in the ACPI MMCONFIG table was
completly bogus). Follow this link for more details:
http://lkml.indiana.edu/hypermail/linux/kernel/0605.2/0923.html

I honestly can''t remember anymore the details of the E820 check, but
the
check isn''t that difficult:

Read the addresses and make sure they are the host E820 as reserved.
> here if you restrict this to the E820 check; I''m not certain how
significant
> the ACPI check is (which would require Dom0 assistance in my opinion, as
> it requires the ACPI interpreter to be available).

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

>As I''m planning to 
>work on multi-segment PCI support in Q1, this stuff will get revisited
>>again.
>
>Is that going to come through Linux again, or is this a Xen-only plan?
>
It will involve both xen and Linux.  Segment information for the device will
come from dom0 Linux.  Segment knowledge in VT-d will be added in xen.  For
example, current vtd/dmar.c/acpi_find_matched_drhd_unit() assumes the entire
system has only one INCLUDE_ALL vt-d engine.  In a multi PCI segment system,
each segment can have a INCLUDE_ALL VT-d engine.

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

>> +int __init pci_mmcfg_arch_init(void)
>> +{
>> +	return 1;
>> +}
>
>Should return 0.  And add a dummy pci_dev_base() function.
>
>
Done.  Added pci_dev_base() returning 0 in x86_32 version of mmconfig.c.

>> diff -r 6595393a3d28 xen/arch/x86/mmconfig_64.c
>> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>> +++ b/xen/arch/x86/mmconfig_64.c	Tue Dec 09 06:42:13 2008 -0800
>
>Put following two functions in generic x86.
>
>> +int pci_mmcfg_read(unsigned int seg, unsigned int bus,
>> +			  unsigned int devfn, int reg, int len, 
>u32 *value)
>> +{
>> +	char __iomem *addr;
>> +
>> +	/* Why do we have this when nobody checks it. How about 
>a BUG()!? -AK */
>> +	if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095)))
{
>> +err:		*value = -1;
>> +		return -EINVAL;
>> +	}
>
>Also fail if (pci_probe & PCI_PROBE_MMCONF) == 0.  Or alternatively do
>this check in pci_dev_base().
>
I added the check in both read/write functions instead of pci_dev_base().  It is
probably better from maintainence point as it is more straight forward.

Allen

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