[v4,2/3] hw/pci: Introduce pci_requester_id()

For GICv3 ITS implementation we are going to use requester IDs in KVM IRQ
routing code. This patch introduces reusable convenient way to obtain this
ID from the device pointer.

Since requester ID is an architecture-specific thing, the function can be
overridden on per-architecture basis. Default stub just returns 0.

MemTxAttrs.stream_id also renamed to requester_id in order to better
reflect semantics of the field.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 hw/pci/msi.c             |  2 +-
 include/exec/memattrs.h  |  4 ++--
 include/hw/pci/pci.h     |  1 +
 stubs/Makefile.objs      |  1 +
 stubs/pci.c              | 16 ++++++++++++++++
 target-arm/Makefile.objs |  1 +
 target-arm/pci.c         | 16 ++++++++++++++++
 7 files changed, 38 insertions(+), 3 deletions(-)
 create mode 100644 stubs/pci.c
 create mode 100644 target-arm/pci.c

On Wed, Oct 14, 2015 at 06:06:37PM +0300, Pavel Fedin wrote:
> For GICv3 ITS implementation we are going to use requester IDs in KVM IRQ
> routing code. This patch introduces reusable convenient way to obtain this
> ID from the device pointer.
> 
> Since requester ID is an architecture-specific thing, the function can be
> overridden on per-architecture basis. Default stub just returns 0.
> 
> MemTxAttrs.stream_id also renamed to requester_id in order to better
> reflect semantics of the field.
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  hw/pci/msi.c             |  2 +-
>  include/exec/memattrs.h  |  4 ++--
>  include/hw/pci/pci.h     |  1 +
>  stubs/Makefile.objs      |  1 +
>  stubs/pci.c              | 16 ++++++++++++++++
>  target-arm/Makefile.objs |  1 +
>  target-arm/pci.c         | 16 ++++++++++++++++
>  7 files changed, 38 insertions(+), 3 deletions(-)
>  create mode 100644 stubs/pci.c
>  create mode 100644 target-arm/pci.c
> 
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index f9c0484..c1dd531 100644
> --- a/hw/pci/msi.c
> +++ b/hw/pci/msi.c
> @@ -294,7 +294,7 @@ void msi_send_message(PCIDevice *dev, MSIMessage msg)
>  {
>      MemTxAttrs attrs = {};
>  
> -    attrs.stream_id = (pci_bus_num(dev->bus) << 8) | dev->devfn;
> +    attrs.requester_id = pci_requester_id(dev);
>      address_space_stl_le(&dev->bus_master_as, msg.address, msg.data,
>                           attrs, NULL);
>  }
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index f8537a8..e601061 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -35,8 +35,8 @@ typedef struct MemTxAttrs {
>      unsigned int secure:1;
>      /* Memory access is usermode (unprivileged) */
>      unsigned int user:1;
> -    /* Stream ID (for MSI for example) */
> -    unsigned int stream_id:16;
> +    /* Requester ID (for MSI for example) */
> +    unsigned int requester_id:16;
>  } MemTxAttrs;
>  
>  /* Bus masters which don't specify any attributes will get this,
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 551cb3d..708dc3a 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -381,6 +381,7 @@ void pci_bus_fire_intx_routing_notifier(PCIBus *bus);
>  void pci_device_set_intx_routing_notifier(PCIDevice *dev,
>                                            PCIINTxRoutingNotifier notifier);
>  void pci_device_reset(PCIDevice *dev);
> +uint16_t pci_requester_id(PCIDevice *dev);
>  
>  PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus,
>                                 const char *default_model,
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 85e4e81..dbd69a9 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -22,6 +22,7 @@ stub-obj-y += migr-blocker.o
>  stub-obj-y += mon-is-qmp.o
>  stub-obj-y += mon-printf.o
>  stub-obj-y += monitor-init.o
> +stub-obj-$(CONFIG_PCI) += pci.o
>  stub-obj-y += notify-event.o
>  stub-obj-$(CONFIG_SPICE) += qemu-chr-open-spice.o
>  stub-obj-y += qtest.o
> 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.

> diff --git a/target-arm/Makefile.objs b/target-arm/Makefile.objs
> index 9460b40..7b47434 100644
> --- a/target-arm/Makefile.objs
> +++ b/target-arm/Makefile.objs
> @@ -1,5 +1,6 @@
>  obj-y += arm-semi.o
>  obj-$(CONFIG_SOFTMMU) += machine.o
> +obj-$(CONFIG_PCI) += pci.o
>  obj-$(CONFIG_KVM) += kvm.o
>  obj-$(call land,$(CONFIG_KVM),$(call lnot,$(TARGET_AARCH64))) += kvm32.o
>  obj-$(call land,$(CONFIG_KVM),$(TARGET_AARCH64)) += kvm64.o
> diff --git a/target-arm/pci.c b/target-arm/pci.c
> new file mode 100644
> index 0000000..9bd5964
> --- /dev/null
> +++ b/target-arm/pci.c
> @@ -0,0 +1,16 @@
> +/*
> + * QEMU ARM 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 (pci_bus_num(dev->bus) << 8) | dev->devfn;
> +}
> -- 
> 2.4.4

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

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
>

Hello!

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

 Ok. So, will it be good if i place it in includes/hw/pci.h as static inline, similar to how it was done in v1?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

On Thu, Oct 15, 2015 at 12:42:06PM +0300, Pavel Fedin wrote:
>  Hello!
> 
> > 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.
> 
>  Ok. So, will it be good if i place it in includes/hw/pci.h as static inline, similar to how it was done in v1?
> 
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia

I think so, yes.