On Mon, Jun 21, 2010 at 03:03:58PM +0900, Isaku Yamahata wrote:
> Set PCI multi-function bit according to multifunction property.
> PCI address, devfn ,is exported to users as addr property,
> so users can populate pci function(PCIDevice in qemu)
> at arbitrary devfn.
> It means each function(PCIDevice) don't know whether pci device
> (PCIDevice[8]) is multi function or not.
> So this patch allows user to set multifunction bit via property
> and checks whether multifunction bit is set correctly.
>
> Signed-off-by: Isaku Yamahata <yamah...@valinux.co.jp>
Applying it this way will break bisect.
We also need to handle migration compatibility.
I propose we split it this way:
- patch to add multifunction property (ignored)
- set property in builtin devices where appropriate
- patch to look at property and set bit in header
> ---
> changes v3 -> v4:
> - introduce multifunction property.
>
> changes v2 -> v3:
> - introduce PCI_FUNC_MAX
> - more commit log
>
> changes v1 -> v2:
> ---
> hw/pci.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> hw/pci.h | 4 ++++
> 2 files changed, 61 insertions(+), 3 deletions(-)
>
> diff --git a/hw/pci.c b/hw/pci.c
> index b6c0a10..abc3c1d 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -67,6 +67,7 @@ static struct BusInfo pci_bus_info = {
> DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1),
> DEFINE_PROP_STRING("romfile", PCIDevice, romfile),
> DEFINE_PROP_UINT32("rombar", PCIDevice, rom_bar, 1),
> + DEFINE_PROP_UINT8("multifunction", PCIDevice, mf, 0),
Please make this a bit property, not UINT8. It can be stored in
cap_present.
> DEFINE_PROP_END_OF_LIST()
> }
> };
> @@ -575,6 +576,44 @@ static void pci_init_wmask_bridge(PCIDevice *d)
> pci_set_word(d->wmask + PCI_BRIDGE_CONTROL, 0xffff);
> }
>
> +static int pci_init_multifunction(PCIBus *bus, PCIDevice *dev)
> +{
IMO we should just add in pci_register_device:
if (d->cap_resent & QEMU_PCI_CAP_MULTIFUNCTION) {
dev->config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
} else if (PCI_FUNC(dev->devfn)) {
error_report("PCI: single function device can't be populated
%x.%x",
PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
return -1;
}
And be done with it.
> + uint8_t slot = PCI_SLOT(dev->devfn);
> + uint8_t func = PCI_FUNC(dev->devfn);
> +
> + /* we are here before bus->devices[dev->devfn] = dev */
> + assert(!bus->devices[dev->devfn]);
Can users trigger this?
If yes, this needs and error, not an assert.
> +
> + if (dev->mf) {
> + dev->config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
> + }
> +
> + if (func) {
Please open-code func above.
> + PCIDevice *d = bus->devices[PCI_DEVFN(slot, 0)];
> + if (d && !d->mf) {
> + /* function 0 should set multifunction bit */
> + error_report("PCI: single function device can't be populated "
> + "in function %x.%x", slot, func);
> + return -1;
> + }
> + return 0;
> + }
> +
> + if (dev->mf) {
> + return 0;
> + }
> + /* function 0 indicates single function, so function > 0 must be NULL */
We don't need the below test: each function will be checked
when it is added.
> + for (func = 1; func < PCI_FUNC_MAX; ++func) {
> + if (bus->devices[PCI_DEVFN(slot, func)]) {
> + error_report("PCI: %x.0 indicates single function, "
> + "but %x.%x is already populated.",
> + slot, slot, func);
> + return -1;
> + }
> + }
> + return 0;
> +}
> +
> static void pci_config_alloc(PCIDevice *pci_dev)
> {
> int config_size = pci_config_size(pci_dev);
> @@ -629,6 +668,9 @@ static PCIDevice *do_pci_register_device(PCIDevice
> *pci_dev, PCIBus *bus,
> if (is_bridge) {
> pci_init_wmask_bridge(pci_dev);
> }
> + if (pci_init_multifunction(bus, pci_dev)) {
> + return NULL;
> + }
>
> if (!config_read)
> config_read = pci_default_read_config;
> @@ -1652,22 +1694,34 @@ void pci_qdev_register_many(PCIDeviceInfo *info)
> }
> }
>
> -PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name)
> +PCIDevice *pci_create_mf(PCIBus *bus, int devfn, uint8_t mf, const char
> *name)
> {
> DeviceState *dev;
>
> dev = qdev_create(&bus->qbus, name);
> qdev_prop_set_uint32(dev, "addr", devfn);
> + qdev_prop_set_uint8(dev, "multifunction", mf);
> return DO_UPCAST(PCIDevice, qdev, dev);
> }
>
> -PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
> +PCIDevice *pci_create_simple_mf(PCIBus *bus, int devfn, uint8_t mf,
> + const char *name)
> {
> - PCIDevice *dev = pci_create(bus, devfn, name);
> + PCIDevice *dev = pci_create_mf(bus, devfn, mf, name);
> qdev_init_nofail(&dev->qdev);
> return dev;
> }
>
> +PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name)
> +{
> + return pci_create_mf(bus, devfn, 0, name);
> +}
> +
> +PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
> +{
> + return pci_create_simple_mf(bus, devfn, 0, name);
> +}
> +
> static int pci_find_space(PCIDevice *pdev, uint8_t size)
> {
> int config_size = pci_config_size(pdev);
> diff --git a/hw/pci.h b/hw/pci.h
> index 76adc66..685fd44 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -131,6 +131,7 @@ struct PCIDevice {
> /* the following fields are read only */
> PCIBus *bus;
> uint32_t devfn;
> + uint8_t mf; /* multi function capabile device */
Add a bit in cap_present please.
> char name[64];
> PCIIORegion io_regions[PCI_NUM_REGIONS];
>
> @@ -343,6 +344,9 @@ typedef struct {
> void pci_qdev_register(PCIDeviceInfo *info);
> void pci_qdev_register_many(PCIDeviceInfo *info);
>
> +PCIDevice *pci_create_mf(PCIBus *bus, int devfn, uint8_t mf, const char
> *name);
> +PCIDevice *pci_create_simple_mf(PCIBus *bus, int devfn, uint8_t mf,
> + const char *name);
mf->multifunction
But do we need the extra functions? I thought qdev can handle
the flag?
> PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name);
> PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
>
> --
> 1.6.6.1
