All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: "Jørgen Hansen" <Jorgen.Hansen@wdc.com>
Cc: Fan Ni <fan.ni@samsung.com>,
	"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: Wed, 30 Aug 2023 16:37:20 +0100	[thread overview]
Message-ID: <20230830163720.00000f9c@Huawei.com> (raw)
In-Reply-To: <d0d7ca1d-81bc-19b3-4904-d60046ded844@wdc.com>


> > +
> > +/*
> > + * 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;
oops. Len is probably always smaller than block_size (typically 8 or less)
so nbits always 0.  Should be 1 in those cases.

> > +    return find_next_zero_bit(region->blk_bitmap, nbits, nr) >= nr + nbits;  
> 
> The 2nd parameter to find_next_zero_bit is the length of the bitmap, so 
> shouldn't this be something like (also considering that len is the 
> read/write len, and will be smaller than the region block size):
> 
>    nbits = DIV_ROUND_UP(len, region->block_size);

>  
> 
>    return find_next_zero_bit(region->blk_bitmap, nbits + nr, nr) ==
>           nbits + nr;

Agreed with your suggestion. I'll carry that in my forward port of this
series for now and update my tree at
gitlab.com/jic23/qemu branch will probably be cxl-2023-08-30
a bit later today.

Jonathan


WARNING: multiple messages have this Message-ID (diff)
From: Jonathan Cameron via <qemu-devel@nongnu.org>
To: "Jørgen Hansen" <Jorgen.Hansen@wdc.com>
Cc: Fan Ni <fan.ni@samsung.com>,
	"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: Wed, 30 Aug 2023 16:37:20 +0100	[thread overview]
Message-ID: <20230830163720.00000f9c@Huawei.com> (raw)
In-Reply-To: <d0d7ca1d-81bc-19b3-4904-d60046ded844@wdc.com>


> > +
> > +/*
> > + * 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;
oops. Len is probably always smaller than block_size (typically 8 or less)
so nbits always 0.  Should be 1 in those cases.

> > +    return find_next_zero_bit(region->blk_bitmap, nbits, nr) >= nr + nbits;  
> 
> The 2nd parameter to find_next_zero_bit is the length of the bitmap, so 
> shouldn't this be something like (also considering that len is the 
> read/write len, and will be smaller than the region block size):
> 
>    nbits = DIV_ROUND_UP(len, region->block_size);

>  
> 
>    return find_next_zero_bit(region->blk_bitmap, nbits + nr, nr) ==
>           nbits + nr;

Agreed with your suggestion. I'll carry that in my forward port of this
series for now and update my tree at
gitlab.com/jic23/qemu branch will probably be cxl-2023-08-30
a bit later today.

Jonathan



  reply	other threads:[~2023-08-30 18:54 UTC|newest]

Thread overview: 48+ 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
2023-07-25 18:39   ` [Qemu PATCH v2 4/9] hw/mem/cxl_type3: Add support to create DC regions to type3 memory devices Fan Ni
2023-08-04 15:55     ` Jonathan Cameron
2023-08-04 15:55       ` Jonathan Cameron via
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
2023-08-04 14:19       ` Jonathan Cameron via
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
2023-08-04 15:24       ` Jonathan Cameron via
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
2023-08-04 15:27       ` Jonathan Cameron via
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
2023-08-04 16:36       ` Jonathan Cameron via
2023-08-04 18:07       ` Gregory Price
2023-08-07 12:10         ` Jonathan Cameron
2023-08-07 12:10           ` Jonathan Cameron via
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
2023-08-07 11:55       ` Jonathan Cameron via
2023-09-08 13:12     ` Jørgen Hansen
2023-09-08 17:12       ` Fan Ni
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
2023-08-07 10:35       ` Jonathan Cameron via
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
2023-08-07 11:42       ` Jonathan Cameron via
2023-09-08 13:00     ` Jørgen Hansen
2023-09-08 17:19       ` Fan Ni
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
2023-08-07  8:53       ` Jonathan Cameron via
2023-08-07  9:37       ` Jonathan Cameron
2023-08-07  9:37         ` Jonathan Cameron via
2023-08-24 20:49       ` Fan Ni
2023-08-25 11:42         ` Jonathan Cameron
2023-08-25 11:42           ` Jonathan Cameron via
2023-08-25 16:34           ` Fan Ni
2023-08-30 15:04             ` Jonathan Cameron
2023-08-30 15:04               ` Jonathan Cameron via
2023-08-30 12:08     ` Jørgen Hansen
2023-08-30 15:37       ` Jonathan Cameron [this message]
2023-08-30 15:37         ` Jonathan Cameron via

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=20230830163720.00000f9c@Huawei.com \
    --to=jonathan.cameron@huawei.com \
    --cc=Jorgen.Hansen@wdc.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 \
    --cc=qemu-devel@nongnu.org \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.