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

["Michael S. Tsirkin" <mst@redhat.com>]

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 */
         u32 pasid : 20;
	 u8 reserved : 11;
	 u8 hardware : 1;
};

or we can stick two lengths and addresses straight in the capability.


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

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


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

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


> 
> So I think a bar or a cap processing admin cmds is way to complex and
> overkill.
> 
> Thanks

Sounds like a straw man argument.

-- 
MST