[Qemu-devel] [PATCHv2 0/2] pci: allow PCI bus slots to be marked as reserved

[Qemu-devel] [PATCHv2 0/2] pci: allow PCI bus slots to be marked as reserved

[Mark Cave-Ayland]

For some machines it is impossible to plug devices into a particular PCI bus
slot, e.g. for a real Ultra 5 there are 2 PCI bridges attached to the root
bus behind which all devices must be plugged. Ignoring this rule will cause
problems with interrupt routing since the interrupt numbers are calculated
based upon PCI bridge id and secondary PCI bus slot id.

This patchset adds a new slot_reserved_mask property to PCIBus which is a
bitmask used to indicate whether PCI bus slots are reserved, i.e. they cannot
be used for hot or cold plugging on a particular PCI bus.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

v2:
- Rename dev_reserved_mask to slot_reserved_mask as suggested by Marcel
- Squash patches 2 and 3 together


Mark Cave-Ayland (2):
  pci: move check for existing devfn into new pci_bus_devfn_available()
    helper
  pci: add reserved slot check to do_pci_register_device()

 hw/pci/pci.c             |   34 ++++++++++++++++++++++++++++++----
 include/hw/pci/pci_bus.h |    1 +
 2 files changed, 31 insertions(+), 4 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCHv2 1/2] pci: move check for existing devfn into new pci_bus_devfn_available() helper

[Mark Cave-Ayland]

Also touch up the logic in do_pci_register_device() accordingly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/pci/pci.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 0c6f74a..04e6edb 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -951,6 +951,15 @@ uint16_t pci_requester_id(PCIDevice *dev)
     return pci_req_id_cache_extract(&dev->requester_id_cache);
 }
 
+static bool pci_bus_devfn_available(PCIBus *bus, int devfn)
+{
+    if (bus->devices[devfn]) {
+        return false;
+    } else {
+        return true;
+    }
+}
+
 /* -1 for devfn means auto assign */
 static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
                                          const char *name, int devfn,
@@ -974,14 +983,15 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
     if (devfn < 0) {
         for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
             devfn += PCI_FUNC_MAX) {
-            if (!bus->devices[devfn])
+            if (pci_bus_devfn_available(bus, devfn)) {
                 goto found;
+            }
         }
         error_setg(errp, "PCI: no slot/function available for %s, all in use",
                    name);
         return NULL;
     found: ;
-    } else if (bus->devices[devfn]) {
+    } else if (!pci_bus_devfn_available(bus, devfn)) {
         error_setg(errp, "PCI: slot %d function %d not available for %s,"
                    " in use by %s",
                    PCI_SLOT(devfn), PCI_FUNC(devfn), name,
-- 
1.7.10.4

[Qemu-devel] [PATCHv2 2/2] pci: add reserved slot check to do_pci_register_device()

[Mark Cave-Ayland]

Add a new slot_reserved_mask bitmask to PCIBus indicating whether or not each
PCI slot on the bus is reserved. Ensure that it is initialised to zero to
maintain the existing behaviour that all slots are available by default, and
add the additional check with appropriate error reporting to
do_pci_register_device().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/pci/pci.c             |   22 +++++++++++++++++++---
 include/hw/pci/pci_bus.h |    1 +
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 04e6edb..3ce25f7 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -371,6 +371,7 @@ static void pci_bus_init(PCIBus *bus, DeviceState *parent,
 {
     assert(PCI_FUNC(devfn_min) == 0);
     bus->devfn_min = devfn_min;
+    bus->slot_reserved_mask = 0x0;
     bus->address_space_mem = address_space_mem;
     bus->address_space_io = address_space_io;
 
@@ -960,6 +961,15 @@ static bool pci_bus_devfn_available(PCIBus *bus, int devfn)
     }
 }
 
+static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
+{
+    if (bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn))) {
+        return true;
+    } else {
+        return false;
+    }
+}
+
 /* -1 for devfn means auto assign */
 static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
                                          const char *name, int devfn,
