Re: [PATCH v9 vfio 4/7] vfio/pds: Add VFIO live migration support

[Brett Creeley <bcreeley@amd.com>]

On 5/4/2023 10:50 AM, Jason Gunthorpe wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Fri, Apr 21, 2023 at 06:06:39PM -0700, Brett Creeley wrote:
> 
>> +static int
>> +pds_vfio_dma_map_lm_file(struct device *dev, enum dma_data_direction dir,
>> +                      struct pds_vfio_lm_file *lm_file)
>> +{
>> +     struct pds_lm_sg_elem *sgl, *sge;
>> +     struct scatterlist *sg;
>> +     dma_addr_t sgl_addr;
>> +     size_t sgl_size;
>> +     int err;
>> +     int i;
>> +
>> +     if (!lm_file)
>> +             return -EINVAL;
>> +
>> +     /* dma map file pages */
>> +     err = dma_map_sgtable(dev, &lm_file->sg_table, dir, 0);
>> +     if (err)
>> +             return err;
>> +
>> +     lm_file->num_sge = lm_file->sg_table.nents;
>> +
>> +     /* alloc sgl */
>> +     sgl_size = lm_file->num_sge * sizeof(struct pds_lm_sg_elem);
>> +     sgl = kzalloc(sgl_size, GFP_KERNEL);
>> +     if (!sgl) {
>> +             err = -ENOMEM;
>> +             goto err_alloc_sgl;
>> +     }
>> +
>> +     sgl_addr = dma_map_single(dev, sgl, sgl_size, DMA_TO_DEVICE);
>> +     if (dma_mapping_error(dev, sgl_addr)) {
>> +             err = -EIO;
>> +             goto err_map_sgl;
>> +     }
>> +
>> +     lm_file->sgl = sgl;
>> +     lm_file->sgl_addr = sgl_addr;
>> +
>> +     /* fill sgl */
>> +     sge = sgl;
>> +     for_each_sgtable_dma_sg(&lm_file->sg_table, sg, i) {
>> +             sge->addr = cpu_to_le64(sg_dma_address(sg));
>> +             sge->len  = cpu_to_le32(sg_dma_len(sg));
>> +             dev_dbg(dev, "addr = %llx, len = %u\n", sge->addr, sge->len);
>> +             sge++;
>> +     }
> 
> This sequence is in the wrong order, the dma_map_single() has to be
> after the data is written to the memory as it synchronizes the caches
> on some arches

This is an easy enough fix and I'll keep this in mind moving forward. 
Thanks for pointing it out.

> 
>> +
>> +     return 0;
>> +
>> +err_map_sgl:
>> +     kfree(sgl);
>> +err_alloc_sgl:
>> +     dma_unmap_sgtable(dev, &lm_file->sg_table, dir, 0);
>> +     return err;
> 
> And why is the goto error unwind in a messed up order? Error unwinds
> should always be strictly in the opposite order of the success
> path. Check them all when you are fixing the "come from" notation
> 
> Jason

Yeah, this wasn't on purpose. Thanks again for pointing things like this 
out. As you noted I will go through the entire series to make sure the 
goto labels are named by what they are freeing/clearing and also that 
they are in the correct opposite order.

Thanks again for the review.

Brett