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