RE: [virtio-comment] RE: [PATCH v2 1/2] transport-pci: Introduce legacy registers access commands

[Parav Pandit <parav@nvidia.com>]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Thursday, May 18, 2023 9:54 PM

> > > Hypervisor needs to inspect MSI-X enable/disable settings to decide when
> to issue command #1 vs #3 for the requested offset.
> >
> 
> I don't know why it is a problem, for any hypervisor that supports legacy, the
> code (MSI-X enablement detection) should already be there.
> 
> And it is just something like what Qemu had:
> 
> /* The remaining space is defined by each driver as the per-driver
>  * configuration space */
> #define VIRTIO_PCI_CONFIG_SIZE(dev)
> VIRTIO_PCI_CONFIG_OFF(msix_enabled(dev))
> 
> Is this really expensive?
>
Not really, its just an additional query on the config access.

> > > So we will have total 5 commands (instead of 3).
> > >
> > > 1. legacy common config read
> > > 2. legacy common config write
> > >
> > > 3. legacy device config read
> > > 4. legacy device config write
> > > 5. query device notification area
> > >
> > > #1 and #3 same cmd signature but different opcode.
> > > #2 and #4 same cmd signature but different opcode.
> > >
> >
> > Sounds reasonable. Jason?
> 
> Better, but for transport virtqueue/adminq we won't go this way but using
> individual commands for common and other configurations for a more
> deterministic behaviour with a simpler device.
> 
> Two things in my mind:
> 
> 1) There's probably no need for a BAR and it should be sufficient for an address,
> the device just needs to make sure to report a correct address if BAR is
> remapped. It helps to save BAR for VF as well.
> 2) We need a size, otherwise we don't know if the notification area could be
> mapped safely to the guest. And in the case of virtio_pci_notify_cap, though it
> could be implied by the offset but it deserves a formal clarification as well there.
>
A dedicated command that returns BAR, offset and size is elegant to avoid mixing pci capability and register linking.
Sort of self-contained command.