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

[Qemu-devel] [PATCH 0/3] 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 dev_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>


Mark Cave-Ayland (3):
  pci: move check for existing devfn into new pci_bus_devfn_available()
    helper
  pci: add dev_reserved_mask property to PCIBus
  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] [PATCH 1/3] 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

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

[Marcel Apfelbaum]

On 07/07/2017 10:44, 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 |   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);
>   }
> 


Hi Mark,

> +static bool pci_bus_devfn_available(PCIBus *bus, int devfn)
> +{
> +    if (bus->devices[devfn]) {
> +        return false;
> +    } else {
> +        return true;
> +    }
> +}
> +
The function may simply return bus->devices[devfn], right?
(the return type should take care of the rest)



>   /* -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)) {

I am all for making the code more readable, but in this
case I am not sure if it worth it. "bus->devices[devfn]"
is self explanatory, but maybe is a matter of taste.

Thanks,
Marcel

>                   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] [PATCH 1/3] pci: move check for existing devfn into new pci_bus_devfn_available() helper

[Mark Cave-Ayland]

On 10/07/17 08:24, Marcel Apfelbaum wrote:

> On 07/07/2017 10:44, 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 |   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);
>>   }
>>
> 
> 
> Hi Mark,
> 
>> +static bool pci_bus_devfn_available(PCIBus *bus, int devfn)
>> +{
>> +    if (bus->devices[devfn]) {
>> +        return false;
>> +    } else {
>> +        return true;
>> +    }
>> +}
>> +
> The function may simply return bus->devices[devfn], right?
> (the return type should take care of the rest)
> 
> 
> 
>>   /* -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)) {
> 
> I am all for making the code more readable, but in this
> case I am not sure if it worth it. "bus->devices[devfn]"
> is self explanatory, but maybe is a matter of taste.

Yes I agree that on its own it comes across as a more cosmetic patch,
however I felt that it made the final logic in patch 3 much more
readable in terms of determining the behaviour for reserved vs.
unavailable slots.

Does anyone else have any strong opinions?


ATB,

Mark.

[Qemu-devel] [PATCH 2/3] pci: add dev_reserved_mask property to PCIBus

[Mark Cave-Ayland]

This is just a simple bitmask indicating whether or not each PCI slot on the
bus is reserved. Ensure that it is initialised to zero so that all bus slots
are available by default.

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

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 04e6edb..239161e 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->dev_reserved_mask = 0x0;
     bus->address_space_mem = address_space_mem;
     bus->address_space_io = address_space_io;
 
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index 5484a9b..a0cf655 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 dev_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] [PATCH 2/3] pci: add dev_reserved_mask property to PCIBus

[Marcel Apfelbaum]

On 07/07/2017 10:44, Mark Cave-Ayland wrote:
> This is just a simple bitmask indicating whether or not each PCI slot on the
> bus is reserved. Ensure that it is initialised to zero so that all bus slots
> are available by default.
> 

Hi Mark,

> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/pci/pci.c             |    1 +
>   include/hw/pci/pci_bus.h |    1 +
>   2 files changed, 2 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 04e6edb..239161e 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->dev_reserved_mask = 0x0;

Not really necessary, the object are zeroed when created
(as far as I remember).

>       bus->address_space_mem = address_space_mem;
>       bus->address_space_io = address_space_io;
>   
> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> index 5484a9b..a0cf655 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 dev_reserved_mask;

I would merge this patch with the next one to
see the new field "in action".

Thanks,
Marcel

>       pci_set_irq_fn set_irq;
>       pci_map_irq_fn map_irq;
>       pci_route_irq_fn route_intx_to_irq;
> 

Re: [Qemu-devel] [PATCH 2/3] pci: add dev_reserved_mask property to PCIBus

[Mark Cave-Ayland]

On 10/07/17 08:27, Marcel Apfelbaum wrote:

> On 07/07/2017 10:44, Mark Cave-Ayland wrote:
>> This is just a simple bitmask indicating whether or not each PCI slot
>> on the
>> bus is reserved. Ensure that it is initialised to zero so that all bus
>> slots
>> are available by default.
>>
> 
> Hi Mark,
> 
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/pci/pci.c             |    1 +
>>   include/hw/pci/pci_bus.h |    1 +
>>   2 files changed, 2 insertions(+)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 04e6edb..239161e 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->dev_reserved_mask = 0x0;
> 
> Not really necessary, the object are zeroed when created
> (as far as I remember).

Yes I believe that's true. The main reason I added it was that it makes
it easy to find the default value with grep rather than having to work
your way through the initialisation sequence to verify that in fact it
belongs to a struct that is zeroed before use. So again, not 100%
necessary but I felt it made life a bit easier.

>>       bus->address_space_mem = address_space_mem;
>>       bus->address_space_io = address_space_io;
>>   diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
>> index 5484a9b..a0cf655 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 dev_reserved_mask;
> 
> I would merge this patch with the next one to
> see the new field "in action".

If everyone wants me to drop the above setting of dev_reserved_mask than
I can squash it into the following patch.


ATB,

Mark.

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

[Mark Cave-Ayland]

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

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 239161e..9dece2a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -961,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->dev_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,
@@ -984,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",
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 3/3] pci: add reserved slot check to do_pci_register_device()

[Marcel Apfelbaum]

On 07/07/2017 10:44, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/pci/pci.c |   21 ++++++++++++++++++---
>   1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 239161e..9dece2a 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -961,6 +961,15 @@ static bool pci_bus_devfn_available(PCIBus *bus, int devfn)
>       }
>   }
>  

Hi Mark,


> +static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
> +{
> +    if (bus->dev_reserved_mask & (1UL << PCI_SLOT(devfn))) {

Looking at this code maybe we should rename the field to
"slot_mask"? Only a suggestion.

> +        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,
> @@ -984,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);

I wouldn't combine slot available/reserved, maybe check for reserved
before "available"?

>           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",
> 

Thanks for the patches!
Marcel

Re: [Qemu-devel] [PATCH 3/3] pci: add reserved slot check to do_pci_register_device()

[Mark Cave-Ayland]

On 10/07/17 08:35, Marcel Apfelbaum wrote:

> On 07/07/2017 10:44, Mark Cave-Ayland wrote:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/pci/pci.c |   21 ++++++++++++++++++---
>>   1 file changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 239161e..9dece2a 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -961,6 +961,15 @@ static bool pci_bus_devfn_available(PCIBus *bus,
>> int devfn)
>>       }
>>   }
>>  
> 
> Hi Mark,
> 
> 
>> +static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
>> +{
>> +    if (bus->dev_reserved_mask & (1UL << PCI_SLOT(devfn))) {
> 
> Looking at this code maybe we should rename the field to
> "slot_mask"? Only a suggestion.

Interestingly enough, I did start with that initially but then
discovered that "slot_mask" is ambiguous - does the bit in the mask tell
you whether the slot is free, or whether the slot is available?

The use of the "dev" prefix was to match the use of the devfn parameter,
however if you're happy with just a "slot" prefix then would you be okay
with "slot_reserved_mask"?

>> +        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,
>> @@ -984,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);
> 
> I wouldn't combine slot available/reserved, maybe check for reserved
> before "available"?

I did think about doing separate reserved/available checks, but it
doesn't seem to make sense in the code above.

By passing devfn == -1 the caller is asking for the next free slot, if
there is one available. Whether the slot is reserved or occupied makes
no difference to the fact that the slot is itself unavailable. All the
change does here is alter the error message to provide hint that while
no slot was available, it could be because all slots were in use or
reserved.

When you mention to check for reserved before available, do you mean to
swap the order of the arguments in the if() statement?

> 
>>           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",
>>
> 
> Thanks for the patches!
> Marcel


ATB,

Mark.

Re: [Qemu-devel] [PATCH 3/3] pci: add reserved slot check to do_pci_register_device()

[Marcel Apfelbaum]

On 10/07/2017 16:05, Mark Cave-Aland wrote:
> On 10/07/17 08:35, Marcel Apfelbaum wrote:
> 
>> On 07/07/2017 10:44, Mark Cave-Ayland wrote:
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>    hw/pci/pci.c |   21 ++++++++++++++++++---
>>>    1 file changed, 18 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>> index 239161e..9dece2a 100644
>>> --- a/hw/pci/pci.c
>>> +++ b/hw/pci/pci.c
>>> @@ -961,6 +961,15 @@ static bool pci_bus_devfn_available(PCIBus *bus,
>>> int devfn)
>>>        }
>>>    }
>>>   
>>
>> Hi Mark,
>>
>>
>>> +static bool pci_bus_devfn_reserved(PCIBus *bus, int devfn)
>>> +{
>>> +    if (bus->dev_reserved_mask & (1UL << PCI_SLOT(devfn))) {
>>
>> Looking at this code maybe we should rename the field to
>> "slot_mask"? Only a suggestion.
> 
> Interestingly enough, I did start with that initially but then
> discovered that "slot_mask" is ambiguous - does the bit in the mask tell
> you whether the slot is free, or whether the slot is available?
> 
> The use of the "dev" prefix was to match the use of the devfn parameter,
> however if you're happy with just a "slot" prefix then would you be okay
> with "slot_reserved_mask"?

Sounds good, thanks.

> 
>>> +        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,
>>> @@ -984,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);
>>
>> I wouldn't combine slot available/reserved, maybe check for reserved
>> before "available"?
> 
> I did think about doing separate reserved/available checks, but it
> doesn't seem to make sense in the code above.
> 
> By passing devfn == -1 the caller is asking for the next free slot, if
> there is one available. Whether the slot is reserved or occupied makes
> no difference to the fact that the slot is itself unavailable. All the
> change does here is alter the error message to provide hint that while
> no slot was available, it could be because all slots were in use or
> reserved.

Agreed, sorry for the noise.

Thanks,
Marcel

> When you mention to check for reserved before available, do you mean to
> swap the order of the arguments in the if() statement?
> >>
>>>            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",
>>>
>>
>> Thanks for the patches!
>> Marcel
> 
> 
> ATB,
> 
> Mark.
> 

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

[Michael S. Tsirkin]

On Fri, Jul 07, 2017 at 08:43:59AM +0100, 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 dev_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>

Could you please point me at series making use of this
functionality?
Thanks!

> 
> Mark Cave-Ayland (3):
>   pci: move check for existing devfn into new pci_bus_devfn_available()
>     helper
>   pci: add dev_reserved_mask property to PCIBus
>   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

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

[Marcel Apfelbaum]

On 11/07/2017 18:31, Michael S. Tsirkin wrote:
> On Fri, Jul 07, 2017 at 08:43:59AM +0100, 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 dev_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>
> 

Hi Michael,

> Could you please point me at series making use of this
> functionality?

There is a discussion upstream, I am not aware of patches yet.

https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg00771.html

Thanks,
Marcel


> Thanks!
> 
>>
>> Mark Cave-Ayland (3):
>>    pci: move check for existing devfn into new pci_bus_devfn_available()
>>      helper
>>    pci: add dev_reserved_mask property to PCIBus
>>    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

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

[Michael S. Tsirkin]

On Tue, Jul 11, 2017 at 06:51:50PM +0300, Marcel Apfelbaum wrote:
> On 11/07/2017 18:31, Michael S. Tsirkin wrote:
> > On Fri, Jul 07, 2017 at 08:43:59AM +0100, 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 dev_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>
> > 
> 
> Hi Michael,
> 
> > Could you please point me at series making use of this
> > functionality?
> 
> There is a discussion upstream, I am not aware of patches yet.
> 
> https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg00771.html
> 
> Thanks,
> Marcel

It is probably a good idea to defer merging this patchset
until there's an agreement on how it will be used.


> 
> > Thanks!
> > 
> > > 
> > > Mark Cave-Ayland (3):
> > >    pci: move check for existing devfn into new pci_bus_devfn_available()
> > >      helper
> > >    pci: add dev_reserved_mask property to PCIBus
> > >    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

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

[Mark Cave-Ayland]

On 11/07/17 16:57, Michael S. Tsirkin wrote:

> On Tue, Jul 11, 2017 at 06:51:50PM +0300, Marcel Apfelbaum wrote:
>> On 11/07/2017 18:31, Michael S. Tsirkin wrote:
>>> On Fri, Jul 07, 2017 at 08:43:59AM +0100, 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 dev_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>
>>>
>>
>> Hi Michael,
>>
>>> Could you please point me at series making use of this
>>> functionality?
>>
>> There is a discussion upstream, I am not aware of patches yet.
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg00771.html
>>
>> Thanks,
>> Marcel
> 
> It is probably a good idea to defer merging this patchset
> until there's an agreement on how it will be used.

I have a local patchset that uses it, but it has been held up by the
on-going issues with the fw_cfg patchset upon which it is also dependent.

I will try and find some time this evening to rebase the patchset and
send it out as I am aware that freeze is coming up soon.


ATB,

Mark.

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

[Mark Cave-Ayland]

On 11/07/17 18:59, Mark Cave-Ayland wrote:

> On 11/07/17 16:57, Michael S. Tsirkin wrote:
> 
>> On Tue, Jul 11, 2017 at 06:51:50PM +0300, Marcel Apfelbaum wrote:
>>> On 11/07/2017 18:31, Michael S. Tsirkin wrote:
>>>> On Fri, Jul 07, 2017 at 08:43:59AM +0100, 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 dev_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>
>>>>
>>>
>>> Hi Michael,
>>>
>>>> Could you please point me at series making use of this
>>>> functionality?
>>>
>>> There is a discussion upstream, I am not aware of patches yet.
>>>
>>> https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg00771.html
>>>
>>> Thanks,
>>> Marcel
>>
>> It is probably a good idea to defer merging this patchset
>> until there's an agreement on how it will be used.
> 
> I have a local patchset that uses it, but it has been held up by the
> on-going issues with the fw_cfg patchset upon which it is also dependent.
> 
> I will try and find some time this evening to rebase the patchset and
> send it out as I am aware that freeze is coming up soon.

You can see the use of this feature in patch 8 of my sun4u patchset
here:
https://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg03041.html.


ATB,

Mark.