@@ -983,14 +993,20 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
     if (devfn < 0) {
         for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
             devfn += PCI_FUNC_MAX) {
-            if (pci_bus_devfn_available(bus, devfn)) {
+            if (pci_bus_devfn_available(bus, devfn) &&
+                   !pci_bus_devfn_reserved(bus, devfn)) {
                 goto found;
             }
         }
-        error_setg(errp, "PCI: no slot/function available for %s, all in use",
-                   name);
+        error_setg(errp, "PCI: no slot/function available for %s, all in use "
+                   "or reserved", name);
         return NULL;
     found: ;
+    } else if (pci_bus_devfn_reserved(bus, devfn)) {
+        error_setg(errp, "PCI: slot %d function %d not available for %s,"
+                   " reserved",
+                   PCI_SLOT(devfn), PCI_FUNC(devfn), name);
+        return NULL;
     } else if (!pci_bus_devfn_available(bus, devfn)) {
         error_setg(errp, "PCI: slot %d function %d not available for %s,"
                    " in use by %s",
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index 5484a9b..bc34fd0 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -23,6 +23,7 @@ struct PCIBus {
     PCIIOMMUFunc iommu_fn;
     void *iommu_opaque;
     uint8_t devfn_min;
+    uint32_t slot_reserved_mask;
     pci_set_irq_fn set_irq;
     pci_map_irq_fn map_irq;
     pci_route_irq_fn route_intx_to_irq;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCHv2 0/2] pci: allow PCI bus slots to be marked as reserved

[Mark Cave-Ayland]

On 11/07/17 22:44, Mark Cave-Ayland wrote:

> For some machines it is impossible to plug devices into a particular PCI bus
> slot, e.g. for a real Ultra 5 there are 2 PCI bridges attached to the root
> bus behind which all devices must be plugged. Ignoring this rule will cause
> problems with interrupt routing since the interrupt numbers are calculated
> based upon PCI bridge id and secondary PCI bus slot id.
> 
> This patchset adds a new slot_reserved_mask property to PCIBus which is a
> bitmask used to indicate whether PCI bus slots are reserved, i.e. they cannot
> be used for hot or cold plugging on a particular PCI bus.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> v2:
> - Rename dev_reserved_mask to slot_reserved_mask as suggested by Marcel
> - Squash patches 2 and 3 together
> 
> 
> Mark Cave-Ayland (2):
>   pci: move check for existing devfn into new pci_bus_devfn_available()
>     helper
>   pci: add reserved slot check to do_pci_register_device()
> 
>  hw/pci/pci.c             |   34 ++++++++++++++++++++++++++++++----
>  include/hw/pci/pci_bus.h |    1 +
>  2 files changed, 31 insertions(+), 4 deletions(-)

Ping? Any further feedback on the v2 version? My latest set of sun4u
patches is dependent upon this patchset and it's freeze coming up next week!


ATB,

Mark.

Re: [Qemu-devel] [PATCHv2 0/2] pci: allow PCI bus slots to be marked as reserved

[Marcel Apfelbaum]

On 14/07/2017 12:59, Mark Cave-Ayland wrote:
> On 11/07/17 22:44, Mark Cave-Ayland wrote:
> 
>> For some machines it is impossible to plug devices into a particular PCI bus
>> slot, e.g. for a real Ultra 5 there are 2 PCI bridges attached to the root
>> bus behind which all devices must be plugged. Ignoring this rule will cause
>> problems with interrupt routing since the interrupt numbers are calculated
>> based upon PCI bridge id and secondary PCI bus slot id.
>>
>> This patchset adds a new slot_reserved_mask property to PCIBus which is a
>> bitmask used to indicate whether PCI bus slots are reserved, i.e. they cannot
>> be used for hot or cold plugging on a particular PCI bus.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>> v2:
>> - Rename dev_reserved_mask to slot_reserved_mask as suggested by Marcel
>> - Squash patches 2 and 3 together
>>
>>
>> Mark Cave-Ayland (2):
>>    pci: move check for existing devfn into new pci_bus_devfn_available()
>>      helper
>>    pci: add reserved slot check to do_pci_register_device()
>>
>>   hw/pci/pci.c             |   34 ++++++++++++++++++++++++++++++----
>>   include/hw/pci/pci_bus.h |    1 +
>>   2 files changed, 31 insertions(+), 4 deletions(-)
> 
> Ping? Any further feedback on the v2 version? My latest set of sun4u
> patches is dependent upon this patchset and it's freeze coming up next week!
> 

Hi,

As in prev version, other than the minor comment
on replacing "if (...) return true; else return false"
with the actual value, I am OK with it.

I believe Michael asked to see the series using this feature,
can you add a link to it, or post it with the dependency on this one?

Thanks,
Marcel

> 
> ATB,
> 
> Mark.
> 

Re: [Qemu-devel] [PATCHv2 0/2] pci: allow PCI bus slots to be marked as reserved

[Mark Cave-Ayland]

On 14/07/17 11:25, Marcel Apfelbaum wrote:

> On 14/07/2017 12:59, Mark Cave-Ayland wrote:
>> On 11/07/17 22:44, Mark Cave-Ayland wrote:
>>
>>> For some machines it is impossible to plug devices into a particular
>>> PCI bus
>>> slot, e.g. for a real Ultra 5 there are 2 PCI bridges attached to the
>>> root
>>> bus behind which all devices must be plugged. Ignoring this rule will
>>> cause
>>> problems with interrupt routing since the interrupt numbers are
>>> calculated
>>> based upon PCI bridge id and secondary PCI bus slot id.
>>>
>>> This patchset adds a new slot_reserved_mask property to PCIBus which
>>> is a
>>> bitmask used to indicate whether PCI bus slots are reserved, i.e.
>>> they cannot
>>> be used for hot or cold plugging on a particular PCI bus.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>
>>> v2:
>>> - Rename dev_reserved_mask to slot_reserved_mask as suggested by Marcel
>>> - Squash patches 2 and 3 together
>>>
>>>
>>> Mark Cave-Ayland (2):
>>>    pci: move check for existing devfn into new pci_bus_devfn_available()
>>>      helper
>>>    pci: add reserved slot check to do_pci_register_device()
>>>
>>>   hw/pci/pci.c             |   34 ++++++++++++++++++++++++++++++----
>>>   include/hw/pci/pci_bus.h |    1 +
>>>   2 files changed, 31 insertions(+), 4 deletions(-)
>>
>> Ping? Any further feedback on the v2 version? My latest set of sun4u
>> patches is dependent upon this patchset and it's freeze coming up next
>> week!
>>
> 
> Hi,
> 
> As in prev version, other than the minor comment
> on replacing "if (...) return true; else return false"
> with the actual value, I am OK with it.

Okay great! So change pci_bus_devfn_available() to something like this?

static bool pci_bus_devfn_available(PCIBus *bus, int devfn)
{
    return !(bus->devices[devfn]);
}

> I believe Michael asked to see the series using this feature,
> can you add a link to it, or post it with the dependency on this one?

Sure, I posted the link earlier in the week:
https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg03045.html.
Or more specifically:

https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg03041.html


ATB,

Mark.

Re: [Qemu-devel] [PATCHv2 0/2] pci: allow PCI bus slots to be marked as reserved

[Marcel Apfelbaum]

On 14/07/2017 13:37, Mark Cave-Ayland wrote:
> On 14/07/17 11:25, Marcel Apfelbaum wrote:
> 
>> On 14/07/2017 12:59, Mark Cave-Ayland wrote:
>>> On 11/07/17 22:44, Mark Cave-Ayland wrote:
>>>
>>>> For some machines it is impossible to plug devices into a particular
>>>> PCI bus
>>>> slot, e.g. for a real Ultra 5 there are 2 PCI bridges attached to the
>>>> root
>>>> bus behind which all devices must be plugged. Ignoring this rule will
>>>> cause
>>>> problems with interrupt routing since the interrupt numbers are
>>>> calculated
>>>> based upon PCI bridge id and secondary PCI bus slot id.
>>>>
>>>> This patchset adds a new slot_reserved_mask property to PCIBus which
>>>> is a
>>>> bitmask used to indicate whether PCI bus slots are reserved, i.e.
>>>> they cannot
>>>> be used for hot or cold plugging on a particular PCI bus.
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>
>>>> v2:
>>>> - Rename dev_reserved_mask to slot_reserved_mask as suggested by Marcel
>>>> - Squash patches 2 and 3 together
>>>>
>>>>
>>>> Mark Cave-Ayland (2):
>>>>     pci: move check for existing devfn into new pci_bus_devfn_available()
>>>>       helper
>>>>     pci: add reserved slot check to do_pci_register_device()
>>>>
>>>>    hw/pci/pci.c             |   34 ++++++++++++++++++++++++++++++----
>>>>    include/hw/pci/pci_bus.h |    1 +
>>>>    2 files changed, 31 insertions(+), 4 deletions(-)
>>>
>>> Ping? Any further feedback on the v2 version? My latest set of sun4u
>>> patches is dependent upon this patchset and it's freeze coming up next
>>> week!
>>>
>>

Hi Mark,

>> Hi,
>>
>> As in prev version, other than the minor comment
>> on replacing "if (...) return true; else return false"
>> with the actual value, I am OK with it.
> 
> Okay great! So change pci_bus_devfn_available() to something like this?
> 
> static bool pci_bus_devfn_available(PCIBus *bus, int devfn)
> {
>      return !(bus->devices[devfn]);
> }
> 

Yes, thanks.

>> I believe Michael asked to see the series using this feature,
>> can you add a link to it, or post it with the dependency on this one?
> 
> Sure, I posted the link earlier in the week:
> https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg03045.html.
> Or more specifically:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg03041.html
> 

Hi Michael,
Can you please have a look to the above patch using
the 'reserved slots' patch?

Thanks,
Marcel

> 
> ATB,
> 
> Mark.
> 

Re: [Qemu-devel] [PATCHv2 0/2] pci: allow PCI bus slots to be marked as reserved

[Mark Cave-Ayland]

On 16/07/17 08:28, Marcel Apfelbaum wrote:

>>> As in prev version, other than the minor comment
>>> on replacing "if (...) return true; else return false"
>>> with the actual value, I am OK with it.
>>
>> Okay great! So change pci_bus_devfn_available() to something like this?
>>
>> static bool pci_bus_devfn_available(PCIBus *bus, int devfn)
>> {
>>      return !(bus->devices[devfn]);
>> }
>>
> 
> Yes, thanks.

Great, I'll send a v3 with the above modification shortly.

>>> I believe Michael asked to see the series using this feature,
>>> can you add a link to it, or post it with the dependency on this one?
>>
>> Sure, I posted the link earlier in the week:
>> https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg03045.html.
>> Or more specifically:
>>
>> https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg03041.html
>>
> 
> Hi Michael,
> Can you please have a look to the above patch using
> the 'reserved slots' patch?

I should add that if this patch isn't suitable for freeze then it's
likely I'll drop the sun4u patchset for 2.10 since otherwise using
-device places devices on the PCI root bus.

For existing users this means that the machine will start up with their
existing command lines but any extra devices added on the command line
via -device, e.g. virtio will be silently broken because the interrupt
routing is incorrect.

At least with this patch the user will get an error, and thus it's easy
to update the wiki to explain that moving forward it is necessary to add
"bus=pciB" for extra devices to function correctly.


ATB,

Mark.