Re: [virtio-comment] Re: [PATCH V2 6/6] virtio-pci: implement dirty page tracking

["Zhu, Lingshan" <lingshan.zhu@intel.com>]

On 11/6/2023 6:15 PM, Michael S. Tsirkin wrote:
> On Mon, Nov 06, 2023 at 05:16:43PM +0800, Zhu, Lingshan wrote:
> > On 11/3/2023 10:21 PM, Zhu, Lingshan wrote:
> > > On 11/3/2023 6:46 PM, Michael S. Tsirkin wrote:
> > > > On Fri, Nov 03, 2023 at 06:34:37PM +0800, Zhu Lingshan wrote:
> > > > > +\begin{lstlisting}
> > > > > +struct virtio_pci_dity_page_track {
> > > > > +ÂÂÂÂÂÂÂ u8 enable;ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ /* Read-Write */
> > > > > +ÂÂÂÂÂÂÂ u8 gra_power;ÂÂÂÂÂÂÂÂÂÂÂ /* Read-Write */
> > > > > +ÂÂÂÂÂÂÂ u8 reserved[2];
> > > > > +ÂÂÂÂÂÂÂ le32 {
> > > > > +ÂÂÂÂÂÂÂÂÂÂÂ pasid: 20;ÂÂÂÂÂÂÂÂÂÂ /* Read-Write */
> > > > > +ÂÂÂÂÂÂÂÂÂÂÂ reserved: 12;
> > > > > +ÂÂÂÂÂÂÂ };
> > > > > +ÂÂÂÂÂÂÂ le64 bitmap_addr;ÂÂÂÂÂÂÂ /* Read-Write */
> > > > > +ÂÂÂÂÂÂÂ le64 bitmap_length;ÂÂÂÂÂ /* Read-Write */
> > > > > +};
> > > > > +\end{lstlisting}
> > > > Okay, so it's a simple mailbox in config space. Which by itself is
> > > > probably a very reasonable idea - more or less what I suggested.
> > > > However, using such a generic facility just for the dirty bitmap seems
> > > > too limited. Please make it accept arbitrary commands. Reusing admin
> > > > command structure with a special "device itself" group sounds like one
> > > > way to do it.
> > > processing admin cmds in a cap may be too complex and overkill.
> > > we need to handle variable length of cmds, handle async returned
> > > results, and so on.
> > >
> > > This struct seems easy and simple. And shall we use platform facilities
> > > like vt-d
> > > to track dirty pages?
> > To demonstrate these issues, suppose we have a struct in a bar to process
> > admin cmds:
> >
> > struct virtio_admin_cmd {
> >  ÂÂÂÂÂÂÂ u64 in_data_length;
> >  ÂÂÂÂÂÂÂ u8 cmd_in_data[];
> >  ÂÂÂÂÂÂÂ u64 out_data_length;
> >  ÂÂÂÂÂÂÂ u8 cmd_out_data[];
> >  ÂÂÂÂÂÂÂ u8 ret;
> > };
> An alternative is do same as you did here, e.g.:
> struct virtio_admin_cmd {
>   ÂÂÂÂÂÂÂ u64 admin_cmd_pa; /* an out descriptor followed by an in descriptor */
PA depends on IOMMU and ATS
>   ÂÂÂÂÂÂÂ u32 pasid : 20;
> 	 u8 reserved : 11;
> 	Âu8 hardware : 1;
> };
>
> or we can stick two lengths and addresses straight in the capability.
Still can only process only one cmd at a time and others are blocked.
> > The problems are:
> > 1) command_in_data and command_out data have variable length, so how many HW
> > resource should be reserved in the bar?
> actually admin commands are truncated by device so just
> set to length that device understands.
How the framework know the length? How can the vendor know how many
HW resource they should place here? I am not sure it is a good
idea to guess.
> > 2) To process the cmds in the bar, the device MAY need to read many
> > registers in cmd_in_data[] and write many registers in cmd_out_data[],
> > which can be ineffective, this is not DMA.
> True. Again if you don't want to depend on pasid that's the
> only option.
Yes, this exactly shows how complex and overkill to use
a cap to handle admin cmds. And maybe admin cmds is not a must.
> > 3) a bar can only process one cmd at a time, and the driver can only issue
> > another cmd after received an ret.
> > This process has to be synchronous IO, one cmd blocks another.
> Exactly same as what you did though.
We have implemented a cap in PCI for dirty page tracking, there are no 
admin cmds.

And for dirty page tracking, we still believe it is better to use 
platform facilities,
as Jason explained.
> > 4) VF implementing a bar processing admin cmds conflicts with PF's admin vq.
> So just don't create conflicts. It's same as multiple admin vqs
> really which we already support.
How to tell the SW don't create conflict?
> > So I think a bar or a cap processing admin cmds is way to complex and
> > overkill.
> >
> > Thanks
> Sounds like a straw man argument.