>>> On 14.10.10 at 02:05, "Kay, Allen M" <allen.m.kay@xxxxxxxxx> wrote:
> Consolidate VT-d quirks into a single file - quirks.c. This includes quirks
> to
> workaround OEM BIOS issue with VT-d enabling in IGD, Cantiga VT-d buffer
> flush
> issue, Cantiga IGD Vt-d low power related errata, and a quirk to workaround
> issues related to wifi direct assignment.
>
> Signed-off-by: Allen Kay allen.m.kay@xxxxxxxxx
Hmm, I was actually e.g. expecting the extra parameter c/s had
added to acpi_parse_dev_scope() to go away altogether again.
Below is the relevant portion of the reduced equivalent of your
original patch that we currently apply to 4.0.1 - note that
acpi_parse_dev_scope() even before this already did similar
type dependent work using acpi_entry.
>+static int map_igd_reg(void)
>+{
>+ u64 igd_mmio, igd_reg;
>+ int status;
>+
>+ if ( igd_reg_va != NULL )
>+ return 0;
>+
>+ igd_mmio = ((u64)pci_conf_read32(0, INTEL_IGD_DEV, 0, 0x14) << 32) +
>+ pci_conf_read32(0, INTEL_IGD_DEV, 0, 0x10);
>+ igd_reg = (igd_mmio & IGD_BAR_MASK) + 0x2000;
>+ status = map_pages_to_xen(igd_reg, igd_reg >> PAGE_SHIFT, 1,
>+ __PAGE_HYPERVISOR);
You're mapping to *virtual* address igd_reg here? Even if this was
done only at boot time it would seem wrong, but as I understand it
this is done so even at run time, which is an absolute no-go.
>+static void cantiga_vtd_ops_preamble(struct iommu* iommu)
>+{
>+ struct intel_iommu *intel = iommu->intel;
>+ struct acpi_drhd_unit *drhd = intel ? intel->drhd : NULL;
>+ int data;
>+
>+ if ( !is_igd_drhd(drhd) || !is_cantiga_b3 )
>+ return;
>+
>+ if ( map_igd_reg() )
>+ return;
>+
>+ /* just want to read from the regster, don't need the data */
>+ data = *((int*)(igd_reg_va + 0x20A4));
This needs a volatile qualifier or consumer of "data", or else the
compiler will very likely eliminate the memory read.
Also the comment above map_igd_reg() talks about reading a
PCI config space register, but here you do a thus unexplained
memory read (on top of the explained config space accesses in
map_igd_reg()).
>+void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, u8 map)
>+{
>+ struct acpi_drhd_unit *drhd;
>+ struct pci_dev *pdev;
>+ u32 id;
>+
>+ id = pci_conf_read32(0, 0, 0, 0);
>+ if ( IS_CTG(id) )
>+ {
>+ /* quit if ME does not exist */
>+ if ( pci_conf_read32(0, 3, 0, 0) == 0xffffffff )
>+ return;
>+
>+ /* if device is WLAN device, map ME phantom device 0:3.7 */
>+ id = pci_conf_read32(bus, PCI_SLOT(devfn), PCI_FUNC(devfn), 0);
>+ switch (id)
>+ {
>+ case 0x42328086:
>+ case 0x42358086:
>+ case 0x42368086:
>+ case 0x42378086:
>+ case 0x423a8086:
>+ case 0x423b8086:
>+ case 0x423c8086:
>+ case 0x423d8086:
>+ /* find ME VT-d engine base on a real ME device 0:3.0 */
>+ pdev = pci_get_pdev(0, PCI_DEVFN(3, 0));
>+ drhd = acpi_find_matched_drhd_unit(pdev);
>+
>+ /* map or unmap ME phantom function 0:3.7 */
>+ if ( map )
>+ domain_context_mapping_one(domain, drhd->iommu, 0,
>PCI_DEVFN(3, 7));
>+ else
>+ domain_context_unmap_one(domain, drhd->iommu, 0,
>PCI_DEVFN(3, 7));
>+
>+ break;
>+ default:
>+ break;
>+ }
>+ }
This and the subsequent else if's body look identical except for the
former using device 3 and the latter using device 22. Can't these
be folded, thus making the code smaller and easier to read (I
assume that the device IDs checked for in the switch statement are
actually unique, i.e. are known to only occur as either device 3 or
device 22)?
One general comment: Reviewing would be easier if you generated
your patches with -p.
Jan
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -46,6 +46,7 @@ LIST_HEAD(acpi_rmrr_units);
LIST_HEAD(acpi_atsr_units);
LIST_HEAD(acpi_rhsa_units);
+static u64 igd_drhd_address;
u8 dmar_host_address_width;
void dmar_scope_add_buses(struct dmar_scope *scope, u16 sec_bus, u16 sub_bus)
@@ -239,6 +240,11 @@ struct acpi_rhsa_unit * drhd_to_rhsa(str
return NULL;
}
+int is_igd_drhd(struct acpi_drhd_unit *drhd)
+{
+ return ( drhd->address == igd_drhd_address ? 1 : 0);
+}
+
/*
* Count number of devices in device scope. Do not include PCI sub
* hierarchies.
@@ -333,6 +339,15 @@ static int __init acpi_parse_dev_scope(v
if ( iommu_verbose )
dprintk(VTDPREFIX, " endpoint: %x:%x.%x\n",
bus, path->dev, path->fn);
+
+ if ( type == DMAR_TYPE )
+ {
+ struct acpi_drhd_unit *drhd = acpi_entry;
+
+ if ( (bus == 0) && (path->dev == 2) && (path->fn == 0) )
+ igd_drhd_address = drhd->address;
+ }
+
break;
case ACPI_DEV_IOAPIC:
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home