Re: [Xen-devel] [PATCH 2/2] ioemu: Enable guest OS to program D0-D3hot states of an assigned device

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Yuji Shimada writes ("[Xen-devel] [PATCH 2/2] ioemu: Enable guest OS to program 
D0-D3hot states of an assigned device"):
> This patch enables guest OS to program D0-D3hot states of assigned
> device.

I applied this patch but unfortunately it broke the automatic test
(and thus the staging propagation) because our automatic test system
has a very old version of libpci.  I have applied a band-aid - see
below - but would you please review it and consider whether a
different fix would be correct ?  For example, we might copy the
definitions of PCI_ERR_* from the libpci headers into pass-through.h
protected by an #ifdef, or something.

Also, this logic is very very strange:

> +    int aer_size = 0x2c;
> +
> +    for (i=0; i < aer_size; i+=4)
> +    {
> +        switch (i) {
> +        /* after reset, following register values should be restored.
> +         * So, save them.
> +         */
> +        case PCI_ERR_UNCOR_MASK:
> +        case PCI_ERR_UNCOR_SEVER:
> +        case PCI_ERR_COR_MASK:
> +        case PCI_ERR_CAP:
> +            *(uint32_t*)(d->config + (aer_base + i))
> +                 = pci_read_long(ptdev->pci_dev, (aer_base + i));
> +            break;
> +        default:
> +            break;
> +        }
> +    }

What purpose does the loop serve ?  Is there an ordering constraint ?
There are two copies of this construct, too - one in the save and one
in the restore.  That duplication is rather unfortunate.

Thanks,
Ian.

commit b80fc65b715099ee28465f6571eb5242588ef246
Author: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Date:   Fri Feb 6 16:16:06 2009 +0000

    hw/pass-through.c: workaround for old libpci
    
    Old versions of libpci (including the ones on the automatic tests
    which control Xen staging propagation) do not define
    PCI_LIB_VERSION or the PCI_ERR_{UNCOR_MASK,...} constants.
    
    This means that change 8c771eb6294afc5b3754a9e3de51568d4e5986c2 breaks
    the build.  In this changeset I apply what is intended to be a
    workaround for this problem but it may not be completely correct; this
    is therefore perhaps an interim fix.
    
    The potential problem is that the save/restore of some PCI passthrough
    error handling registers (across suspend/resume) may not work properly
    with old versions of libpci.  However non-passthrough and non-suspect
    use cases should now be fine.
    
    Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

diff --git a/hw/pass-through.c b/hw/pass-through.c
index 8dfae3c..29714c7 100644
--- a/hw/pass-through.c
+++ b/hw/pass-through.c
@@ -1503,7 +1503,7 @@ exit:
 
 static void pt_libpci_fixup(struct pci_dev *dev)
 {
-#if PCI_LIB_VERSION < 0x030100
+#if !defined(PCI_LIB_VERSION) || PCI_LIB_VERSION < 0x030100
     int i;
     FILE *fp;
     char path[PATH_MAX], buf[256];
@@ -1850,6 +1850,7 @@ static void pt_aer_reg_save(struct pt_dev *ptdev)
     /* Root Port and Root Complex Event Collector need size expansion */
     int aer_size = 0x2c;
 
+#ifdef PCI_ERR_UNCOR_MASK
     for (i=0; i < aer_size; i+=4)
     {
         switch (i) {
@@ -1867,6 +1868,7 @@ static void pt_aer_reg_save(struct pt_dev *ptdev)
             break;
         }
     }
+#endif
 }
 
 /* restore AER register */
@@ -1879,6 +1881,7 @@ static void pt_aer_reg_restore(struct pt_dev *ptdev)
     /* Root Port and Root Complex Event Collector need size expansion */
     int aer_size = 0x2c;
 
+#ifdef PCI_ERR_UNCOR_MASK
     for (i=0; i < aer_size; i+=4)
     {
         switch (i) {
@@ -1899,6 +1902,7 @@ static void pt_aer_reg_restore(struct pt_dev *ptdev)
             break;
         }
     }
+#endif
 }
 
 /* reset Interrupt and I/O resource  */

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