Re: [PATCH V7 02/11] vpci: add hooks for PCI device assign/de-assign

[Oleksandr <olekstysh@gmail.com>]

On 27.07.22 13:03, Jan Beulich wrote:


Hello Jan


> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
>> @@ -1558,6 +1560,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>>   {
>>       const struct domain_iommu *hd = dom_iommu(d);
>>       struct pci_dev *pdev;
>> +    uint8_t old_devfn;
> Why "old"? There's nothing "new" here. Perhaps "orig", considering ...


ok


>
>> @@ -1594,6 +1599,8 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>>                             pci_to_dev(pdev), flag)) )
>>           goto done;
>>   
>> +    old_devfn = devfn;
>> +
>>       for ( ; pdev->phantom_stride; rc = 0 )
>>       {
>>           devfn += pdev->phantom_stride;
> ... this updating of devfn is what you mean to deal with?

As I understand that was the intention of the author. At least I don't 
see other reasons.



> Then again
> I see no need for a separate variable in the first place. The input
> (seg,bus,devfn) tuple is translated to a pdev, so you can simply ...
>
>> @@ -1603,6 +1610,10 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>>                           pci_to_dev(pdev), flag);
>>       }
>>   
>> +    rc = vpci_assign_device(pdev);
>> +    if ( rc && deassign_device(d, seg, bus, old_devfn) )
> ... use pdev->devfn here.


Thanks, good point, will drop old_devfn and use pdev->devfn. I am 
wondering whether the printk after "done:" label (and other possible 
printk-s down the code) should really use pdev->devfn instead of devfn 
in PCI_SBDF construct?



>
>> +        domain_crash(d);
>> +
>>    done:
>>       if ( rc )
>>           printk(XENLOG_G_WARNING "%pd: assign (%pp) failed (%d)\n",
> This log message will want to appear _before_ the domain_crash()
> related output, or you will want to add another log message there.

I will probably add another log message before domain_crash() leaving 
this one as is.

printk(XENLOG_ERR "%pd: %pp was left partially assigned\n", d, 
&PCI_SBDF(seg, bus, devfn));


>
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -92,6 +92,37 @@ int vpci_add_handlers(struct pci_dev *pdev)
>>   
>>       return rc;
>>   }
>> +
>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>> +/* Notify vPCI that device is assigned to guest. */
>> +int vpci_assign_device(struct pci_dev *pdev)
>> +{
>> +    int rc;
>> +
>> +    ASSERT(pcidevs_write_locked());
>> +
>> +    if ( !has_vpci(pdev->domain) )
>> +        return 0;
>> +
>> +    rc = vpci_add_handlers(pdev);
>> +    if ( rc )
>> +        vpci_deassign_device(pdev);
>> +
>> +    return rc;
>> +}
>> +
>> +/* Notify vPCI that device is de-assigned from guest. */
>> +void vpci_deassign_device(struct pci_dev *pdev)
>> +{
>> +    ASSERT(pcidevs_write_locked());
>> +
>> +    if ( !has_vpci(pdev->domain) )
>> +        return;
> There's no need for this check since ...
>
>> +    vpci_remove_device(pdev);
> ... this function already has it. At which point the question is why
> a separate function needs to exist in the first place. To match with
> vpci_assign_device(), a simple #define to alias is would be enough.
> (This is one of the cases where personally I think a #define is
> superior to an inline wrapper.)


Agree, but ...


>
> Unless, of course, later patches extend this function. If so, the
> commit message giving this as justification for the introduction of
> (apparent) redundancy would be helpful.

... exactly, the later patches extend this function. I will update 
commit description.



>
> Jan

-- 
Regards,

Oleksandr Tyshchenko