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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>]

On 31.01.22 13:27, Roger Pau Monné wrote:
> On Mon, Jan 31, 2022 at 11:04:29AM +0000, Oleksandr Andrushchenko wrote:
>> Hi, Jan!
>>
>> On 31.01.22 12:54, Jan Beulich wrote:
>>> On 31.01.2022 11:40, 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.
>> Replying to myself: it is still possible to have vpci_ignored_{read|write}
>> to handle defaults if, when vpci_add_register is called, the handler
>> provided is NULL
>>> It feels like we had been there before: For your initial purposes it may
>>> be fine to do as you suggest, but any such patches should carry RFC tags
>>> or alike to indicate they're not considered ready. Once you're aiming
>>> for things to go in, I think there's no good way around white-listing
>>> what guests may access. You may know that we've been bitten by starting
>>> out with black-listing in the past, first and foremost with x86'es MSRs.
>> I already have a big TODO patch describing the issue. Do you want
>> it to have a list of handlers that we support as of now? What sort of
>> while/black list would you expect?
>> I do understand that we do need proper handling for all the PCI registers
>> and capabilities long term, but this can't be done at the moment when
>> we have nothing working at all. Requesting proper handling now will
>> turn this series into a huge amount of code and undefined time frame.
> We should at least make sure the code added now doesn't need to be
> changed in the future when the default is switched. If you don't
> want to switch the default handling for domUs to ignore writes and
> return ~0 from reads to unhandled registers right now you should keep
> the patches that add the ignore handlers to the end of the series and
> mark them as 'HACK' or some such in order to notice they are just
> used for testing purposes.
Or for all the registers that I do want the writes to be rejected and
reads return ~0 I can pass NULL while calling vpci_add_register,
so the following works:

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)
{
[snip]
     r->read = read_handler ?: vpci_ignored_read;
     r->write = write_handler ?: vpci_ignored_write;
which does what we want.
> Thanks, Roger.
Thank you,
Oleksandr