Re: [Qemu-devel] [PATCH v1 1/2] hw/pvrdma: make DSR mapping idempotent in load_dsr()

[Marcel Apfelbaum]

On 8/28/19 5:23 PM, Sukrit Bhatnagar wrote:

Map to DSR only when there is no mapping done already i.e., when
dev->dsr_info.dsr is NULL. This allows the rest of mappings and
ring inits to be done by calling load_dsr() when DSR has already
been mapped to, somewhere else.

Move free_dsr() out of load_dsr() and call it before the latter
as and when needed. This aids the case where load_dsr() is called
having DSR mapping already done, but the rest of map and init
operations are pending, and prevents an unmap of the DSR.

Cc: Marcel Apfelbaum 
Cc: Yuval Shaia 
Signed-off-by: Sukrit Bhatnagar 
---
  hw/rdma/vmw/pvrdma_main.c | 17 +
  1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index adcf79cd63..6c90db96f9 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -172,15 +172,15 @@ static int load_dsr(PVRDMADev *dev)
  DSRInfo *dsr_info;
  struct pvrdma_device_shared_region *dsr;
  
-free_dsr(dev);

-
-/* Map to DSR */
-dev->dsr_info.dsr = rdma_pci_dma_map(pci_dev, dev->dsr_info.dma,
-  sizeof(struct pvrdma_device_shared_region));
  if (!dev->dsr_info.dsr) {
-rdma_error_report("Failed to map to DSR");
-rc = -ENOMEM;
-goto out;
+/* Map to DSR */
+dev->dsr_info.dsr = rdma_pci_dma_map(pci_dev, dev->dsr_info.dma,
+  sizeof(struct pvrdma_device_shared_region));
+if (!dev->dsr_info.dsr) {
+rdma_error_report("Failed to map to DSR");
+rc = -ENOMEM;
+goto out;
+}
  }
  
  /* Shortcuts */

@@ -402,6 +402,7 @@ static void pvrdma_regs_write(void *opaque, hwaddr addr, 
uint64_t val,
  case PVRDMA_REG_DSRHIGH:
  trace_pvrdma_regs_write(addr, val, "DSRHIGH", "");
  dev->dsr_info.dma |= val << 32;
+free_dsr(dev);
  load_dsr(dev);
  init_dsr_dev_caps(dev);
  break;


Reviewed-by: Marcel Apfelbaum 

Thanks,
Marcel

Re: [Qemu-devel] [PATCH v1 1/2] hw/pvrdma: make DSR mapping idempotent in load_dsr()

[Yuval Shaia]

On Wed, Aug 28, 2019 at 07:53:27PM +0530, Sukrit Bhatnagar wrote:
> Map to DSR only when there is no mapping done already i.e., when
> dev->dsr_info.dsr is NULL. This allows the rest of mappings and
> ring inits to be done by calling load_dsr() when DSR has already
> been mapped to, somewhere else.
> 
> Move free_dsr() out of load_dsr() and call it before the latter
> as and when needed. This aids the case where load_dsr() is called
> having DSR mapping already done, but the rest of map and init
> operations are pending, and prevents an unmap of the DSR.
> 
> Cc: Marcel Apfelbaum 
> Cc: Yuval Shaia 
> Signed-off-by: Sukrit Bhatnagar 
> ---
>  hw/rdma/vmw/pvrdma_main.c | 17 +
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> index adcf79cd63..6c90db96f9 100644
> --- a/hw/rdma/vmw/pvrdma_main.c
> +++ b/hw/rdma/vmw/pvrdma_main.c
> @@ -172,15 +172,15 @@ static int load_dsr(PVRDMADev *dev)
>  DSRInfo *dsr_info;
>  struct pvrdma_device_shared_region *dsr;
>  
> -free_dsr(dev);
> -
> -/* Map to DSR */
> -dev->dsr_info.dsr = rdma_pci_dma_map(pci_dev, dev->dsr_info.dma,
> -  sizeof(struct pvrdma_device_shared_region));
>  if (!dev->dsr_info.dsr) {
> -rdma_error_report("Failed to map to DSR");
> -rc = -ENOMEM;
> -goto out;
> +/* Map to DSR */
> +dev->dsr_info.dsr = rdma_pci_dma_map(pci_dev, dev->dsr_info.dma,
> +  sizeof(struct 
> pvrdma_device_shared_region));
> +if (!dev->dsr_info.dsr) {
> +rdma_error_report("Failed to map to DSR");
> +rc = -ENOMEM;
> +goto out;
> +}
>  }
>  
>  /* Shortcuts */
> @@ -402,6 +402,7 @@ static void pvrdma_regs_write(void *opaque, hwaddr addr, 
> uint64_t val,
>  case PVRDMA_REG_DSRHIGH:
>  trace_pvrdma_regs_write(addr, val, "DSRHIGH", "");
>  dev->dsr_info.dma |= val << 32;
> +free_dsr(dev);
>  load_dsr(dev);
>  init_dsr_dev_caps(dev);
>  break;

LGTM, thanks.

Reviewed-by: Yuval Shaia 

> -- 
> 2.21.0
> 
> 

[Qemu-devel] [PATCH v1 1/2] hw/pvrdma: make DSR mapping idempotent in load_dsr()

[Sukrit Bhatnagar]

Map to DSR only when there is no mapping done already i.e., when
dev->dsr_info.dsr is NULL. This allows the rest of mappings and
ring inits to be done by calling load_dsr() when DSR has already
been mapped to, somewhere else.

Move free_dsr() out of load_dsr() and call it before the latter
as and when needed. This aids the case where load_dsr() is called
having DSR mapping already done, but the rest of map and init
operations are pending, and prevents an unmap of the DSR.

Cc: Marcel Apfelbaum 
Cc: Yuval Shaia 
Signed-off-by: Sukrit Bhatnagar 
---
 hw/rdma/vmw/pvrdma_main.c | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index adcf79cd63..6c90db96f9 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -172,15 +172,15 @@ static int load_dsr(PVRDMADev *dev)
 DSRInfo *dsr_info;
 struct pvrdma_device_shared_region *dsr;
 
-free_dsr(dev);
-
-/* Map to DSR */
-dev->dsr_info.dsr = rdma_pci_dma_map(pci_dev, dev->dsr_info.dma,
-  sizeof(struct pvrdma_device_shared_region));
 if (!dev->dsr_info.dsr) {
-rdma_error_report("Failed to map to DSR");
-rc = -ENOMEM;
-goto out;
+/* Map to DSR */
+dev->dsr_info.dsr = rdma_pci_dma_map(pci_dev, dev->dsr_info.dma,
+  sizeof(struct pvrdma_device_shared_region));
+if (!dev->dsr_info.dsr) {
+rdma_error_report("Failed to map to DSR");
+rc = -ENOMEM;
+goto out;
+}
 }
 
 /* Shortcuts */
@@ -402,6 +402,7 @@ static void pvrdma_regs_write(void *opaque, hwaddr addr, 
uint64_t val,
 case PVRDMA_REG_DSRHIGH:
 trace_pvrdma_regs_write(addr, val, "DSRHIGH", "");
 dev->dsr_info.dma |= val << 32;
+free_dsr(dev);
 load_dsr(dev);
 init_dsr_dev_caps(dev);
 break;
-- 
2.21.0