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

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

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.

Thanks, Roger.