qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Jonathan Cameron via <qemu-devel@nongnu.org>
To: Fan Ni <fan.ni@samsung.com>
Cc: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>,
	"gregory.price@memverge.com" <gregory.price@memverge.com>,
	"hchkuo@avery-design.com.tw" <hchkuo@avery-design.com.tw>,
	"cbrowy@avery-design.com" <cbrowy@avery-design.com>,
	"ira.weiny@intel.com" <ira.weiny@intel.com>,
	"dan.j.williams@intel.com" <dan.j.williams@intel.com>,
	 Adam Manzanares <a.manzanares@samsung.com>,
	"dave@stgolabs.net" <dave@stgolabs.net>,
	"nmtadam.samsung@gmail.com" <nmtadam.samsung@gmail.com>,
	 "nifan@outlook.com" <nifan@outlook.com>
Subject: Re: [Qemu PATCH v2 9/9] hw/mem/cxl_type3: Add dpa range validation for accesses to dc regions
Date: Mon, 7 Aug 2023 09:53:42 +0100	[thread overview]
Message-ID: <20230807095342.00006f88@Huawei.com> (raw)
In-Reply-To: <20230725183939.2741025-10-fan.ni@samsung.com>

On Tue, 25 Jul 2023 18:39:56 +0000
Fan Ni <fan.ni@samsung.com> wrote:

> From: Fan Ni <nifan@outlook.com>
> 
> Not all dpa range in the dc regions is valid to access until an extent
> covering the range has been added. Add a bitmap for each region to
> record whether a dc block in the region has been backed by dc extent.
> For the bitmap, a bit in the bitmap represents a dc block. When a dc
> extent is added, all the bits of the blocks in the extent will be set,
> which will be cleared when the extent is released.
> 
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
Hi Fan,

A few of the bits of feedback apply broadly across the series.  Given I'm
rebasing this anyway to give myself something to test I'll tidy things up
(feel free to disagree with and revert any changes !) 
and push a tree out in next day or two.  I'll message when I've done so.

Jonathan

> ---
>  hw/mem/cxl_type3.c          | 155 ++++++++++++++++++++++++++++++++++++
>  include/hw/cxl/cxl_device.h |   1 +
>  2 files changed, 156 insertions(+)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 41a828598a..51943a36fc 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -787,13 +787,37 @@ static int cxl_create_dc_regions(CXLType3Dev *ct3d)
>          /* dsmad_handle is set when creating cdat table entries */
>          region->flags = 0;
>  
> +        region->blk_bitmap = bitmap_new(region->len / region->block_size);

In common with many allocators in qemu if this fails it calls abort()
internally so no need to handle potential errors.

