Re: [PATCH v18 01/14] hw/pci: Rename has_power to enabled
On 4/1/25 08:52, Akihiko Odaki wrote: The renamed state will not only represent powering state of PFs, but also represent SR-IOV VF enablement in the future. Signed-off-by: Akihiko Odaki --- include/hw/pci/pci.h| 7 ++- include/hw/pci/pci_device.h | 2 +- hw/pci/pci.c| 14 +++--- hw/pci/pci_host.c | 4 ++-- 4 files changed, 16 insertions(+), 11 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v18 13/14] hw/pci: Use -1 as the default value for rombar
On 4/1/25 08:52, Akihiko Odaki wrote: vfio_pci_size_rom() distinguishes whether rombar is explicitly set to 1 by checking dev->opts, bypassing the QOM property infrastructure. Use -1 as the default value for rombar to tell if the user explicitly set it to 1. The property is also converted from unsigned to signed. -1 is signed so it is safe to give it a new meaning. The values in [2 ^ 31, 2 ^ 32) become invalid, but nobody should have typed these values by chance. Suggested-by: Markus Armbruster Signed-off-by: Akihiko Odaki Reviewed-by: Markus Armbruster --- include/hw/pci/pci_device.h | 2 +- hw/pci/pci.c| 2 +- hw/vfio/pci.c | 5 ++--- 3 files changed, 4 insertions(+), 5 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
[PATCH v6 1/2] memory: Update inline documentation
Do not refer to "memory region's reference count" - Now MemoryRegions do have their own reference counts, but they will not be used when their owners are not themselves. However, the documentation of memory_region_ref() says it adds "1 to a memory region's reference count", which is confusing. Avoid referring to "memory region's reference count" and just say: "Add a reference to a memory region". Make a similar change to memory_region_unref() too. Refer to docs/devel/memory.rst for "owner" -- memory_region_ref() and memory_region_unref() used to have their own descriptions of "owner", but they are somewhat out-of-date and misleading. In particular, they say "whenever memory regions are accessed outside the BQL, they need to be preserved against hot-unplug", but protecting against hot-unplug is not mandatory if it is known that they will never be hot-unplugged. They also say "MemoryRegions actually do not have their own reference count", but they actually do. They just will not be used unless their owners are not themselves. Refer to docs/devel/memory.rst as the single source of truth instead of maintaining duplicate descriptions of "owner". Clarify that owner may be missing - A memory region may not have an owner, and memory_region_ref() and memory_region_unref() do nothing for such. memory: Clarify owner must not call memory_region_ref() The owner must not call this function as it results in a circular reference. Signed-off-by: Akihiko Odaki --- include/exec/memory.h | 59 --- 1 file changed, 28 insertions(+), 31 deletions(-) diff --git a/include/exec/memory.h b/include/exec/memory.h index 9458e2801d50..ca247343f433 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -1210,7 +1210,7 @@ void memory_region_section_free_copy(MemoryRegionSection *s); * memory_region_add_subregion() to add subregions. * * @mr: the #MemoryRegion to be initialized - * @owner: the object that tracks the region's reference count + * @owner: the object that keeps the region alive * @name: used for debugging; not visible to the user or ABI * @size: size of the region; any subregions beyond this size will be clipped */ @@ -1220,29 +1220,26 @@ void memory_region_init(MemoryRegion *mr, uint64_t size); /** - * memory_region_ref: Add 1 to a memory region's reference count + * memory_region_ref: Add a reference to the owner of a memory region * - * Whenever memory regions are accessed outside the BQL, they need to be - * preserved against hot-unplug. MemoryRegions actually do not have their - * own reference count; they piggyback on a QOM object, their "owner". - * This function adds a reference to the owner. - * - * All MemoryRegions must have an owner if they can disappear, even if the - * device they belong to operates exclusively under the BQL. This is because - * the region could be returned at any time by memory_region_find, and this - * is usually under guest control. + * This function adds a reference to the owner of a memory region to keep the + * memory region alive. It does nothing if the owner is not present as a memory + * region without owner will never die. + * For references internal to the owner, use object_ref() instead to avoid a + * circular reference. + * See docs/devel/memory.rst to know about owner. * * @mr: the #MemoryRegion */ void memory_region_ref(MemoryRegion *mr); /** - * memory_region_unref: Remove 1 to a memory region's reference count + * memory_region_unref: Remove a reference to the memory region of the owner * - * Whenever memory regions are accessed outside the BQL, they need to be - * preserved against hot-unplug. MemoryRegions actually do not have their - * own reference count; they piggyback on a QOM object, their "owner". - * This function removes a reference to the owner and possibly destroys it. + * This function removes a reference to the owner of a memory region and + * possibly destroys the owner along with the memory region. It does nothing if + * the owner is not present. + * See docs/devel/memory.rst to know about owner. * * @mr: the #MemoryRegion */ @@ -1255,7 +1252,7 @@ void memory_region_unref(MemoryRegion *mr); * if @size is nonzero, subregions will be clipped to @size. * * @mr: the #MemoryRegion to be initialized. - * @owner: the object that tracks the region's reference count + * @owner: the object that keeps the region alive * @ops: a structure containing read and write callbacks to be used when * I/O is performed on the region. * @opaque: passed to the read and write callbacks of the @ops structure. @@ -1275,7 +1272,7 @@ void memory_region_init_io(MemoryRegion *mr, *directly. * * @mr: the #MemoryRegion to be initia
[PATCH v6 0/2] Fix check-qtest-ppc64 sanitizer errors
I saw various sanitizer errors when running check-qtest-ppc64. While
I could just turn off sanitizers, I decided to tackle them this time.
Unfortunately, GLib versions older than 2.81.0 do not free test data in
some cases so some sanitizer errors remain. All sanitizer errors will be
gone with this patch series combined with the following change for GLib:
https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4120
Signed-off-by: Akihiko Odaki
---
Changes in v6:
- Avoid referring owner as "the object that tracks the region's
reference count".
- Noted that memroy_region_ref() and memroy_region_unref() do nothing
if the owner is not present.
- Explicitly stated that memory_region_unref() may destroy the owner
along with the memory region itself.
- Link to v5:
https://lore.kernel.org/r/20250104-san-v5-0-8b430457b...@daynix.com
Changes in v5:
- Rebased.
- Merged four patches to update inline documentation into one
- Link to v4:
https://lore.kernel.org/r/20240823-san-v4-0-a24c6dfa4...@daynix.com
Changes in v4:
- Changed to create a reference to the subregion instead of its owner
when its owner equals to the container's owner.
- Dropped R-b from patch "memory: Do not create circular reference with
subregion".
- Rebased.
- Link to v3:
https://lore.kernel.org/r/20240708-san-v3-0-b03f671c4...@daynix.com
Changes in v3:
- Added patch "memory: Clarify that we use owner's reference count".
- Added patch "memory: Refer to docs/devel/memory.rst for 'owner'".
- Fixed the message of patch
"memory: Do not create circular reference with subregion".
- Dropped patch "cpu: Free cpu_ases" in favor of:
https://lore.kernel.org/r/20240607115649.214622-7-salil.me...@huawei.com/
("[PATCH V13 6/8] physmem: Add helper function to destroy CPU
AddressSpace")
- Dropped patches "hw/ide: Convert macio ide_irq into GPIO line" and
"hw/ide: Remove internal DMA qemu_irq" in favor of commit efb359346c7a
("hw/ide/macio: switch from using qemu_allocate_irq() to qdev input
GPIOs")
- Dropped patch "hw/isa/vt82c686: Define a GPIO line between vt82c686
and i8259" in favor of:
https://patchew.org/QEMU/20240704205854.18537-1-shen...@gmail.com/
("[PATCH 0/3] Resolve vt82c686 and piix4 qemu_irq memory leaks")
- Dropped pulled patches.
- Link to v2:
https://lore.kernel.org/r/20240627-san-v2-0-750bb0946...@daynix.com
Changes in v2:
- Rebased to "[PATCH] cpu: fix memleak of 'halt_cond' and 'thread'".
(Philippe Mathieu-Daudé)
- Converted IRQs into GPIO lines and removed one qemu_irq usage.
(Peter Maydell)
- s/suppresses/fixes/ (Michael S. Tsirkin)
- Corrected title of patch "hw/virtio: Free vqs after vhost_dev_cleanup()"
(was "hw/virtio: Free vqs before vhost_dev_cleanup()")
- Link to v1:
https://lore.kernel.org/r/20240626-san-v1-0-f3cc42302...@daynix.com
---
Akihiko Odaki (2):
memory: Update inline documentation
memory: Do not create circular reference with subregion
include/exec/memory.h | 59 ---
system/memory.c | 8 +--
2 files changed, 34 insertions(+), 33 deletions(-)
---
base-commit: 38d0939b86e2eef6f6a622c6f1f7befda0146595
change-id: 20240625-san-097afaf4f1c2
Best regards,
--
Akihiko Odaki
[PATCH v6 2/2] memory: Do not create circular reference with subregion
memory_region_update_container_subregions() used to call
memory_region_ref(), which creates a reference to the owner of the
subregion, on behalf of the owner of the container. This results in a
circular reference if the subregion and container have the same owner.
memory_region_ref() creates a reference to the owner instead of the
memory region to match the lifetime of the owner and memory region. We
do not need such a hack if the subregion and container have the same
owner because the owner will be alive as long as the container is.
Therefore, create a reference to the subregion itself instead ot its
owner in such a case; the reference to the subregion is still necessary
to ensure that the subregion gets finalized after the container.
Signed-off-by: Akihiko Odaki
---
system/memory.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/system/memory.c b/system/memory.c
index 78e17e0efa83..16b051249c5f 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -2644,7 +2644,9 @@ static void
memory_region_update_container_subregions(MemoryRegion *subregion)
memory_region_transaction_begin();
-memory_region_ref(subregion);
+object_ref(mr->owner == subregion->owner ?
+ OBJECT(subregion) : subregion->owner);
+
QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
if (subregion->priority >= other->priority) {
QTAILQ_INSERT_BEFORE(other, subregion, subregions_link);
@@ -2702,7 +2704,9 @@ void memory_region_del_subregion(MemoryRegion *mr,
assert(alias->mapped_via_alias >= 0);
}
QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
-memory_region_unref(subregion);
+object_unref(mr->owner == subregion->owner ?
+ OBJECT(subregion) : subregion->owner);
+
memory_region_update_pending |= mr->enabled && subregion->enabled;
memory_region_transaction_commit();
}
--
2.47.1
Re: [PATCH v5 1/2] memory: Update inline documentation
On 2025/01/04 21:51, BALATON Zoltan wrote: On Sat, 4 Jan 2025, Akihiko Odaki wrote: Do not refer to "memory region's reference count" - Now MemoryRegions do have their own reference counts, but they will not be used when their owners are not themselves. However, the documentation of memory_region_ref() says it adds "1 to a memory region's reference count", which is confusing. Avoid referring to "memory region's reference count" and just say: "Add a reference to a memory region". Make a similar change to memory_region_unref() too. Refer to docs/devel/memory.rst for "owner" -- memory_region_ref() and memory_region_unref() used to have their own descriptions of "owner", but they are somewhat out-of-date and misleading. In particular, they say "whenever memory regions are accessed outside the BQL, they need to be preserved against hot-unplug", but protecting against hot-unplug is not mandatory if it is known that they will never be hot-unplugged. They also say "MemoryRegions actually do not have their own reference count", but they actually do. They just will not be used unless their owners are not themselves. Refer to docs/devel/memory.rst as the single source of truth instead of maintaining duplicate descriptions of "owner". Clarify that owner may be missing - A memory region may not have an owner, and memory_region_ref() and memory_region_unref() do nothing for such. The commit message is longer than the documentation it changes :-) That probably means the docs could be more detailed. For example this relation to the owner may be mentioned unless it's something to be changed in the future to clean this up. It's odd that the commit message is longer but there are a few reasons: 1. It describes why the old documentation is wrong. Some of such statements are replaced with the reference to docs/devel/memory.rst to make them shorter. 2. It contains information that is already described in the existing documentation for context. In this particular case, the owner is already described in docs/devel/memory.rst extensively. memory: Clarify owner must not call memory_region_ref() The owner must not call this function as it results in a circular reference. Signed-off-by: Akihiko Odaki --- include/exec/memory.h | 22 +++--- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/include/exec/memory.h b/include/exec/memory.h index 9458e2801d50..cd91fe0c51cf 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -1220,29 +1220,21 @@ void memory_region_init(MemoryRegion *mr, uint64_t size); /** - * memory_region_ref: Add 1 to a memory region's reference count + * memory_region_ref: Add a reference to a memory region * - * Whenever memory regions are accessed outside the BQL, they need to be - * preserved against hot-unplug. MemoryRegions actually do not have their - * own reference count; they piggyback on a QOM object, their "owner". - * This function adds a reference to the owner. - * - * All MemoryRegions must have an owner if they can disappear, even if the - * device they belong to operates exclusively under the BQL. This is because - * the region could be returned at any time by memory_region_find, and this - * is usually under guest control. + * This function adds a reference to the owner if present. Maybe it's just not clear to me but the title says "Add a reference to a memory region" then here it says "adds a reference to the owner" and does not say what happens if there's no owner present. Maybe it's better to be explicit and say add 1 to the owner's ref count or do nothing if owner is not present. I wrote the title looking at other comments that describes the owner as "the object that tracks the region's reference count", which implies the region's reference count is part of the owner. But it is confusing to to describe the owner as "the object that tracks the region's reference count" in the first place. A memory region has its own reference count that can be used to express references internal to the owner (and actually used in the following patch). With v6 I have just sent, I changed to describe the owner as "the object that keeps the region alive". The title of this function now says "add a reference to the owner of a memory region". I replaced "add 1 to ~'s reference count" with "add a reference to" to make in concise (to fit into 80 columns in particular). I think it's fine not to mention the use of counter as it is an implementation detail users don't have to care. With v6, I also noted that this function does nothing when the owner is not present in the comment body as suggested. + * The owner must not call this function as it results in a circular reference. + * See docs/devel/memory.rst to know a
[PATCH] vvfat: fix out of bounds array write
In function create_long_filname(), the array name[8 + 3] in
struct direntry_t is used as if it were defined as name[32].
This is intentional and works. It's nevertheless an out of
bounds array access. To avoid this problem, this patch adds a
struct lfn_direntry_t with multiple name arrays. A directory
entry for a long FAT file name is significantly different from
a directory entry for a regular FAT file name.
Signed-off-by: Volker Rümelin
---
block/vvfat.c | 55 ++-
1 file changed, 41 insertions(+), 14 deletions(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index 8ffe8b3b9b..626c4f0163 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -255,6 +255,17 @@ typedef struct direntry_t {
uint32_t size;
} QEMU_PACKED direntry_t;
+typedef struct lfn_direntry_t {
+uint8_t sequence;
+uint8_t name01[10];
+uint8_t attributes;
+uint8_t direntry_type;
+uint8_t sfn_checksum;
+uint8_t name0e[12];
+uint16_t begin;
+uint8_t name1c[4];
+} QEMU_PACKED lfn_direntry_t;
+
/* this structure are used to transparently access the files */
typedef struct mapping_t {
@@ -399,11 +410,28 @@ static void init_mbr(BDRVVVFATState *s, int cyls, int
heads, int secs)
/* direntry functions */
+static void write_long_filename(lfn_direntry_t *entry, int index, uint8_t c)
+{
+if (index < ARRAY_SIZE(entry->name01)) {
+entry->name01[index] = c;
+return;
+}
+index -= ARRAY_SIZE(entry->name01);
+if (index < ARRAY_SIZE(entry->name0e)) {
+entry->name0e[index] = c;
+return;
+}
+index -= ARRAY_SIZE(entry->name0e);
+if (index < ARRAY_SIZE(entry->name1c)) {
+entry->name1c[index] = c;
+}
+}
+
static direntry_t *create_long_filename(BDRVVVFATState *s, const char
*filename)
{
int number_of_entries, i;
glong length;
-direntry_t *entry;
+lfn_direntry_t *entry;
gunichar2 *longname = g_utf8_to_utf16(filename, -1, NULL, &length, NULL);
if (!longname) {
@@ -415,24 +443,23 @@ static direntry_t *create_long_filename(BDRVVVFATState
*s, const char *filename)
for(i=0;idirectory));
+entry->sequence = (number_of_entries - i) | (i == 0 ? 0x40 : 0);
entry->attributes=0xf;
-entry->reserved[0]=0;
+entry->direntry_type = 0;
entry->begin=0;
-entry->name[0]=(number_of_entries-i)|(i==0?0x40:0);
-}
-for(i=0;i<26*number_of_entries;i++) {
-int offset=(i%26);
-if(offset<10) offset=1+offset;
-else if(offset<22) offset=14+offset-10;
-else offset=28+offset-22;
-entry=array_get(&(s->directory),s->directory.next-1-(i/26));
+}
+for (i = 0; i < 26 * number_of_entries; i++) {
+uint8_t c;
+
+entry = array_get(&s->directory, s->directory.next - i / 26 - 1);
if (i >= 2 * length + 2) {
-entry->name[offset] = 0xff;
+c = 0xff;
} else if (i % 2 == 0) {
-entry->name[offset] = longname[i / 2] & 0xff;
+c = longname[i / 2] & 0xff;
} else {
-entry->name[offset] = longname[i / 2] >> 8;
+c = longname[i / 2] >> 8;
}
+write_long_filename(entry, i % 26, c);
}
g_free(longname);
return array_get(&(s->directory),s->directory.next-number_of_entries);
@@ -731,7 +758,7 @@ static inline direntry_t*
create_short_and_long_name(BDRVVVFATState* s,
/* calculate anew, because realloc could have taken place */
entry_long=array_get(&(s->directory),long_index);
while(entry_longreserved[1]=chksum;
+((lfn_direntry_t *)entry_long)->sfn_checksum = chksum;
entry_long++;
}
}
--
2.43.0
