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

[Qemu-devel] [PATCHv3 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>

v3:
- Rebase onto master
- Simplify pci_bus_devfn_available() as suggested by Marcel
- Also simplify pci_bus_devfn_reserved() in a similar manner

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             |   26 ++++++++++++++++++++++----
 include/hw/pci/pci_bus.h |    1 +
 2 files changed, 23 insertions(+), 4 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCHv3 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 |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 0c6f74a..efc9c86 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -951,6 +951,11 @@ 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)
+{
+    return !(bus->devices[devfn]);
+}
+
 /* -1 for devfn means auto assign */
 static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
                                          const char *name, int devfn,
@@ -974,14 +979,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

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

[Marcel Apfelbaum]

On 16/07/2017 23:27, Mark Cave-Ayland wrote:
> 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 |   10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 0c6f74a..efc9c86 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -951,6 +951,11 @@ 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)
> +{
> +    return !(bus->devices[devfn]);
> +}
> +
>   /* -1 for devfn means auto assign */
>   static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>                                            const char *name, int devfn,
> @@ -974,14 +979,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,
> 

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

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

[Yi Min Zhao]

在 2017/7/17 上午4:27, 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 |   10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 0c6f74a..efc9c86 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -951,6 +951,11 @@ 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)
> +{
> +    return !(bus->devices[devfn]);
Hi,

I want to ask a question. According to the next patch, you check 
bus->devices[devfn]
and reserved bit separately. Why not move the reserved bit check here?
I think bus->devices[devfn] != NULL or revsered bit set means slot is 
unavailable.

Regards,

Yi Min
> +}
> +
>   /* -1 for devfn means auto assign */
>   static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>                                            const char *name, int devfn,
> @@ -974,14 +979,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,

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

[Mark Cave-Ayland]

On 04/09/17 11:01, Yi Min Zhao wrote:

> I want to ask a question. According to the next patch, you check
> bus->devices[devfn]
> and reserved bit separately. Why not move the reserved bit check here?
> I think bus->devices[devfn] != NULL or revsered bit set means slot is
> unavailable.

Primarily it was to allow callers to distinguish between the two
different cases i.e. the error message can tell you that the reason the
slot was unavailable was because it was reserved, i.e. you can *never*
plug anything into it compared with unavailable where someone has
plugged something into the slot with -device but it could be later
removed, for example with hotplug.


ATB,

Mark.

[Qemu-devel] [PATCHv3 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             |   18 +++++++++++++++---
 include/hw/pci/pci_bus.h |    1 +
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index efc9c86..41591dc 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;
 
@@ -956,6 +957,11 @@ static bool pci_bus_devfn_available(PCIBus *bus, int devfn)
     return !(bus->devices[devfn]);
 }
 
+static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
+{
+    return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
+}
+
 /* -1 for devfn means auto assign */
 static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
                                          const char *name, int devfn,
@@ -979,14 +985,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] [PATCHv3 2/2] pci: add reserved slot check to do_pci_register_device()

[Marcel Apfelbaum]

On 16/07/2017 23:27, Mark Cave-Ayland wrote:
> 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             |   18 +++++++++++++++---
>   include/hw/pci/pci_bus.h |    1 +
>   2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index efc9c86..41591dc 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;
>   
> @@ -956,6 +957,11 @@ static bool pci_bus_devfn_available(PCIBus *bus, int devfn)
>       return !(bus->devices[devfn]);
>   }
>   
> +static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
> +{
> +    return bus->slot_reserved_mask & (1UL << PCI_SLOT(devfn));
> +}
> +
>   /* -1 for devfn means auto assign */
>   static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>                                            const char *name, int devfn,
> @@ -979,14 +985,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;
> 

Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Thanks,
Marcel

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

[Mark Cave-Ayland]

On 16/07/17 21:27, 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>
> 
> v3:
> - Rebase onto master
> - Simplify pci_bus_devfn_available() as suggested by Marcel
> - Also simplify pci_bus_devfn_reserved() in a similar manner
> 
> 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             |   26 ++++++++++++++++++++++----
>  include/hw/pci/pci_bus.h |    1 +
>  2 files changed, 23 insertions(+), 4 deletions(-)

Ping? Is there any chance to get this in soon, as I ended up having to
drop a couple of sun4u patch series for 2.10 that were dependent upon
this :(

I have R-B tags from Marcel, however my understanding is that this patch
still needs review from Michael?

Here is the link to the patch that depends upon this:
https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg03041.html


Many thanks,

Mark.

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

[Marcel Apfelbaum]

On 31/08/2017 9:54, Mark Cave-Ayland wrote:
> On 16/07/17 21:27, 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>
>>
>> v3:
>> - Rebase onto master
>> - Simplify pci_bus_devfn_available() as suggested by Marcel
>> - Also simplify pci_bus_devfn_reserved() in a similar manner
>>
>> 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             |   26 ++++++++++++++++++++++----
>>   include/hw/pci/pci_bus.h |    1 +
>>   2 files changed, 23 insertions(+), 4 deletions(-)
> 
> Ping? Is there any chance to get this in soon, as I ended up having to
> drop a couple of sun4u patch series for 2.10 that were dependent upon
> this :(
> 
> I have R-B tags from Marcel, however my understanding is that this patch
> still needs review from Michael?
> 

Hi Mark,
I think Michael did not start yet to pick patches for 2.11 .

Thanks,
Marcel

> Here is the link to the patch that depends upon this:
> https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg03041.html
> 
> 
> Many thanks,
> 
> Mark.
>