Re: [PATCH 1/2] hw/nvme: Implement shadow doorbell buffer support

[Jinhao Fan]

> On Jun 14, 2022, at 11:41 PM, Keith Busch <kbusch@kernel.org> wrote:
> 
> It's a pretty nasty hack, and definitely not in compliance with the spec: the
> db_addr is supposed to be read-only from the device side, though I do think
> it's safe for this environment. Unless Klaus or anyone finds something I'm
> missing, I feel this is an acceptable compromise to address this odd
> discrepency.

:) In my next patch I will check the performance numbers with this hack. Not
sure if updating db_addr value from the host will have any performance 
implications but I guess it should be OK.

> 
> I believe the recommended tag for something like this is "Suggested-by:", but
> no need to credit me. Just fold it into your first patch and send a v2.

Sure. Thanks!

> 
> By the way, I noticed that the patch never updates the cq's ei_addr value. Is
> that on purpose?

Klaus also raised a similar question in a prior comment. I think we need to 
figure
this out before we move on to the v2 patch. I did this because the original 
Google
extension patch did not update cq’s ei_addr. This seems to make sense because
the purpose of cq’s ei_addr is for the guest to notify the host about cq head
changes when necessary. However, the host does not need this notification 
because we let the host proactively check for cq’s db_addr value when it wants
to post a new cqe. This is also the only point where the host uses the cq’s
db_addr. Therefore, it is OK to postpone the check for cq’s db_addr to this 
point,
instead of getting timely but not useful notifications by updating cq’s ei_addr.
This helps to reduce the number of MMIO’s on the cq’s doorbell register.

Klaus, Keith, do you think this design makes sense?