Re: [PATCH 4/4] vpci: move lock outside of struct vpci

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Jan!

On 02.02.22 11:22, Jan Beulich wrote:
> On 01.02.2022 17:25, Oleksandr Andrushchenko wrote:
>> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
>>
>> This way the lock can be used to check whether vpci is present, and
>> removal can be performed while holding the lock, in order to make
>> sure there are no accesses to the contents of the vpci struct.
>> Previously removal could race with vpci_read for example, since the
>> lock was dropped prior to freeing pdev->vpci.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>> ---
>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>> Cc: Julien Grall <julien@xxxxxxx>
>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> ---
>> New in v5 of this series: this is an updated version of the patch published 
>> at
>> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/20180717094830.54806-2-roger.pau@xxxxxxxxxx/__;!!GF_29dbcQIUBPA!jmmcewY6y9Ur4rgvOgqscz8gBWaod2JnQOkHvWtYKgnqeU6BoWJTqCN3UDpCw3io8Ynk-wBXhA$
>>  [lore[.]kernel[.]org]
>>
>> Changes since v5:
> This is a little odd in a series implicitly tagged as v1.
>
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -142,12 +142,13 @@ bool vpci_process_pending(struct vcpu *v)
>>           if ( rc == -ERESTART )
>>               return true;
>>   
>> -        spin_lock(&v->vpci.pdev->vpci->lock);
>> -        /* Disable memory decoding unconditionally on failure. */
>> -        modify_decoding(v->vpci.pdev,
>> -                        rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : 
>> v->vpci.cmd,
>> -                        !rc && v->vpci.rom_only);
>> -        spin_unlock(&v->vpci.pdev->vpci->lock);
>> +        spin_lock(&v->vpci.pdev->vpci_lock);
>> +        if ( v->vpci.pdev->vpci )
>> +            /* Disable memory decoding unconditionally on failure. */
>> +            modify_decoding(v->vpci.pdev,
>> +                            rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : 
>> v->vpci.cmd,
>> +                            !rc && v->vpci.rom_only);
>> +        spin_unlock(&v->vpci.pdev->vpci_lock);
> While I certainly see the point, the addition of this if() (and a
> few more elsewhere) isn't covered by title or description.
The commit message says:
"This way the lock can be used to check whether vpci is present, and
removal can be performed while holding the lock, in order to make
sure there are no accesses to the contents of the vpci struct."
So, I think this is enough to describe the fact that after you have locked
the protected structure may have gone already and we need to
re-check it is still present.
>
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -35,12 +35,10 @@ extern vpci_register_init_t *const __start_vpci_array[];
>>   extern vpci_register_init_t *const __end_vpci_array[];
>>   #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
>>   
>> -void vpci_remove_device(struct pci_dev *pdev)
>> +static void vpci_remove_device_locked(struct pci_dev *pdev)
>>   {
>> -    if ( !has_vpci(pdev->domain) )
>> -        return;
>> +    ASSERT(spin_is_locked(&pdev->vpci_lock));
> While, unlike here, ...
>
>> @@ -152,8 +164,6 @@ int vpci_add_register(struct vpci *vpci, vpci_read_t 
>> *read_handler,
>>       r->offset = offset;
>>       r->private = data;
>>   
>> -    spin_lock(&vpci->lock);
> ... you did explain why you don't want to add a similar assertion
> here, I think in return the function wants to have a comment added
> that it's required to be called with the respective lock held.
Will add the comments
>   I
> notice you did so for the declaration, but I think such a comment
> would better be present at the definition as well. Same for
> vpci_remove_register() then, obviously.
Ok
>
>> @@ -311,7 +316,7 @@ static uint32_t merge_result(uint32_t data, uint32_t 
>> new, unsigned int size,
>>   uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
>>   {
>>       const struct domain *d = current->domain;
>> -    const struct pci_dev *pdev;
>> +    struct pci_dev *pdev;
>>       const struct vpci_register *r;
>>       unsigned int data_offset = 0;
>>       uint32_t data = ~(uint32_t)0;
>> @@ -327,7 +332,12 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, 
>> unsigned int size)
>>       if ( !pdev )
>>           return vpci_read_hw(sbdf, reg, size);
>>   
>> -    spin_lock(&pdev->vpci->lock);
>> +    spin_lock(&pdev->vpci_lock);
>> +    if ( !pdev->vpci )
>> +    {
>> +        spin_unlock(&pdev->vpci_lock);
>> +        return vpci_read_hw(sbdf, reg, size);
>> +    }
> In this case as well as in its write counterpart it becomes even more
> important to justify (in the description) the new behavior. It is not
> obvious at all that the absence of a struct vpci should be taken as
> an indication that the underlying device needs accessing instead.
> This also cannot be inferred from the "!pdev" case visible in context.
> In that case we have no record of a device at this SBDF, and hence the
> fallback pretty clearly is a "just in case" one. Yet if we know of a
> device, the absence of a struct vpci may mean various possible things.
Ok, I'll describe this change
>
> Jan
>
Thank you,
Oleksandr