> +        if (!region->blk_bitmap) {
> +            break;
> +        }
> +
>          region_base += region->len;
>      }
> +
> +    if (i < ct3d->dc.num_regions) {
> +        while (--i >= 0) {
> +            g_free(ct3d->dc.regions[i].blk_bitmap);
> +        }
> +        return -1;
> +    }
> +
>      QTAILQ_INIT(&ct3d->dc.extents);
>  
>      return 0;
>  }
>  
> +static void cxl_destroy_dc_regions(CXLType3Dev *ct3d)
> +{
> +    int i;
> +    struct CXLDCD_Region *region;
> +
> +    for (i = 0; i < ct3d->dc.num_regions; i++) {
> +        region = &ct3d->dc.regions[i];
> +        g_free(region->blk_bitmap);
> +    }
> +}
> +
>  static bool cxl_setup_memory(CXLType3Dev *ct3d, Error **errp)
>  {
>      DeviceState *ds = DEVICE(ct3d);
> @@ -1021,6 +1045,7 @@ err_free_special_ops:
>      g_free(regs->special_ops);
>  err_address_space_free:
>      if (ct3d->dc.host_dc) {
> +        cxl_destroy_dc_regions(ct3d);
>          address_space_destroy(&ct3d->dc.host_dc_as);
>      }
>      if (ct3d->hostpmem) {
> @@ -1043,6 +1068,7 @@ static void ct3_exit(PCIDevice *pci_dev)
>      spdm_sock_fini(ct3d->doe_spdm.socket);
>      g_free(regs->special_ops);
>      if (ct3d->dc.host_dc) {
> +        cxl_destroy_dc_regions(ct3d);
>          address_space_destroy(&ct3d->dc.host_dc_as);
>      }
>      if (ct3d->hostpmem) {
> @@ -1053,6 +1079,110 @@ static void ct3_exit(PCIDevice *pci_dev)
>      }
>  }
>  
> +/*
> + * This function will marked the dpa range [dpa, dap + len) to be backed and
> + * accessible, this happens when a dc extent is added and accepted by the
> + * host.
> + */
> +static void set_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
> +        uint64_t len)
> +{
> +    int i;
> +    CXLDCD_Region *region = &ct3d->dc.regions[0];
> +
> +    if (dpa < region->base
> +            || dpa >= region->base + ct3d->dc.total_capacity)
> +        return;
> +
> +    /*
> +     * spec 3.0 9.13.3: Regions are used in increasing-DPA order, with
> +     * Region 0 being used for the lowest DPA of Dynamic Capacity and
> +     * Region 7 for the highest DPA.
> +     * So we check from the last region to find where the dpa belongs.
> +     * access across multiple regions is not allowed.
> +     **/
> +    for (i = ct3d->dc.num_regions - 1; i >= 0; i--) {
> +        region = &ct3d->dc.regions[i];
> +        if (dpa >= region->base) {
> +            break;
> +        }
> +    }
> +
> +    bitmap_set(region->blk_bitmap, (dpa - region->base) / region->block_size,
> +            len / region->block_size);
> +}
> +
> +/*
> + * This function check whether a dpa range [dpa, dpa + len) has been backed
> + * with dc extents, used when validating read/write to dc regions
> + */
> +static bool test_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
> +        uint64_t len)
> +{
> +    int i;
> +    CXLDCD_Region *region = &ct3d->dc.regions[0];
> +    uint64_t nbits;
> +    long nr;
> +
> +    if (dpa < region->base
> +            || dpa >= region->base + ct3d->dc.total_capacity)
> +        return false;
> +
> +    /*
> +     * spec 3.0 9.13.3: Regions are used in increasing-DPA order, with
> +     * Region 0 being used for the lowest DPA of Dynamic Capacity and
> +     * Region 7 for the highest DPA.
> +     * So we check from the last region to find where the dpa belongs.
> +     * access across multiple regions is not allowed.
> +     */
> +    for (i = ct3d->dc.num_regions - 1; i >= 0; i--) {
> +        region = &ct3d->dc.regions[i];
> +        if (dpa >= region->base) {
> +            break;
> +        }
> +    }
> +
> +    nr = (dpa - region->base) / region->block_size;
> +    nbits = len / region->block_size;
> +    return find_next_zero_bit(region->blk_bitmap, nbits, nr) >= nr + nbits;
> +}
> +
> +/*
> + * This function will marked the dpa range [dpa, dap + len) to be unbacked and
> + * inaccessible, this happens when a dc extent is added and accepted by the
> + * host.
> + */
> +static void clear_region_block_backed(CXLType3Dev *ct3d, uint64_t dpa,
> +        uint64_t len)
> +{
> +    int i;
> +    CXLDCD_Region *region = &ct3d->dc.regions[0];
> +    uint64_t nbits;
> +    long nr;
> +
> +    if (dpa < region->base
> +            || dpa >= region->base + ct3d->dc.total_capacity)
> +        return;
> +
> +    /*
> +     * spec 3.0 9.13.3: Regions are used in increasing-DPA order, with
> +     * Region 0 being used for the lowest DPA of Dynamic Capacity and
> +     * Region 7 for the highest DPA.
> +     * So we check from the last region to find where the dpa belongs.
> +     * access across multiple regions is not allowed.
> +     */
> +    for (i = ct3d->dc.num_regions - 1; i >= 0; i--) {
> +        region = &ct3d->dc.regions[i];
> +        if (dpa >= region->base) {
> +            break;
> +        }
> +    }
> +
> +    nr = (dpa - region->base) / region->block_size;
> +    nbits = len / region->block_size;
> +    bitmap_clear(region->blk_bitmap, nr, nbits);
> +}
> +
>  static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr host_addr, uint64_t *dpa)
>  {
>      uint32_t *cache_mem = ct3d->cxl_cstate.crb.cache_mem_registers;
> @@ -1145,6 +1275,10 @@ static int cxl_type3_hpa_to_as_and_dpa(CXLType3Dev *ct3d,
>          *as = &ct3d->hostpmem_as;
>          *dpa_offset -= vmr_size;
>      } else {
> +        if (!test_region_block_backed(ct3d, *dpa_offset, size)) {
> +            return -ENODEV;
> +        }
> +
>          *as = &ct3d->dc.host_dc_as;
>          *dpa_offset -= (vmr_size + pmr_size);
>      }
> @@ -1944,6 +2078,27 @@ static void qmp_cxl_process_dynamic_capacity_event(const char *path,
>      }
>  
>      g_free(extents);
> +
> +    /* Another choice is to do the set/clear after getting mailbox response*/
> +    list = records;
> +    while (list) {
> +        dpa = list->value->dpa * 1024 * 1024;
> +        len = list->value->len * 1024 * 1024;
> +        rid = list->value->region_id;
> +
> +        switch (type) {
> +        case DC_EVENT_ADD_CAPACITY:
> +            set_region_block_backed(dcd, dpa, len);
> +            break;
> +        case DC_EVENT_RELEASE_CAPACITY:
> +            clear_region_block_backed(dcd, dpa, len);
> +            break;
> +        default:
> +            error_setg(errp, "DC event type not handled yet");
> +            break;
> +        }
> +        list = list->next;
> +    }
>  }
>  
>  void qmp_cxl_add_dynamic_capacity_event(const char *path,
> diff --git a/include/hw/cxl/cxl_device.h b/include/hw/cxl/cxl_device.h
> index 01a5eaca48..1f85c88017 100644
> --- a/include/hw/cxl/cxl_device.h
> +++ b/include/hw/cxl/cxl_device.h
> @@ -412,6 +412,7 @@ typedef struct CXLDCD_Region {
>      uint64_t block_size;
>      uint32_t dsmadhandle;
>      uint8_t flags;
> +    unsigned long *blk_bitmap;
>  } CXLDCD_Region;
>  
>  struct CXLType3Dev {



  reply	other threads:[~2023-08-07  8:54 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20230725183956uscas1p154e945516c2a4091479f4906d7652648@uscas1p1.samsung.com>
2023-07-25 18:39 ` [Qemu PATCH v2 0/9] Enabling DCD emulation support in Qemu Fan Ni
     [not found]   ` <CGME20230725183956uscas1p17a64ec512cdf5b9348451926d6f0b224@uscas1p1.samsung.com>
2023-07-25 18:39     ` [Qemu PATCH v2 1/9] hw/cxl/cxl-mailbox-utils: Add dc_event_log_size field to output payload of identify memory device command Fan Ni
2023-08-04 14:19       ` Jonathan Cameron via
     [not found]   ` <CGME20230725183956uscas1p296403063c710f4b546d4fec7650915c4@uscas1p2.samsung.com>
2023-07-25 18:39     ` [Qemu PATCH v2 2/9] hw/cxl/cxl-mailbox-utils: Add dynamic capacity region representative and mailbox command support Fan Ni
2023-08-04 15:24       ` Jonathan Cameron via
     [not found]   ` <CGME20230725183956uscas1p153242eb4b12cb9cb6529476b4e9058c4@uscas1p1.samsung.com>
2023-07-25 18:39     ` [Qemu PATCH v2 3/9] include/hw/cxl/cxl_device: Rename mem_size as static_mem_size for type3 memory devices Fan Ni
2023-08-04 15:27       ` Jonathan Cameron via
     [not found]   ` <CGME20230725183956uscas1p2008fba59779b70405c74d28a30e4fbaa@uscas1p2.samsung.com>
2023-07-25 18:39     ` [Qemu PATCH v2 4/9] hw/mem/cxl_type3: Add support to create DC regions to " Fan Ni
2023-08-04 15:55       ` Jonathan Cameron via
     [not found]   ` <CGME20230725183957uscas1p1ebf676c30d21896d1fd7f9b652250449@uscas1p1.samsung.com>
2023-07-25 18:39     ` [Qemu PATCH v2 8/9] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents Fan Ni
2023-08-07 10:35       ` Jonathan Cameron via
     [not found]   ` <CGME20230725183957uscas1p2ca5293c7229ab989ad1a2d95395436a6@uscas1p2.samsung.com>
2023-07-25 18:39     ` [Qemu PATCH v2 9/9] hw/mem/cxl_type3: Add dpa range validation for accesses to dc regions Fan Ni
2023-08-07  8:53       ` Jonathan Cameron via [this message]
2023-08-07  9:37         ` Jonathan Cameron via
2023-08-24 20:49         ` Fan Ni
2023-08-25 11:42           ` Jonathan Cameron via
2023-08-25 16:34             ` Fan Ni
2023-08-30 15:04               ` Jonathan Cameron via
2023-08-30 12:08       ` Jørgen Hansen
2023-08-30 15:37         ` Jonathan Cameron via
     [not found]   ` <CGME20230725183957uscas1p1eeb8e8eccc6c00b460d183027642374b@uscas1p1.samsung.com>
2023-07-25 18:39     ` [Qemu PATCH v2 5/9] hw/mem/cxl_type3: Add host backend and address space handling for DC regions Fan Ni
2023-07-26 12:53       ` Nathan Fontenot
2023-07-26 16:17         ` nifan
2023-08-04 16:36       ` Jonathan Cameron via
2023-08-04 18:07         ` Gregory Price
2023-08-07 12:10           ` Jonathan Cameron via
     [not found]   ` <CGME20230725183957uscas1p28b38d294f90b97f99769466cc533b4de@uscas1p2.samsung.com>
2023-07-25 18:39     ` [Qemu PATCH v2 6/9] hw/mem/cxl_type3: Add DC extent list representative and get DC extent list mailbox support Fan Ni
2023-08-07 11:55       ` Jonathan Cameron via
2023-09-08 13:12       ` Jørgen Hansen
2023-09-08 17:12         ` Fan Ni
     [not found]   ` <CGME20230725183957uscas1p2a076b6f7b694d2e632a0b8025ec331d7@uscas1p2.samsung.com>
2023-07-25 18:39     ` [Qemu PATCH v2 7/9] hw/cxl/cxl-mailbox-utils: Add mailbox commands to support add/release dynamic capacity response Fan Ni
2023-08-07 11:42       ` Jonathan Cameron via
2023-09-08 13:00       ` Jørgen Hansen
2023-09-08 17:19         ` Fan Ni

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230807095342.00006f88@Huawei.com \
    --to=qemu-devel@nongnu.org \
    --cc=Jonathan.Cameron@Huawei.com \
    --cc=a.manzanares@samsung.com \
    --cc=cbrowy@avery-design.com \
    --cc=dan.j.williams@intel.com \
    --cc=dave@stgolabs.net \
    --cc=fan.ni@samsung.com \
    --cc=gregory.price@memverge.com \
    --cc=hchkuo@avery-design.com.tw \
    --cc=ira.weiny@intel.com \
    --cc=linux-cxl@vger.kernel.org \
    --cc=nifan@outlook.com \
    --cc=nmtadam.samsung@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).