Re: [Qemu-devel] [PATCH v4 2/3] hw/pci: Introduce pci_requester_id()

[Michael S. Tsirkin]

On Thu, Oct 15, 2015 at 11:54:57AM +0300, Pavel Fedin wrote:
>  Hello!
> 
> > > diff --git a/stubs/pci.c b/stubs/pci.c
> > > new file mode 100644
> > > index 0000000..3b13000
> > > --- /dev/null
> > > +++ b/stubs/pci.c
> > > @@ -0,0 +1,16 @@
> > > +/*
> > > + * QEMU architecture-specific PCI functions
> > > + *
> > > + * Copyright (c) 2015 Samsung Electronics Co., Ltd.
> > > + * Written by Pavel Fedin
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 or 
> > > later.
> > > + * See the COPYING file in the top-level directory.
> > > + *
> > > + */
> > > +#include "hw/pci/pci.h"
> > > +
> > > +uint16_t pci_requester_id(PCIDevice *dev)
> > > +{
> > > +    return 0;
> > > +}
> > 
> > OK this is now wrong. You should move the logic back here
> > from target-arm.
> 
>  Thank you very much for quick responses and cooperation, but... Could we 
> discuss a little bit, what you actually want as a result?
> This starts to be very inefficient and time-consuming. Every time i redo the 
> whole thing only to hear that "this single line is now
> wrong".
>  So far, previously you told me that since requester ID is 
> architecture-specific, if should be moved to arch code.

Sorry about that, it does sometimes take a while and some back and forth
to arrive at the correct approach.

So originally you had an msi_requester_id thing. That didn't mean
anything (maybe outside ARM). So I said put it in ARM then.

But you still had a stub in pci code, and I didn't like that.
Then I went back and actually looked at what it is, to see
whether we can get rid of it completely.

Then I realized it's actually the PCI requester ID.
That, in turn, means it can just be a generic PCI API,
e.g. it's also used for assigning pci-x devices.

> Now you tell me
> that you want it in stubs... Why? Stubs are something to be replaced. And we 
> won't have the replacement anywhere, because nowadays
> no architecture except ARM uses it. And probably it will stay this way 
> forever. The function is very small, and even if we add more
> bits for root complex ID, it will stay very small. Why not to have it as 
> inline in include/hw/pci.h? If we ever need to replace it
> in future, then we'll move it to stubs and override in target-XXX.
>  And, what with other two patches? Actually, all three are independent, i 
> combined them into one series only because they are part
> of original bigger series. You didn't have any comments on them, could you 
> ACK them finally in order to simplify things down?
> 
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
>