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

[Yuji Shimada <shimada-yxb@xxxxxxxxxxxxxxx>]

On Fri, 6 Feb 2009 16:20:55 +0000
Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> wrote:

> 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.

I think coping the definitions of PCI_ERR_* is better, because AER
registers should be saved and restored. The reason is BIOS might
configure AER registers to achieve system-specific requirement.

Actually, the same issue occurred with other registers. We have fixed the
issue, coping the definition from libpci to pass-through.h into
#ifndef/#endif section.

This is a example.

#ifndef PCI_MSI_FLAGS_MASK_BIT
/* interrupt masking & reporting supported */
#define PCI_MSI_FLAGS_MASK_BIT  0x0100
#endif

> 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.

There is no ordering constraint. The purpose is only
saving/restoring the registers. Is there any good way to cleanup them?

Thanks,
--
Yuji Shimada

> 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