[virtio-dev] Re: [virtio-comment] Re: [PATCH 00/11] Introduce transitional mmr pci device

[Parav Pandit <parav@nvidia.com>]

On 4/3/2023 5:04 PM, Michael S. Tsirkin wrote:
> On Mon, Apr 03, 2023 at 08:25:02PM +0000, Parav Pandit wrote:
>>
>>> From: Michael S. Tsirkin <mst@redhat.com>
>>> Sent: Monday, April 3, 2023 2:02 PM
>>
>>>> Because vqs involve DMA operations.
>>>> It is left to the device implementation to do it, but a generic wisdom
>>>> is not implement such slow work in the data path engines.
>>>> So such register access vqs can/may be through firmware.
>>>> Hence it can involve a lot higher latency.
>>>
>>> Then that wisdom is wrong? tens of microseconds is not workable even for
>>> ethtool operations, you are killing boot time.
>>>
>> Huh.
>> What ethtool latencies have you experienced? Number?
> 
> I know an order of tens of eth calls happens during boot.
> If as you said each takes tens of ms then we are talking close to a second.
> That is measureable.
I said it can take, doesn't have to be always same for all the commands.
Better to work with real numbers. :)

Let me take an example to walk through.

If a cvq or aq command takes 0.5msec, total of 100 such commands will 
take 50msec.

Once a while if two of commands say take 5msec, will result in 50 -> 60 
msec.


> OK then. Then if it is a dead end then it looks weird to add a whole new
> config space as memory mapped.
> 
I am aligned with you to not add any new register as memory mapped for 1.x.
Or access through device own's tvq is fine if such q can be initialized 
before during device reset (init) phase.

I explained that legacy registers are sub-set of existing 1.x.
They should not consume extra memory.

Lets walk through the merits and negatives of both to conclude.

>>> Let me try again.

> If hardware vendors do not want to bear the costs of registers then they
> will not implement devices with registers, and then the whole thing will
> become yet another legacy thing we need to support. If legacy emulation
> without IO is useful, then can we not find a way to do it that will
> survive the test of time?
legacy_register_transport_vq for VF can be a option, but not for PF 
emulation.
More below.

> 
>> Again, I want to emphasize that register read/write over tvq has merits with trade-off.
>> And so the mmr has merits with trade-off too.
>>
>> Better to list them and proceed forward.
>>
>> Method-1: VF's register read/write via PF based transport VQ
>> Pros:
>> a. Light weight registers implementation in device for new memory region window
> 
> Is that all? I mentioned more.
> 
b. device reset is more optimal with transport VQ
c. a hypervisor may want to check (but not necessary) register content
d. Some unknown guest VM driver which modifies mac address and still 
expect atomicity can benefit if hypervisor wants to do extra checks

>> Cons:
>> a. Higher DMA read/write latency
>> b. Device requires synchronization between non legacy memory mapped registers and legacy regs access via tvq
> 
> Same as a separate mmemory bar really.  Just don't do it. Either access
> legacy or non legacy.
> 
It is really not same to treat them equally as tvq encapsulation is 
different, and hw wouldn't prefer to treat them equally like regular 
memory writes.

Transitional device exposed by hypervisor contains both legacy I/O bar 
and also the memory mapped registers. So a guest vm can access both.

>> c. Can only work with the VF. Cannot work for thin hypervisor, which can map transitional PF to bare metal OS
>> (also listed in cover letter)
> 
> Is that a significant limitation? Why?
It is a functional limitation for the PF, as PF has no parent.
and PF can also utilize memory BAR.

> 
>> Method-2: VF's register read/write via MMR (current proposal)
>> Pros:
>> a. Device utilizes the same legacy and non-legacy registers.
> 
>> b. an order of magnitude lower latency due to avoidance of DMA on register accesses
>> (Important but not critical)
> 
> And no cons? Even if you could not see them yourself did I fail to express myself to such
> an extent?
> 
Method-1 pros covered the advantage of it over method-2, but yes worth 
to list here for completeness.

Cons:
requires creating new memory region window in the device for 
configuration access

>>>> No. Interrupt latency is in usec range.
>>>> The major latency contributors in msec range can arise from the device side.
>>>
>>> So you are saying there are devices out there already with this MMR hack
>>> baked in, and in hardware not firmware, so it works reasonably?
>> It is better to not assert a solution a "hack",
> 
> Sorry if that sounded offensive.  a hack is not necessary a bad thing.
> It's a quick solution to a very local problem, though.
> 
It is a solution because device can do at near to zero extra memory for 
existing registers.
Anyways, we have better technical details to resolve. :)
Lets focus on it.

> Yes motivation is one of the things I'm trying to work out here.
> It does however not help that it's an 11 patch strong patchset
> adding 500 lines of text for what is supposedly a small change.
> 
Many of the patches are rework and incorrect to attribute to the 
specific feature.

Like others it could have been one giant patch... but we see value in 
smaller patches..

Using tvq is even bigger change than this. So we shouldn't be afraid of 
making transitional device actually work using it with larger spec patch.

>> Regarding tvq, I have some idea on how to improve the register read/writes so that its optimal for devices to implement.
> 
> Sounds useful, and maybe if tvq addresses legacy need then focus on
> that?
> 

tvq specific for legacy register access make sense.
Some generic tvq is abstract and dont see any relation here.

So better to name it as legacy_reg_transport_vq (lrt_vq).

How about having below format?

/* Format of 16B descriptors for lrt_vq
  * lrt_vq = legacy register tranport vq.
  */
struct legacy_reg_req_vf {
	union {
		struct {
			le32 reg_wr_data;
			le32 reserved;
		} write;
		struct {
			le64 reg_read_addr;
		};
	};
	le8 rd_wr : 1;	/* rd=0, wr=1 */
	le8 reg_byte_offset : 7;
	le8 req_tag;	/* unique request tag on this vq */
	le16 vf_num;

	le16 flags; /* new flag below */
         le16 next;
};

#define VIRTQ_DESC_F_Q_DEFINED 8
/* Content of the VQ descriptor other than flags field is VQ
  * specific and defined by the VQ type.
  */

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org