Re: [PATCH v4 15/15] hw/nvme: Update the initalization place for the AER queue

2022-02-10 Thread Klaus Jensen
On Jan 26 18:11, Lukasz Maniak wrote:
> From: Łukasz Gieryk 
> 
> This patch updates the initialization place for the AER queue, so it’s
> initialized once, at controller initialization, and not every time
> controller is enabled.
> 
> While the original version works for a non-SR-IOV device, as it’s hard
> to interact with the controller if it’s not enabled, the multiple
> reinitialization is not necessarily correct.
> 
> With the SR/IOV feature enabled a segfault can happen: a VF can have its
> controller disabled, while a namespace can still be attached to the
> controller through the parent PF. An event generated in such case ends
> up on an uninitialized queue.
> 
> While it’s an interesting question whether a VF should support AER in
> the first place, I don’t think it must be answered today.
> 
> Signed-off-by: Łukasz Gieryk 
> ---
>  hw/nvme/ctrl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 624db2f9c6..b2228e960f 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -6029,8 +6029,6 @@ static int nvme_start_ctrl(NvmeCtrl *n)
>  
>  nvme_set_timestamp(n, 0ULL);
>  
> -QTAILQ_INIT(&n->aer_queue);
> -
>  nvme_select_iocs(n);
>  
>  return 0;
> @@ -7007,6 +7005,8 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice 
> *pci_dev)
>  id->cmic |= NVME_CMIC_MULTI_CTRL;
>  }
>  
> +QTAILQ_INIT(&n->aer_queue);
> +
>  NVME_CAP_SET_MQES(cap, 0x7ff);
>  NVME_CAP_SET_CQR(cap, 1);
>  NVME_CAP_SET_TO(cap, 0xf);
> -- 
> 2.25.1
> 

Fix is good, but I think this belongs in nvme_init_state(). Otherwise,

Reviewed-by: Klaus Jensen 


signature.asc
Description: PGP signature


[PATCH v4 15/15] hw/nvme: Update the initalization place for the AER queue

2022-01-26 Thread Lukasz Maniak
From: Łukasz Gieryk 

This patch updates the initialization place for the AER queue, so it’s
initialized once, at controller initialization, and not every time
controller is enabled.

While the original version works for a non-SR-IOV device, as it’s hard
to interact with the controller if it’s not enabled, the multiple
reinitialization is not necessarily correct.

With the SR/IOV feature enabled a segfault can happen: a VF can have its
controller disabled, while a namespace can still be attached to the
controller through the parent PF. An event generated in such case ends
up on an uninitialized queue.

While it’s an interesting question whether a VF should support AER in
the first place, I don’t think it must be answered today.

Signed-off-by: Łukasz Gieryk 
---
 hw/nvme/ctrl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 624db2f9c6..b2228e960f 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -6029,8 +6029,6 @@ static int nvme_start_ctrl(NvmeCtrl *n)
 
 nvme_set_timestamp(n, 0ULL);
 
-QTAILQ_INIT(&n->aer_queue);
-
 nvme_select_iocs(n);
 
 return 0;
@@ -7007,6 +7005,8 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice 
*pci_dev)
 id->cmic |= NVME_CMIC_MULTI_CTRL;
 }
 
+QTAILQ_INIT(&n->aer_queue);
+
 NVME_CAP_SET_MQES(cap, 0x7ff);
 NVME_CAP_SET_CQR(cap, 1);
 NVME_CAP_SET_TO(cap, 0xf);
-- 
2.25.1