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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

Thanks, Roger.