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

[Espen Skoglund <espen.skoglund@xxxxxxxxxxxxx>]

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@xxxxxxxxx>

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