Xen devel - Nov 2008 - [PATCH 1/2][VTD] pci mmconfig support to be used for ATS

ATS (Address Translation Service) is a PCI specification used for synchronizing
iotlb translations between chipset iommu such as vt-d with iotlb on board a PCI
device.

This patch enables PCI mmconfig for Intel64 systems.  Most of the code were
copied from Linux.  This functionality is need for parsing ATS capability in
PCIe extended configuration space.

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> 20.11.08
02:42 >>>
>ATS (Address Translation Service) is a PCI specification used for
synchronizing iotlb translations between chipset iommu such as vt-d with iotlb
on board a PCI device.
>
>This patch enables PCI mmconfig for Intel64 systems.  Most of the code were
copied from Linux.  This functionality is need for parsing ATS capability in
PCIe extended configuration space.
If this is x86-64 only anyway, why do you need to use fixmap space for this (and
limit things arbitrarily to 4 segments) rather than using a 256Mb block from one
of the unused virtual address regions in hypervisor space?

Also, using obj-$(CONFIG_X86_64) in the Makefile changes would avoid building
dead code for x86-32...

Jan


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

I see that you have sort of started to introduce pci_ops functionality
into Xen.  However, the current patch looks very hackish and ugly in
my eyes.  I do undestand that you somehow need to get access to the
PCIe extended configuration space, but I would rather see this happen
in one of the following ways:

  - Use the regular PCI config access functions, and if accesses above
    the regular 255 registers are detected then use mmio (if
    available) for accesses.  The regular PCI configuration space is
    still always accessible using standard mechanisms.

  - Introduce pci_ops integrated properly into Xen internals.  You may
    even want to pci_ops on a per device or per bus basis.

The current proposal very much looks like a quick cut-n-paste hack.

I also see that you''ve modified struct pci_dev to accomomodate for
specifc ATS needs.  The ''bus'' and ''devfn''
fields have been made non
constant.  They were marked as ''const'' for a reason ---
modifying them
leads to race conditions.  Further, you don''t actually allocate the
pci_dev using the proper allocate function.  If you don''t want the ATS
unit to be treated as a PCI device in Xen then don''t use struct
pci_dev.  Create a separate struct ats_unit containing the fields you
need instead.

	eSk



[Allen M Kay]
> ATS (Address Translation Service) is a PCI specification used for
> synchronizing iotlb translations between chipset iommu such as vt-d
> with iotlb on board a PCI device.
> This patch enables PCI mmconfig for Intel64 systems.  Most of the
> code were copied from Linux.  This functionality is need for parsing
> ATS capability in PCIe extended configuration space.
> 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

>
>  - Use the regular PCI config access functions, and if accesses above
>    the regular 255 registers are detected then use mmio (if
>    available) for accesses.  The regular PCI configuration space is
>    still always accessible using standard mechanisms.
>
>  - Introduce pci_ops integrated properly into Xen internals.  You may
>    even want to pci_ops on a per device or per bus basis.
>
I will look into to see which of the above method is cleaner to implement.
>I also see that you''ve modified struct pci_dev to accomomodate for
>specifc ATS needs.  The ''bus'' and
''devfn'' fields have been made non
>constant.  They were marked as ''const'' for a reason ---
modifying them
>leads to race conditions.  Further, you don''t actually allocate the
>pci_dev using the proper allocate function.  If you don''t want the
ATS
>unit to be treated as a PCI device in Xen then don''t use struct
>pci_dev.  Create a separate struct ats_unit containing the fields you
>need instead.
>
I will define a new ATS structure instead of using the existing pci_dev
structure so that it will not disturb existing use model for pci_dev structure.

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

>
>If this is x86-64 only anyway, why do you need to use fixmap 
>space for this (and limit things arbitrarily to 4 segments) 
>rather than using a 256Mb block from one of the unused virtual 
>address regions in hypervisor space?
Can you point me how to use unused virtual address region to do ioremap without
using fixmap?  Is there a existing code that is already doing this?
>
>Also, using obj-$(CONFIG_X86_64) in the Makefile changes would 
>avoid building dead code for x86-32...
>
Will do.  Thanks!

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

On 20/11/08 20:10, "Kay, Allen M" <allen.m.kay@intel.com> wrote:
>> If this is x86-64 only anyway, why do you need to use fixmap
>> space for this (and limit things arbitrarily to 4 segments)
>> rather than using a 256Mb block from one of the unused virtual
>> address regions in hypervisor space?
> 
> Can you point me how to use unused virtual address region to do ioremap
> without using fixmap?  Is there a existing code that is already doing this?
Pick yourself a nice piece of virtual address space (see the layout in
include/asm-x86/config.h -- 0xffff840000000000 would be fine) and then use
map_pages_to_xen().

 -- Keir



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

>>> Keir Fraser <keir.fraser@eu.citrix.com> 20.11.08 21:20
>>>
>On 20/11/08 20:10, "Kay, Allen M" <allen.m.kay@intel.com>
wrote:
>
>>> If this is x86-64 only anyway, why do you need to use fixmap
>>> space for this (and limit things arbitrarily to 4 segments)
>>> rather than using a 256Mb block from one of the unused virtual
>>> address regions in hypervisor space?
>> 
>> Can you point me how to use unused virtual address region to do ioremap
>> without using fixmap?  Is there a existing code that is already doing
this?
>
>Pick yourself a nice piece of virtual address space (see the layout in
>include/asm-x86/config.h -- 0xffff840000000000 would be fine) and then use
>map_pages_to_xen().
While it can be adjusted later anyway, I''d suggest not using a part of
this
largest available range - sooner or later we''ll want to use this to
extend
the 1:1 mapping to 4Tb. Instead, I''d pick another 1Gb range out of the
461Gb reserved region.

Jan


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