Re: [virtio-comment] RE: [PATCH v5 2/7] Introduce admin command set

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

On Mon, Jun 20, 2022 at 07:54:00PM +0300, Max Gurtovoy wrote:
> 
> On 6/20/2022 7:46 PM, Michael S. Tsirkin wrote:
> > On Mon, Jun 20, 2022 at 11:06:07AM +0000, Parav Pandit wrote:
> > > 
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Monday, June 20, 2022 6:00 AM
> > > > 
> > > > On Tue, May 31, 2022 at 08:39:24PM +0000, Parav Pandit wrote:
> > > > > > And as I said, you will need much more spec work to reach the level
> > > > > > to which features are specified - and note we are not yet happy with
> > > > > > how features are
> > > > > Can you be specific of the work that you are expecting in this v5 version?
> > > > I proposed getting rid of attrs_mask/device_admin_caps and instead specify
> > > > that e.g. feature bits 64 to 95 are reserved for management purposes.
> > > > Maybe just add a 32 bit register, or maybe "extended features"
> > > > with a selector and length like mmio and ccw already do, up to you.
> > > > I think Cornelia likes this suggestion too.
> > > > 
> > > > This would replace functionality of the
> > > > VIRTIO_ADMIN_DEVICE_CAPS_IDENTIFY/VIRTIO_ADMIN_DEVICE_CAPS_AC
> > > > CEPT
> > > > commands and have the advantage of being generally well specified and
> > > > understood.
> > > > 
> > > > Is this acceptable? Or is there a reason the new commands are preferable?
> > > We prefer the new commands for following reason.
> > > 
> > > 1. proposed new command doesn't demand exposing registers in the PCI memory mapped area
> > > 2. Today AQ is perceived in to be PF, but it doesn't have to be. For next 10 years spec may evolve to have AQ for some other purpose on VFs/SFs.
> > > Such devices scale at much higher magnitude than PFs.
> > > And exposing memory mapped registers for rare functionality is the last thing to do in my mind.
> > > 3. Placements of such features bits in an AQ gives device lot more flexibility on _how_ to implement them. Some in sw, fw, hw, memory die etc.
> > > Placement of them in PCI register space reduces these options.
> > > 
> > > So exposing them something in PCI register space has to have strong technical reason than just simplicity of access.
> > I ansolutely get this argument. Please do get mine which is avoiding
> > duplicating very similar functionality is subtly differing ways.
> > 
> > Your above arguments apply equally to most other registers we already
> > have.  Wouldn't you then agree we need to address this more drastically by
> > defining an alternative transport such as virtio over virtqueue then?
> 
> No.
> 
> Virtqueues are part of the virtio device.
> 
> Admin virtq is yet another queue. It has its own command set. That's it.
> 
> And it's ok to have an administration queue to do some control
> configurations. It is common practice in other specifications and not of
> them created a new "queue transport".
> 
> Adding a transport is not needed and not relevant to this solution.
> 

Max with transport I am not talking about admin queue specifically.
The point Parav made is that he wants to avoid using memory
writable registers for slow path configuration, as far as possible.


Presumably the idea is to keep the information in question
outside the device, with pci spec rules preventing
blocking writes until another write completes (doing so would
create potential for deadlocks).

To that end your proposal does its best to keep new registers outside
device configuration space.

My response is that we already have a large number of registers,
so why don't we define a transport which avoids writes for
configuration completely? This seems to go further than
just saving 32 bit of capability registers.
This way we can focus on just getting the functionality right
without worrying too much about conserving memory space, and then work
on reducing memory space use separately by moving things out of there.

Thanks,

-- 
MST