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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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.

Jan