Re: [Xen-devel] [PATCH] AMD IOMMU: Enable HPET broadcast msi remapping

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.10.12 at 15:56, Wei Wang <wei.wang2@xxxxxxx> wrote:
>--- a/xen/drivers/passthrough/amd/iommu_acpi.c
>+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>@@ -653,9 +653,25 @@ static u16 __init parse_ivhd_device_special(
>     }
> 
>     add_ivrs_mapping_entry(bdf, bdf, special->header.data_setting, iommu);
>+    
>+    switch ( special->variety )
>+    {
>+    case 1:

Please use the existing #define-s for this and the HPET one below.

>     /* set device id of ioapic */
>-    ioapic_sbdf[special->handle].bdf = bdf;
>-    ioapic_sbdf[special->handle].seg = seg;
>+        ioapic_sbdf[special->handle].bdf = bdf;
>+        ioapic_sbdf[special->handle].seg = seg;
>+        break;
>+    case 2:
>+        /* set device id of hpet */
>+        hpet_sbdf.id = special->handle;
>+        hpet_sbdf.bdf = bdf;
>+        hpet_sbdf.seg = seg;
>+        hpet_sbdf.iommu = iommu;

So there can only ever be a single HPET? Is that written down in
the spec somewhere? Otherwise I would at least want a warning
to be issued if we unexpectedly find a second one (and probably
avoid clobbering the already saved data).

>@@ -267,14 +268,16 @@ static void update_intremap_entry_from_msi_msg(
> {
>     unsigned long flags;
>     u32* entry;
>-    u16 bdf, req_id, alias_id;
>+    u16 seg, bdf, req_id, alias_id;
>     u8 delivery_mode, dest, vector, dest_mode;
>     spinlock_t *lock;
>     int offset;
> 
>-    bdf = PCI_BDF2(pdev->bus, pdev->devfn);
>-    req_id = get_dma_requestor_id(pdev->seg, bdf);
>-    alias_id = get_intremap_requestor_id(pdev->seg, bdf);
>+    bdf = pdev ? PCI_BDF2(pdev->bus, pdev->devfn) : hpet_sbdf.bdf;
>+    seg = pdev ? pdev->seg : hpet_sbdf.seg;
>+    
>+    req_id = get_dma_requestor_id(seg, bdf);
>+    alias_id = get_intremap_requestor_id(seg, bdf);
> 
>     if ( msg == NULL )
>     {

As you're replacing all further uses of pdev in this function anyway,
and the single caller already determines seg and bdf, please replace
the passing of "struct pci_dev *" by passing "bdf" and using
"iommu->seg". That'll also make obvious that no left over unchecked
use of "pdev" exists here (and avoids any getting re-added later).

>--- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
>+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
>@@ -97,12 +97,18 @@ void amd_iommu_msi_msg_update_ire(
>     struct msi_desc *msi_desc, struct msi_msg *msg);
> void amd_iommu_read_msi_from_ire(
>     struct msi_desc *msi_desc, struct msi_msg *msg);
>+int __init amd_setup_hpet_msi(struct msi_desc *msi_desc);

No __init on a declaration please.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel