On Tue, 10 Feb 2009 11:13:08 +0000
Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> wrote:
> Yuji Shimada writes ("Re: [Xen-devel] [PATCH 2/2] ioemu: Enable guest OS to
> program D0-D3hot states of an assigned device"):
> > 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.
>
> I see. Do you want me to just do that or should I expect a patch from
> you ? (I have a recent pciutils-dev here which I could cut-and-paste
> from.)
I am creating the patch to cleanup pass-through.c now. So I will make
the patch include fixing PCI_ERR_* and removing the loop.
> > > 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?
>
> The loop serves no purpose. You can just directly do the saving for
> each value in turn. There are many possible more normal ways to do
> this. For example,
>
> static void aer_save_one_register(int i, loads of other parameters) {
> *(uint32_t*)(d->config + (aer_base + i))
> = pci_read_long(ptdev->pci_dev, (aer_base + i));
> }
>
> static void pt_aer_reg_save(struct pt_dev *ptdev) {
> start of function unchanged
> aer_save_one_register(PCI_ERR_UNCOR_MASK, other parameters);
> aer_save_one_register(PCI_ERR_UNCOR_SEVER, other parameters);
> aer_save_one_register(PCI_ERR_COR_MASK, other parameters);
> aer_save_one_register(PCI_ERR_CAP, other parameters);
> }
This is better than the loop.
I will remove the loop.
Thanks,
--
Yuji Shimada
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home