Re: [PATCH v6 03/13] vpci: move lock outside of struct vpci

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 08.02.22 10:53, Jan Beulich wrote:
> On 07.02.2022 17:44, Oleksandr Andrushchenko wrote:
>>
>> On 07.02.22 18:37, Jan Beulich wrote:
>>> On 07.02.2022 17:21, Oleksandr Andrushchenko wrote:
>>>> On 07.02.22 18:15, Jan Beulich wrote:
>>>>> On 07.02.2022 17:07, Oleksandr Andrushchenko wrote:
>>>>>> On 07.02.22 17:26, Jan Beulich wrote:
>>>>>>> 1b. Make vpci_write use write lock for writes to command register and 
>>>>>>> BARs
>>>>>>> only; keep using the read lock for all other writes.
>>>>>> I am not quite sure how to do that. Do you mean something like:
>>>>>> void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
>>>>>>                     uint32_t data)
>>>>>> [snip]
>>>>>>         list_for_each_entry ( r, &pdev->vpci->handlers, node )
>>>>>> {
>>>>>> [snip]
>>>>>>         if ( r->needs_write_lock)
>>>>>>             write_lock(d->vpci_lock)
>>>>>>         else
>>>>>>             read_lock(d->vpci_lock)
>>>>>> ....
>>>>>>
>>>>>> And provide rw as an argument to:
>>>>>>
>>>>>> int vpci_add_register(struct vpci *vpci, vpci_read_t *read_handler,
>>>>>>                           vpci_write_t *write_handler, unsigned int 
>>>>>> offset,
>>>>>>                           unsigned int size, void *data, --->>> bool 
>>>>>> write_path <<<-----)
>>>>>>
>>>>>> Is this what you mean?
>>>>> This sounds overly complicated. You can derive locally in vpci_write(),
>>>>> from just its "reg" and "size" parameters, whether the lock needs taking
>>>>> in write mode.
>>>> Yes, I started writing a reply with that. So, the summary (ROM
>>>> position depends on header type):
>>>> if ( (reg == PCI_COMMAND) || (reg == ROM) )
>>>> {
>>>>        read PCI_COMMAND and see if memory or IO decoding are enabled.
>>>>        if ( enabled )
>>>>            write_lock(d->vpci_lock)
>>>>        else
>>>>            read_lock(d->vpci_lock)
>>>> }
>>> Hmm, yes, you can actually get away without using "size", since both
>>> command register and ROM BAR are 32-bit aligned registers, and 64-bit
>>> accesses get split in vpci_ecam_write().
>> But, OS may want reading a single byte of ROM BAR, so I think
>> I'll need to check if reg+size fall into PCI_COMAND and ROM BAR
>> ranges
>>> For the command register the memory- / IO-decoding-enabled check may
>>> end up a little more complicated, as the value to be written also
>>> matters. Maybe read the command register only for the ROM BAR write,
>>> using the write lock uniformly for all command register writes?
>> Sounds good for the start.
>> Another concern is that if we go with a read_lock and then in the
>> underlying code we disable memory decoding and try doing
>> something and calling cmd_write handler for any reason then....
>>
>> I mean that the check in the vpci_write is somewhat we can tolerate,
>> but then it is must be considered that no code in the read path
>> is allowed to perform write path functions. Which brings a pretty
>> valid use-case: say in read mode we detect an unrecoverable error
>> and need to remove the device:
>> vpci_process_pending -> ERROR -> vpci_remove_device or similar.
>>
>> What do we do then? It is all going to be fragile...
> Real hardware won't cause a device to disappear upon a problem with
> a read access. There shouldn't be any need to remove a passed-through
> device either; such problems (if any) need handling differently imo.
Yes, at the moment there is a single place in the code which
removes the device (besides normal use-cases such as
pci_add_device on fail path and PHYSDEVOP_manage_pci_remove):

bool vpci_process_pending(struct vcpu *v)
{
[snip]
         if ( rc )
             /*
              * FIXME: in case of failure remove the device from the domain.
              * Note that there might still be leftover mappings. While this is
              * safe for Dom0, for DomUs the domain will likely need to be
              * killed in order to avoid leaking stale p2m mappings on
              * failure.
              */
             vpci_remove_device(v->vpci.pdev);

>
> Jan
>
>