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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Jan 31, 2022 at 11:23:48AM +0000, Oleksandr Andrushchenko wrote:
> 
> 
> 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.

Well, we can only offer handling of the header and the MSI and MSI-X
capabilities right now, because that's all vPCI currently knows about.

> 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.

See my other reply, you can probably move the special handlers into a
separate patch at the end of the series in order to test the
functionality without adding code that will need to be removed when
the defaults for domUs are changed.

> Another question is what needs to be done for vendor specific capabilities?
> How these are going to be emulated?

I think you will need some kind of permissive mode in order to allow a
guest to access those, as they shouldn't be exposed by default.

Thanks, Roger.