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.)
> > 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);
}
Or
#define AER_SAVE_ONE_REGISTER(i) \
(*(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);
...
And if you want to make the list of registers common between the save
and restore code you could do
#define AER_SAVE_REGS_LIST(each) \
each(PCI_ERR_UNCOR_MASK) \
each(PCI_ERR_UNCOR_SEVER) \
each(PCI_ERR_COR_MASK) \
each(PCI_ERR_CAP)
#define AER_SAVE_ONE_REGISTER(i) \
(*(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_REGS_LIST(AER_SAVE_ONE_REGISTER)
}
or
static const int aer_regs_to_save[]= {
PCI_ERR_UNCOR_MASK,
PCI_ERR_UNCOR_SEVER,
PCI_ERR_COR_MASK,
PCI_ERR_CAP,
-1
}
static void pt_aer_reg_save(struct pt_dev *ptdev) {
start of function unchanged
int *reg;
for (reg = aer_regs_to_save; *reg >= 0; reg++) {
i = *reg;
*(uint32_t*)(d->config + (aer_base + i))
= pci_read_long(ptdev->pci_dev, (aer_base + i));
}
}
But those may be overkill to avoid duplicating a four-element list.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home