[3/3] pci: add reserved slot check to do_pci_register_device()

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

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

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.

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