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