Re: [PATCH v5 06/14] vpci/header: implement guest BAR register handlers

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 31.01.22 13:10, Roger Pau Monné wrote:
> On Mon, Jan 31, 2022 at 10:40:47AM +0000, Oleksandr Andrushchenko wrote:
>> On 31.01.22 11:47, Oleksandr Andrushchenko wrote:
>>> Hi, Roger!
>>>
>>> On 12.01.22 14:35, Roger Pau Monné wrote:
>>>>> +static void guest_rom_write(const struct pci_dev *pdev, unsigned int reg,
>>>>> +                            uint32_t val, void *data)
>>>>> +{
>>>>> +}
>>>>> +
>>>>> +static uint32_t guest_rom_read(const struct pci_dev *pdev, unsigned int 
>>>>> reg,
>>>>> +                               void *data)
>>>>> +{
>>>>> +    return 0xffffffff;
>>>>> +}
>>>> There should be no need for those handlers. As said elsewhere: for
>>>> guests registers not explicitly handled should return ~0 for reads and
>>>> drop writes, which is what you are proposing here.
>>> Yes, you are right: I can see in vpci_read that we end up reading ~0 if no
>>> handler exists (which is what I do here with guest_rom_read). But I am not 
>>> that
>>> sure about the dropped writes:
>>>
>>> void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
>>>                    uint32_t data)
>>> {
>>>        unsigned int data_offset = 0;
>>>
>>> [snip]
>>>
>>>        if ( data_offset < size )
>>>            /* Tailing gap, write the remaining. */
>>>            vpci_write_hw(sbdf, reg + data_offset, size - data_offset,
>>>                          data >> (data_offset * 8));
>>>
>>> so it looks like for the un-handled writes we still reach the HW register.
>>> Could you please tell if the code above needs improvement (like checking
>>> if the write was handled) or I still need to provide a write handler, e.g.
>>> guest_rom_write here?
>> Hm, but the same applies to the reads as well... And this is no surprise,
>> as for the guests I can see that it accesses all the configuration space
>> registers that I don't handle. Without that I would have guests unable
>> to properly setup a PCI device being passed through... And this is why
>> I have a big TODO in this series describing unhandled registers.
>> So, it seems that I do need to provide those handlers which I need to
>> drop writes and return ~0 on reads.
> Right (see my previous reply to this comment). I think it would be
> easier (and cleaner) if you switched the default behavior regarding
> unhandled register access for domUs at the start of the series (drop
> writes, reads returns ~0), and then you won't need to add all those
> dummy handler to drop writes and return ~0 for reads.
>
> It's going to be more work initially as you would need to support
> passthrough of more registers, but it's the right approach that we
> need implementation wise.
While I agree in general, this effectively means that I'll need to provide
handling for all PCIe registers and capabilities from the very start.
Otherwise no guest be able to properly initialize a PCI device without that.
Of course, we may want starting from stubs instead of proper emulation,
which will direct the access to real HW and later on we add proper emulation.
But, again, this is going to be a rather big piece of code where we need
to explicitly handle every possible capability.

At the moment we are not going to claim that vPCI provides all means to
pass through a PCI device safely with this respect and this is why the feature
itself won't even be a tech preview yet. For that reason I think we can still
have implemented only crucial set of handlers and still allow the rest to
be read/write directly without emulation.

Another question is what needs to be done for vendor specific capabilities?
How these are going to be emulated?
>
> Thanks, Roger.
Thank you,
Oleksandr