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

[Parav Pandit <parav@nvidia.com>]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Thursday, May 18, 2023 3:42 PM
> 
> On Wed, May 17, 2023 at 07:32:33PM +0000, Parav Pandit wrote:
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Wednesday, May 17, 2023 1:45 AM
> > >
> > > On Sat, May 06, 2023 at 03:01:34AM +0300, Parav Pandit wrote:
> > > > +\begin{lstlisting}
> > > > +struct virtio_admin_cmd_lreg_wr_data {
> > > > +	u8 offset; /* Starting byte offset of the register(s) to write */
> > > > +	u8 register[];
> > > > +};
> > > > +\end{lstlisting}
> > >
> > > So to summarize, I think the main comment here was to have separate
> > > commands for access to common versus device config. Just today I
> > > thought some more and came up with another reason why this is a good
> > > idea: if we ever want legacy MMIO emulation, we can reuse the device
> config command.
> > >
> > MMIO is actually simpler than PCI because all the registers are contiguous
> regardless of msix.
> > So single command with offset can service just fine.
> 
> yes but then you need to implement both MMIO and PCI in hardware.
> with a flag you can implement legacy MMIO in software on top of legacy PCI in
> hardware.
> 
> > More below.
> >
> > > Jason also feels the common config command can be shared with vq
> > > transport effort. We can discuss this, for sure, I guess its putting
> > > code in hardware versus in hypervisor, but if there are hardware
> > > vendors who strictly want this code in hardware I don't have a big problem
> with this even if I don't exactly get why.
> > >
> > > With this idea all hypervisor has to do is subtract the offset from
> > > device config access, sounds like a small price to pay.
> > It is more than the subtraction cost.
> >
> > Hypervisor needs to inspect MSI-X enable/disable settings to decide when to
> issue command #1 vs #3 for the requested offset.
> 
> I thought that for now you wanted to ignore MSI-X disable case?
> Because if you do care then we need INTx emulation.
> 
Right. But the ignorance falls under should category.
So when INTX is enabled/disabled, AQ commands etc can be anchored there.

As opposed to that, config register access needs to decide what to do based on MSIX state.
So hypervisor now must trap MSIX capability access.

With single command, it is not the case in hypervisor.

But it is likely not a big problem given both are in the control path.

> 
> > > Does this sound like a reasonable
> > > compromize to you?
> >
> > Splitting proposed one command to two commands, 1. one for accessing
> > legacy common config 2. second for accessing legacy device specific
> > config
> >
> > seems fine to me as below.
> >
> > 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?
> 
> notification thing needs more thought I feel though.
> It feels weirdly bolted on, but I can't put my finger on what's wrong exactly yet.
> Will think it over.
> 
Ok. I considered alternative to get that from the VF itself, but it was not elegant with little value in future.
Will wait if you have better idea, else will proceed to v3.