RE: [PATCH v2 00/48] Use g_assert_not_reached instead of (g_)assert(0, false)
> >> -- > >> 2.39.2 > >> > > > > Which one did you find? Using which grep command? Sorry, I made a mistake. I haven't found anything new yet.
RE: [PATCH v4] pci-bridge: avoid linking a single downstream port more than once
> -Original Message-
> From: Michael S. Tsirkin
> Sent: Tuesday, September 10, 2024 11:17 PM
> To: Yao, Xingtao/姚 幸涛
> Cc: marcel.apfelb...@gmail.com; qemu-devel@nongnu.org
> Subject: Re: [PATCH v4] pci-bridge: avoid linking a single downstream port
> more
> than once
>
> On Thu, Jul 25, 2024 at 05:38:19AM -0400, Yao Xingtao wrote:
> > Since the downstream port is not checked, two slots can be linked to
> > a single port. However, this can prevent the driver from detecting the
> > device properly.
> >
> > It is necessary to ensure that a downstream port is not linked more than
> > once.
> >
> > Links:
> https://lore.kernel.org/qemu-devel/OSZPR01MB6453BC61D2FF4035F18084EF8D
> d...@oszpr01mb6453.jpnprd01.prod.outlook.com
> > Signed-off-by: Yao Xingtao
> >
> > ---
> > V3[3] -> V4:
> > - make the error message more readable
> > - fix the downstream port check error
> >
> > V2[2] -> V3:
> > - Move this check into pcie_cap_init()
> >
> > V1[1] -> V2:
> > - Move downstream port check forward
> >
> > [1]
> https://lore.kernel.org/qemu-devel/20240704033834.3362-1-yaoxt.f...@fujitsu.co
> m
> > [2]
> https://lore.kernel.org/qemu-devel/20240717085621.55315-1-yaoxt.fnst@fujitsu.c
> om
> > [3]
> https://lore.kernel.org/qemu-devel/20240725032731.13032-1-yaoxt.fnst@fujitsu.c
> om
> > ---
> > hw/pci/pcie.c | 7 +++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 4b2f0805c6e0..1e53be1bc7c5 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -192,6 +192,13 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset,
> >
> > assert(pci_is_express(dev));
> >
> > +if ((type == PCI_EXP_TYPE_DOWNSTREAM || type ==
> PCI_EXP_TYPE_ROOT_PORT) &&
> > +pcie_find_port_by_pn(pci_get_bus(dev), port)) {
> > +error_setg(errp, "The port %d is already in use, please select "
> > + "another port", port);
> > +return -EBUSY;
> > +}
> > +
> > pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
> > PCI_EXP_VER2_SIZEOF, errp);
> > if (pos < 0) {
>
>
> But can't there be two functions of a multi-function device,
> sharing a port?
Good question.
But I am not good at PCIe protocol, can anyone give me some advice when using
the
mulit-function feature?
>
> > --
> > 2.41.0
RE: [PATCH 1/1] platform-bus: fix refcount leak
> -Original Message-
> From: qemu-devel-bounces+yaoxt.fnst=fujitsu@nongnu.org
> On Behalf Of Gao
> Shiyuan via
> Sent: Thursday, August 29, 2024 9:10 PM
> To: Paolo Bonzini
> Cc: qemu-devel@nongnu.org; gaoshiy...@baidu.com
> Subject: [PATCH 1/1] platform-bus: fix refcount leak
>
> Temporary object causes reference count leakage.
>
> Signed-off-by: Gao Shiyuan
> ---
> hw/core/platform-bus.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/platform-bus.c b/hw/core/platform-bus.c
> index b8487b26b6..dc58bf505a 100644
> --- a/hw/core/platform-bus.c
> +++ b/hw/core/platform-bus.c
> @@ -145,9 +145,12 @@ static void platform_bus_map_mmio(PlatformBusDevice
> *pbus, SysBusDevice *sbdev,
> * the target device's memory region
> */
> for (off = 0; off < pbus->mmio_size; off += alignment) {
> -if (!memory_region_find(&pbus->mmio, off, size).mr) {
> +MemoryRegion *mr = memory_region_find(&pbus->mmio, off, size).mr;
> +if (!mr) {
> found_region = true;
> break;
> +} else {
> +memory_region_unref(mr);
> }
LGTM, but if the empty region is not found, the process will stop running, so I
think this bug may be not
serious.
> }
>
> --
> 2.39.3 (Apple Git-146)
>
RE: [PATCH] scripts/coccinelle: New range.cocci
> -Original Message- > From: Peter Maydell > Sent: Thursday, August 22, 2024 11:31 PM > To: Yao, Xingtao/姚 幸涛 > Cc: qemu-devel@nongnu.org > Subject: Re: [PATCH] scripts/coccinelle: New range.cocci > > On Wed, 21 Aug 2024 at 01:21, Xingtao Yao (Fujitsu) > wrote: > > > > > > > > > -Original Message- > > > From: Peter Maydell > > > Sent: Tuesday, August 20, 2024 4:41 PM > > > To: Yao, Xingtao/姚 幸涛 > > > Cc: qemu-devel@nongnu.org > > > Subject: Re: [PATCH] scripts/coccinelle: New range.cocci > > > > > > On Thu, 25 Jul 2024 at 06:55, Yao Xingtao via > wrote: > > > > > > > > This is the semantic patch from commit 7b3e371526 "cxl/mailbox: make > > > > range overlap check more readable" > > > > > > > > Signed-off-by: Yao Xingtao > > > > --- > > > > scripts/coccinelle/range.cocci | 49 > > > ++ > > > > 1 file changed, 49 insertions(+) > > > > create mode 100644 scripts/coccinelle/range.cocci > > > > > > > > diff --git a/scripts/coccinelle/range.cocci > > > > b/scripts/coccinelle/range.cocci > > > > new file mode 100644 > > > > index ..21b07945ccb2 > > > > --- /dev/null > > > > +++ b/scripts/coccinelle/range.cocci > > > > @@ -0,0 +1,49 @@ > > > > +/* > > > > + Usage: > > > > + > > > > +spatch \ > > > > + --macro-file scripts/cocci-macro-file.h \ > > > > + --sp-file scripts/coccinelle/range.cocci \ > > > > + --keep-comments \ > > > > + --in-place \ > > > > + --dir . > > > > + > > > > + Description: > > > > +Find out the range overlap check and use ranges_overlap() instead. > > > > + > > > > + Note: > > > > +This pattern cannot accurately match the region overlap check, and > > > > you > > > > +need to manually delete the use cases that do not meet the > > > > conditions. > > > > + > > > > +In addition, the parameters of ranges_overlap() may be filled > incorrectly, > > > > +and some use cases may be better to use range_overlaps_range(). > > > > > > I think these restrictions mean that we should do one > > > of two things: > > > (1) rewrite this as a Coccinelle script that just prints > > > out the places where it found matches (i.e. a "grep" > > > type operation, not a search-and-replace), so the > > > user can then go and investigate them and do the > > > range_overlap they want to do > > > (2) fix the problems so that you really can apply it to > > > the source tree and get correct fixes > > > > > > The ideal would be (2) -- the ideal flow for coccinelle > > > driven patches is that you can review the coccinelle > > > script to check for things like off-by-one errors, and > > > then can trust all the changes it makes. Otherwise > > > reviewers need to carefully scrutinize each source > > > change individually. > > > > > > It's the off-by-one issue that really makes me think > > > that (2) would be preferable -- it would otherwise > > > be I think quite easy to accidentally rewrite a correct > > > range check into one that's off-by-one and not notice. > > thanks for your reply! > > I tried my best to match all the patterns of the region overlap check, > > but it is difficult, I am not good at cocci, could you give me some advice? > > > Something like this seems to me to work and mostly makes the > same substitutions as your series suggests: > > ===begin=== > // Usage: > //spatch \ > // --macro-file scripts/cocci-macro-file.h \ > // --sp-file scripts/coccinelle/range.cocci \ > // --keep-comments \ > // --in-place \ > // --dir . > // > // Description: > // Find out the range overlap check and use ranges_overlap() instead. > // > // Usage notes: > // * Any change made here shouldn't be a behaviour change, but you'll > // need to check that the suggested changes really are range checks > // semantically, and not something else that happened to match the pattern. > // In particular the patterns for (start1, end1) vs (start2, end2) > // can match very widely. > // * This won'
RE: [PATCH] scripts/coccinelle: New range.cocci
> -Original Message- > From: Peter Maydell > Sent: Tuesday, August 20, 2024 4:41 PM > To: Yao, Xingtao/姚 幸涛 > Cc: qemu-devel@nongnu.org > Subject: Re: [PATCH] scripts/coccinelle: New range.cocci > > On Thu, 25 Jul 2024 at 06:55, Yao Xingtao via wrote: > > > > This is the semantic patch from commit 7b3e371526 "cxl/mailbox: make > > range overlap check more readable" > > > > Signed-off-by: Yao Xingtao > > --- > > scripts/coccinelle/range.cocci | 49 > ++ > > 1 file changed, 49 insertions(+) > > create mode 100644 scripts/coccinelle/range.cocci > > > > diff --git a/scripts/coccinelle/range.cocci b/scripts/coccinelle/range.cocci > > new file mode 100644 > > index ..21b07945ccb2 > > --- /dev/null > > +++ b/scripts/coccinelle/range.cocci > > @@ -0,0 +1,49 @@ > > +/* > > + Usage: > > + > > +spatch \ > > + --macro-file scripts/cocci-macro-file.h \ > > + --sp-file scripts/coccinelle/range.cocci \ > > + --keep-comments \ > > + --in-place \ > > + --dir . > > + > > + Description: > > +Find out the range overlap check and use ranges_overlap() instead. > > + > > + Note: > > +This pattern cannot accurately match the region overlap check, and you > > +need to manually delete the use cases that do not meet the conditions. > > + > > +In addition, the parameters of ranges_overlap() may be filled > > incorrectly, > > +and some use cases may be better to use range_overlaps_range(). > > I think these restrictions mean that we should do one > of two things: > (1) rewrite this as a Coccinelle script that just prints > out the places where it found matches (i.e. a "grep" > type operation, not a search-and-replace), so the > user can then go and investigate them and do the > range_overlap they want to do > (2) fix the problems so that you really can apply it to > the source tree and get correct fixes > > The ideal would be (2) -- the ideal flow for coccinelle > driven patches is that you can review the coccinelle > script to check for things like off-by-one errors, and > then can trust all the changes it makes. Otherwise > reviewers need to carefully scrutinize each source > change individually. > > It's the off-by-one issue that really makes me think > that (2) would be preferable -- it would otherwise > be I think quite easy to accidentally rewrite a correct > range check into one that's off-by-one and not notice. thanks for your reply! I tried my best to match all the patterns of the region overlap check, but it is difficult, I am not good at cocci, could you give me some advice? > > thanks > -- PMM
RE: [PATCH v4] pci-bridge: avoid linking a single downstream port more than once
ping.
> -Original Message-
> From: Yao Xingtao
> Sent: Thursday, July 25, 2024 5:38 PM
> To: m...@redhat.com; marcel.apfelb...@gmail.com
> Cc: qemu-devel@nongnu.org; Yao, Xingtao/姚 幸涛
> Subject: [PATCH v4] pci-bridge: avoid linking a single downstream port more
> than
> once
>
> Since the downstream port is not checked, two slots can be linked to
> a single port. However, this can prevent the driver from detecting the
> device properly.
>
> It is necessary to ensure that a downstream port is not linked more than
> once.
>
> Links:
> https://lore.kernel.org/qemu-devel/OSZPR01MB6453BC61D2FF4035F18084EF8D
> d...@oszpr01mb6453.jpnprd01.prod.outlook.com
> Signed-off-by: Yao Xingtao
>
> ---
> V3[3] -> V4:
> - make the error message more readable
> - fix the downstream port check error
>
> V2[2] -> V3:
> - Move this check into pcie_cap_init()
>
> V1[1] -> V2:
> - Move downstream port check forward
>
> [1]
> https://lore.kernel.org/qemu-devel/20240704033834.3362-1-yaoxt.f...@fujitsu.co
> m
> [2]
> https://lore.kernel.org/qemu-devel/20240717085621.55315-1-yaoxt.fnst@fujitsu.c
> om
> [3]
> https://lore.kernel.org/qemu-devel/20240725032731.13032-1-yaoxt.fnst@fujitsu.c
> om
> ---
> hw/pci/pcie.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 4b2f0805c6e0..1e53be1bc7c5 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -192,6 +192,13 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset,
>
> assert(pci_is_express(dev));
>
> +if ((type == PCI_EXP_TYPE_DOWNSTREAM || type ==
> PCI_EXP_TYPE_ROOT_PORT) &&
> +pcie_find_port_by_pn(pci_get_bus(dev), port)) {
> +error_setg(errp, "The port %d is already in use, please select "
> + "another port", port);
> +return -EBUSY;
> +}
> +
> pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
> PCI_EXP_VER2_SIZEOF, errp);
> if (pos < 0) {
> --
> 2.41.0
RE: [PATCH 13/13] block/qcow2-cluster: make range overlap check more readable
ping.
> -Original Message-
> From: Yao Xingtao
> Sent: Monday, July 22, 2024 12:08 PM
> To: qemu-devel@nongnu.org; Kevin Wolf ; Hanna Reitz
>
> Cc: Yao, Xingtao/姚 幸涛 ; qemu-bl...@nongnu.org
> Subject: [PATCH 13/13] block/qcow2-cluster: make range overlap check more
> readable
>
> use range_overlaps_range() instead of open-coding the overlap check to
> improve the readability of the code.
>
> Signed-off-by: Yao Xingtao
> ---
> block/qcow2-cluster.c | 23 +--
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index ce8c0076b3b5..88d65c4b99e6 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -30,6 +30,7 @@
> #include "qcow2.h"
> #include "qemu/bswap.h"
> #include "qemu/memalign.h"
> +#include "qemu/range.h"
> #include "trace.h"
>
> int coroutine_fn qcow2_shrink_l1_table(BlockDriverState *bs,
> @@ -1408,23 +1409,25 @@ static int coroutine_fn
> handle_dependencies(BlockDriverState *bs,
> BDRVQcow2State *s = bs->opaque;
> QCowL2Meta *old_alloc;
> uint64_t bytes = *cur_bytes;
> +Range range1, range2, range3;
>
> +range_init_nofail(&range1, guest_offset, bytes);
> QLIST_FOREACH(old_alloc, &s->cluster_allocs, next_in_flight) {
>
> -uint64_t start = guest_offset;
> -uint64_t end = start + bytes;
> -uint64_t old_start = start_of_cluster(s,
> l2meta_cow_start(old_alloc));
> -uint64_t old_end = ROUND_UP(l2meta_cow_end(old_alloc),
> s->cluster_size);
> +uint64_t cow_start = l2meta_cow_start(old_alloc);
> +uint64_t cow_end = l2meta_cow_end(old_alloc);
> +uint64_t start = start_of_cluster(s, cow_start);
> +uint64_t end = ROUND_UP(cow_end, s->cluster_size);
>
> -if (end <= old_start || start >= old_end) {
> +range_init_nofail(&range2, start, end - start);
> +if (!range_overlaps_range(&range1, &range2)) {
> /* No intersection */
> continue;
> }
>
> +range_init_nofail(&range3, cow_start, cow_end - cow_start);
> if (old_alloc->keep_old_clusters &&
> -(end <= l2meta_cow_start(old_alloc) ||
> - start >= l2meta_cow_end(old_alloc)))
> -{
> +!range_overlaps_range(&range1, &range3)) {
> /*
> * Clusters intersect but COW areas don't. And cluster itself is
> * already allocated. So, there is no actual conflict.
> @@ -1434,9 +1437,9 @@ static int coroutine_fn
> handle_dependencies(BlockDriverState *bs,
>
> /* Conflict */
>
> -if (start < old_start) {
> +if (guest_offset < start) {
> /* Stop at the start of a running allocation */
> -bytes = old_start - start;
> +bytes = start - guest_offset;
> } else {
> bytes = 0;
> }
> --
> 2.41.0
RE: [PATCH] scripts/coccinelle: New range.cocci
ping. > -Original Message- > From: Yao Xingtao > Sent: Thursday, July 25, 2024 1:55 PM > To: qemu-devel@nongnu.org > Cc: Yao, Xingtao/姚 幸涛 > Subject: [PATCH] scripts/coccinelle: New range.cocci > > This is the semantic patch from commit 7b3e371526 "cxl/mailbox: make > range overlap check more readable" > > Signed-off-by: Yao Xingtao > --- > scripts/coccinelle/range.cocci | 49 > ++ > 1 file changed, 49 insertions(+) > create mode 100644 scripts/coccinelle/range.cocci > > diff --git a/scripts/coccinelle/range.cocci b/scripts/coccinelle/range.cocci > new file mode 100644 > index ..21b07945ccb2 > --- /dev/null > +++ b/scripts/coccinelle/range.cocci > @@ -0,0 +1,49 @@ > +/* > + Usage: > + > +spatch \ > + --macro-file scripts/cocci-macro-file.h \ > + --sp-file scripts/coccinelle/range.cocci \ > + --keep-comments \ > + --in-place \ > + --dir . > + > + Description: > +Find out the range overlap check and use ranges_overlap() instead. > + > + Note: > +This pattern cannot accurately match the region overlap check, and you > +need to manually delete the use cases that do not meet the conditions. > + > +In addition, the parameters of ranges_overlap() may be filled > incorrectly, > +and some use cases may be better to use range_overlaps_range(). > +*/ > + > +@@ > +expression E1, E2, E3, E4; > +@@ > +( > +- E2 <= E3 || E1 >= E4 > ++ !ranges_overlap(E1, E2, E3, E4) > +| > + > +- (E2 <= E3) || (E1 >= E4) > ++ !ranges_overlap(E1, E2, E3, E4) > +| > + > +- E1 < E4 && E2 > E3 > ++ ranges_overlap(E1, E2, E3, E4) > +| > + > +- (E1 < E4) && (E2 > E3) > ++ ranges_overlap(E1, E2, E3, E4) > +| > + > +- (E1 >= E3 && E1 < E4) || (E2 > E3 && E2 <= E4) > ++ ranges_overlap(E1, E2, E3, E4) > + > +| > +- ((E1 >= E3) && (E1 < E4)) || ((E2 > E3) && (E2 <= E4)) > ++ ranges_overlap(E1, E2, E3, E4) > +) > + > -- > 2.41.0
RE: [PATCH 00/13] make range overlap check more readable
> -Original Message-
> From: Peter Maydell
> Sent: Thursday, July 25, 2024 11:14 PM
> To: Yao, Xingtao/姚 幸涛
> Cc: Philippe Mathieu-Daudé ; qemu-devel@nongnu.org
> Subject: Re: [PATCH 00/13] make range overlap check more readable
>
> On Mon, 22 Jul 2024 at 08:00, Xingtao Yao (Fujitsu) via
> wrote:
> >
> >
> >
> > > -Original Message-
> > > From: Philippe Mathieu-Daudé
> > > Sent: Monday, July 22, 2024 2:43 PM
> > > To: Yao, Xingtao/姚 幸涛 ; qemu-devel@nongnu.org
> > > Subject: Re: [PATCH 00/13] make range overlap check more readable
> > >
> > > Hi Yao,
> > >
> > > On 22/7/24 06:07, Yao Xingtao via wrote:
> > > > Currently, some components still open-coding the range overlap check.
> > > > Sometimes this check may be fail because some patterns are missed.
> > >
> > > How did you catch all these use cases?
> > I used the Coccinelle to match these use cases, the pattern is below
> > range_overlap.cocci:
> >
> > // use ranges_overlap() instead of open-coding the overlap check
> > @@
> > expression E1, E2, E3, E4;
> > @@
> > (
> > - E2 <= E3 || E1 >= E4
> > + !ranges_overlap(E1, E2, E3, E4)
> > |
>
> Maybe I'm misunderstanding the coccinelle patch here, but
> I don't see how it produces the results in the patchset.
> ranges_overlap() takes arguments (start1, len1, start2, len2),
> but an expression like "E2 <= E3 || E1 >= E4" is working
> with start,end pairs to indicate the ranges. And looking
> at e.g. patch 9:
>
> - if (cur->phys_addr >= begin + length ||
> - cur->phys_addr + cur->length <= begin) {
> + if (!ranges_overlap(cur->phys_addr, cur->length, begin, length)) {
>
> the kind of if() check you get for start, length pairs
> has an addition in it, which I don't see in any of these
> coccinelle script fragments.
I understand your confusion, but it is difficult to match the region overlap
check because
it has many variations, like below:
case1:
start >= old_start +old_len || start + len <= old_start
case2:
start >= old_end || end <= old_start
case3:
cur->phys_addr >= begin + length || cur->phys_addr + cur->length <= begin
case4:
new->base >= old.base +old.size || new->base + new->size <= old.base
..
and sometimes the length or end may be also an expression, I can not find a way
to
handle all the variants.
So I decided to use the above pattern to find out the region overlap checks as
much as possible,
then manually drop the cases that does not meet the requirements, and then
manually modify
the cases that meets the requirements.
> thanks
> -- PMM
RE: [PATCH v3] pci-bridge: avoid linking a single downstream port more than once
> -Original Message-
> From: Igor Mammedov
> Sent: Thursday, July 25, 2024 4:36 PM
> To: Yao Xingtao via
> Cc: Yao, Xingtao/姚 幸涛 ; m...@redhat.com;
> marcel.apfelb...@gmail.com
> Subject: Re: [PATCH v3] pci-bridge: avoid linking a single downstream port
> more
> than once
>
> On Wed, 24 Jul 2024 23:27:31 -0400
> Yao Xingtao via wrote:
>
> > Since the downstream port is not checked, two slots can be linked to
> > a single port. However, this can prevent the driver from detecting the
> > device properly.
> >
> > It is necessary to ensure that a downstream port is not linked more than
> > once.
> >
> > Links:
> https://lore.kernel.org/qemu-devel/OSZPR01MB6453BC61D2FF4035F18084EF8D
> d...@oszpr01mb6453.jpnprd01.prod.outlook.com
> > Signed-off-by: Yao Xingtao
> >
> > ---
> > V2[2] -> V3:
> > - Move this check into pcie_cap_init()
> >
> > V1[1] -> V2:
> > - Move downstream port check forward
> >
> > [1]
> https://lore.kernel.org/qemu-devel/20240704033834.3362-1-yaoxt.f...@fujitsu.co
> m
> > [2]
> https://lore.kernel.org/qemu-devel/20240717085621.55315-1-yaoxt.fnst@fujitsu.c
> om
> > ---
> > hw/pci/pcie.c | 7 +++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 4b2f0805c6e0..aa154ec4b054 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -192,6 +192,13 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset,
> >
> > assert(pci_is_express(dev));
> >
> > +if (pci_is_express_downstream_port(dev) &&
sorry, this check will not be valid since the dev->exp.exp_cap was not set.
I will fix this bug in next revision.
> > +pcie_find_port_by_pn(pci_get_bus(dev), port)) {
> > +pos = -EBUSY;
> > +error_setg(errp, "Can't link port, error %d", pos);
> perhaps, also mention what's wrong and suggest user how to fix
> mis-configuration
Thanks, will append in next revision.
>
> > +return pos;
> > +}
> > +
> > pos = pci_add_capability(dev, PCI_CAP_ID_EXP, offset,
> > PCI_EXP_VER2_SIZEOF, errp);
> > if (pos < 0) {
RE: [PATCH 1/2] util/getauxval: Ensure setting errno if not found
> -Original Message-
> From: Vivian Wang
> Sent: Monday, July 22, 2024 4:24 PM
> To: Yao, Xingtao/姚 幸涛 ; qemu-devel@nongnu.org
> Cc: Richard Henderson ; Laurent Vivier
> Subject: Re: [PATCH 1/2] util/getauxval: Ensure setting errno if not found
>
> On 7/22/24 08:18, Xingtao Yao (Fujitsu) wrote:
> >
> >> -Original Message-
> >> From: qemu-devel-bounces+yaoxt.fnst=fujitsu@nongnu.org
> >> On Behalf Of
> Vivian
> >> Wang
> >> Sent: Sunday, July 21, 2024 5:08 PM
> >> To: qemu-devel@nongnu.org
> >> Cc: Vivian Wang ; Richard Henderson ;
> >> Laurent Vivier
> >> Subject: [PATCH 1/2] util/getauxval: Ensure setting errno if not found
> >>
> >> Sometimes zero is a valid value for getauxval (e.g. AT_EXECFD). Make
> >> sure that we can distinguish between a valid zero value and a not found
> >> entry by setting errno.
> >>
> >> Ignore getauxval from sys/auxv.h on glibc < 2.19 because it does not set
> >> errno.
> >>
> >> Signed-off-by: Vivian Wang
> >> ---
> >> util/getauxval.c | 14 --
> >> 1 file changed, 12 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/util/getauxval.c b/util/getauxval.c
> >> index b124107d61..f1008bdc59 100644
> >> --- a/util/getauxval.c
> >> +++ b/util/getauxval.c
> >> @@ -24,7 +24,13 @@
> >>
> >> #include "qemu/osdep.h"
> >>
> >> -#ifdef CONFIG_GETAUXVAL
> >> +/* If glibc < 2.19, getauxval can't be used because it does not set errno
> >> if
> >> + entry is not found. */
> >> +#if defined(CONFIG_GETAUXVAL) && \
> >> +(!defined(__GLIBC__) \
> >> +|| __GLIBC__ > 2 \
> >> +|| (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 19))
> > you can use GLIB_CHECK_VERSION(2, 19, 0) instead
> That wouldn't work. I'm testing for glibc, not glib.
sorry, I misunderstood.
> >> +
> >> /* Don't inline this in qemu/osdep.h, because pulling in for
> >> the system declaration of getauxval pulls in the system , which
> >> conflicts with qemu's version. */
> >> @@ -95,6 +101,7 @@ unsigned long qemu_getauxval(unsigned long type)
> >> }
> >> }
> >>
> >> +errno = ENOENT;
> >> return 0;
> >> }
> >>
> >> @@ -104,7 +111,9 @@ unsigned long qemu_getauxval(unsigned long type)
> >> unsigned long qemu_getauxval(unsigned long type)
> >> {
> >> unsigned long aux = 0;
> >> -elf_aux_info(type, &aux, sizeof(aux));
> >> +int ret = elf_aux_info(type, &aux, sizeof(aux));
> >> +if (ret != 0)
> >> +errno = ret;
> >> return aux;
> >> }
> >>
> >> @@ -112,6 +121,7 @@ unsigned long qemu_getauxval(unsigned long type)
> >>
> >> unsigned long qemu_getauxval(unsigned long type)
> >> {
> >> +errno = ENOSYS;
> >> return 0;
> >> }
> >>
> >> --
> >> 2.45.1
> >>
RE: [PATCH 00/13] make range overlap check more readable
> -Original Message- > From: Philippe Mathieu-Daudé > Sent: Monday, July 22, 2024 3:37 PM > To: Yao, Xingtao/姚 幸涛 ; qemu-devel@nongnu.org > Subject: Re: [PATCH 00/13] make range overlap check more readable > > On 22/7/24 08:59, Xingtao Yao (Fujitsu) wrote: > > > > > >> -Original Message- > >> From: Philippe Mathieu-Daudé > >> Sent: Monday, July 22, 2024 2:43 PM > >> To: Yao, Xingtao/姚 幸涛 ; qemu-devel@nongnu.org > >> Subject: Re: [PATCH 00/13] make range overlap check more readable > >> > >> Hi Yao, > >> > >> On 22/7/24 06:07, Yao Xingtao via wrote: > >>> Currently, some components still open-coding the range overlap check. > >>> Sometimes this check may be fail because some patterns are missed. > >> > >> How did you catch all these use cases? > > I used the Coccinelle to match these use cases, the pattern is below > > range_overlap.cocci: > > > > // use ranges_overlap() instead of open-coding the overlap check > > @@ > > expression E1, E2, E3, E4; > > @@ > > ( > > - E2 <= E3 || E1 >= E4 > > + !ranges_overlap(E1, E2, E3, E4) > > | > > > > - (E2 <= E3) || (E1 >= E4) > > + !ranges_overlap(E1, E2, E3, E4) > > | > > > > - E1 < E4 && E2 > E3 > > + ranges_overlap(E1, E2, E3, E4) > > | > > > > - (E1 < E4) && (E2 > E3) > > + ranges_overlap(E1, E2, E3, E4) > > | > > > > - (E1 >= E3 && E1 < E4) || (E2 > E3 && E2 <= E4) > > + ranges_overlap(E1, E2, E3, E4) > > > > | > > - ((E1 >= E3) && (E1 < E4)) || ((E2 > E3) && (E2 <= E4)) > > + ranges_overlap(E1, E2, E3, E4) > > ) > > Please add to scripts/coccinelle/range.cocci. OK, I will add this file in next revision. > > > > > then execute the command: > > # spatch --macro-file scripts/cocci-macro-file.h --sp-file > > range_overlap.cocci > --keep-comments --in-place --use-gitgrep --dir . > > > > but some of the matched cases are not valid and need to be > > manually judged. > > > > there may be cases that have not been matched yet.
RE: [PATCH 00/13] make range overlap check more readable
> -Original Message- > From: Philippe Mathieu-Daudé > Sent: Monday, July 22, 2024 2:43 PM > To: Yao, Xingtao/姚 幸涛 ; qemu-devel@nongnu.org > Subject: Re: [PATCH 00/13] make range overlap check more readable > > Hi Yao, > > On 22/7/24 06:07, Yao Xingtao via wrote: > > Currently, some components still open-coding the range overlap check. > > Sometimes this check may be fail because some patterns are missed. > > How did you catch all these use cases? I used the Coccinelle to match these use cases, the pattern is below range_overlap.cocci: // use ranges_overlap() instead of open-coding the overlap check @@ expression E1, E2, E3, E4; @@ ( - E2 <= E3 || E1 >= E4 + !ranges_overlap(E1, E2, E3, E4) | - (E2 <= E3) || (E1 >= E4) + !ranges_overlap(E1, E2, E3, E4) | - E1 < E4 && E2 > E3 + ranges_overlap(E1, E2, E3, E4) | - (E1 < E4) && (E2 > E3) + ranges_overlap(E1, E2, E3, E4) | - (E1 >= E3 && E1 < E4) || (E2 > E3 && E2 <= E4) + ranges_overlap(E1, E2, E3, E4) | - ((E1 >= E3) && (E1 < E4)) || ((E2 > E3) && (E2 <= E4)) + ranges_overlap(E1, E2, E3, E4) ) then execute the command: # spatch --macro-file scripts/cocci-macro-file.h --sp-file range_overlap.cocci --keep-comments --in-place --use-gitgrep --dir . but some of the matched cases are not valid and need to be manually judged. there may be cases that have not been matched yet. > > > To avoid the above problems and improve the readability of the code, > > it is better to use the ranges_overlap() to do this check. > > > > Yao Xingtao (13): > >range: Make ranges_overlap() return bool > >arm/boot: make range overlap check more readable > >core/loader: make range overlap check more readable > >cxl/mailbox: make range overlap check more readable > >display/sm501: make range overlap check more readable > >aspeed_smc: make range overlap check more readable > >qtest/fuzz: make range overlap check more readable > >sparc/ldst_helper: make range overlap check more readable > >system/memory_mapping: make range overlap check more readable > >block/vhdx: make range overlap check more readable > >crypto/block-luks: make range overlap check more readable > >dump: make range overlap check more readable > >block/qcow2-cluster: make range overlap check more readable
RE: [PATCH 1/2] util/getauxval: Ensure setting errno if not found
> -Original Message-
> From: qemu-devel-bounces+yaoxt.fnst=fujitsu@nongnu.org
> On Behalf Of Vivian
> Wang
> Sent: Sunday, July 21, 2024 5:08 PM
> To: qemu-devel@nongnu.org
> Cc: Vivian Wang ; Richard Henderson ;
> Laurent Vivier
> Subject: [PATCH 1/2] util/getauxval: Ensure setting errno if not found
>
> Sometimes zero is a valid value for getauxval (e.g. AT_EXECFD). Make
> sure that we can distinguish between a valid zero value and a not found
> entry by setting errno.
>
> Ignore getauxval from sys/auxv.h on glibc < 2.19 because it does not set
> errno.
>
> Signed-off-by: Vivian Wang
> ---
> util/getauxval.c | 14 --
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/util/getauxval.c b/util/getauxval.c
> index b124107d61..f1008bdc59 100644
> --- a/util/getauxval.c
> +++ b/util/getauxval.c
> @@ -24,7 +24,13 @@
>
> #include "qemu/osdep.h"
>
> -#ifdef CONFIG_GETAUXVAL
> +/* If glibc < 2.19, getauxval can't be used because it does not set errno if
> + entry is not found. */
> +#if defined(CONFIG_GETAUXVAL) && \
> +(!defined(__GLIBC__) \
> +|| __GLIBC__ > 2 \
> +|| (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 19))
you can use GLIB_CHECK_VERSION(2, 19, 0) instead
> +
> /* Don't inline this in qemu/osdep.h, because pulling in for
> the system declaration of getauxval pulls in the system , which
> conflicts with qemu's version. */
> @@ -95,6 +101,7 @@ unsigned long qemu_getauxval(unsigned long type)
> }
> }
>
> +errno = ENOENT;
> return 0;
> }
>
> @@ -104,7 +111,9 @@ unsigned long qemu_getauxval(unsigned long type)
> unsigned long qemu_getauxval(unsigned long type)
> {
> unsigned long aux = 0;
> -elf_aux_info(type, &aux, sizeof(aux));
> +int ret = elf_aux_info(type, &aux, sizeof(aux));
> +if (ret != 0)
> +errno = ret;
> return aux;
> }
>
> @@ -112,6 +121,7 @@ unsigned long qemu_getauxval(unsigned long type)
>
> unsigned long qemu_getauxval(unsigned long type)
> {
> +errno = ENOSYS;
> return 0;
> }
>
> --
> 2.45.1
>
RE: [PATCH] mem/cxl_type3: Fix overlapping region validation error
> -Original Message- > From: Peter Maydell > Sent: Friday, July 19, 2024 1:12 AM > To: Jonathan Cameron > Cc: Yao, Xingtao/姚 幸涛 ; fan...@samsung.com; > qemu-devel@nongnu.org > Subject: Re: [PATCH] mem/cxl_type3: Fix overlapping region validation error > > On Thu, 18 Jul 2024 at 17:37, Jonathan Cameron via > wrote: > > > > On Thu, 18 Jul 2024 05:07:53 -0400 > > Yao Xingtao wrote: > > > > > When injecting a new poisoned region through qmp_cxl_inject_poison(), > > > the newly injected region should not overlap with existing poisoned > > > regions. > > > > > > The current validation method does not consider the following > > > overlapping region: > > > ┌───┬───┬───┐ > > > │a │ b(a) │a │ > > > └───┴───┴───┘ > > > (a is a newly added region, b is an existing region, and b is a > > > subregion of a) > > > > > > Signed-off-by: Yao Xingtao > > Looks correct to me. > > > > Reviewed-by: Jonathan Cameron > > I've queued it on my local branch. > > I need to put together an updated public one. > > > > No huge rush to queue this up though I think as the effects > > are minor. > > I think you can probably write this as >ranges_overlap(start, len, p->start, p->length) > using the utility function in include/qemu/ranges.h, which is > a bit more readable than open-coding the overlap test. Great! I will fix it in the next revision. > > (There's another couple of open-coded overlap tests in > cxl-mailbox-utils.c.) I will collect these issues and fix them in separate patches. > > thanks > -- PMM
RE: [PATCH] mem/cxl_type3: Fix overlapping region validation error
> > > As mentioned by Peter, we can use ranges_overlap() to improve the > code readability. Other than that, looks good t me. > > btw, not sure only me or not, but the message does not display > correctly in mutt, seems not a plain text message, but looks fine in > outlook. I am not sure as well, but it shows properly on the mailing list. > > Fan
RE: [PATCH v2] pci-bridge: avoid linking a single downstream port more than once
> -Original Message- > From: Michael S. Tsirkin > Sent: Wednesday, July 17, 2024 8:04 PM > To: Yao, Xingtao/姚 幸涛 > Cc: marcel.apfelb...@gmail.com; qemu-devel@nongnu.org; > jonathan.came...@huawei.com > Subject: Re: [PATCH v2] pci-bridge: avoid linking a single downstream port > more > than once > > On Wed, Jul 17, 2024 at 04:56:21AM -0400, Yao Xingtao wrote: > > Since the downstream port is not checked, two slots can be linked to > > a single port. However, this can prevent the driver from detecting the > > device properly. > > > > It is necessary to ensure that a downstream port is not linked more than > > once. > > > > Links: > https://lore.kernel.org/qemu-devel/OSZPR01MB6453BC61D2FF4035F18084EF8D > d...@oszpr01mb6453.jpnprd01.prod.outlook.com > > Signed-off-by: Yao Xingtao > > You also need to take ARI into account. > That can look like slot != 0. Sorry, I'm not familiar with the PCIe protocol, could you explain it in detail?
RE: [PATCH v2] pci-bridge: avoid linking a single downstream port more than once
> -Original Message-
> From: Philippe Mathieu-Daudé
> Sent: Wednesday, July 17, 2024 6:18 PM
> To: Yao, Xingtao/姚 幸涛 ; m...@redhat.com;
> marcel.apfelb...@gmail.com
> Cc: qemu-devel@nongnu.org; jonathan.came...@huawei.com
> Subject: Re: [PATCH v2] pci-bridge: avoid linking a single downstream port
> more
> than once
>
> Hi Yao,
>
> On 17/7/24 10:56, Yao Xingtao via wrote:
> > Since the downstream port is not checked, two slots can be linked to
> > a single port. However, this can prevent the driver from detecting the
> > device properly.
> >
> > It is necessary to ensure that a downstream port is not linked more than
> > once.
> >
> > Links:
> https://lore.kernel.org/qemu-devel/OSZPR01MB6453BC61D2FF4035F18084EF8D
> d...@oszpr01mb6453.jpnprd01.prod.outlook.com
> > Signed-off-by: Yao Xingtao
> >
> > ---
> > V1[1] -> V2:
> > - Move downstream port check forward
> >
> > [1]
> https://lore.kernel.org/qemu-devel/20240704033834.3362-1-yaoxt.f...@fujitsu.co
> m
> > ---
> > hw/pci-bridge/cxl_downstream.c | 5 +
> > hw/pci-bridge/pcie_root_port.c | 5 +
> > hw/pci-bridge/xio3130_downstream.c | 5 +
> > 3 files changed, 15 insertions(+)
> >
> > diff --git a/hw/pci-bridge/cxl_downstream.c
> b/hw/pci-bridge/cxl_downstream.c
> > index 742da07a015a..af81ddfeec13 100644
> > --- a/hw/pci-bridge/cxl_downstream.c
> > +++ b/hw/pci-bridge/cxl_downstream.c
> > @@ -142,6 +142,11 @@ static void cxl_dsp_realize(PCIDevice *d, Error **errp)
> > MemoryRegion *component_bar = &cregs->component_registers;
> > int rc;
> >
> > +if (pcie_find_port_by_pn(pci_get_bus(d), p->port) != NULL) {
> > +error_setg(errp, "Can't link port, error %d", -EBUSY);
> > +return;
>
> Could pcie_cap_slot_init() be a good place to check for that?
>
> Otherwise IMHO we should add a helper in "hw/pci/pcie.h" and
> call it here, not duplicate this code in each model.
Yes, thanks for your comment.
I think pcie_cap_init() may be better.
>
> > +}
> > +
> > pci_bridge_initfn(d, TYPE_PCIE_BUS);
> > pcie_port_init_reg(d);
RE: [PATCH v4] hw/core/loader: allow loading larger ROMs
Reviewed-by: Xingtao Yao
> -Original Message-
> From: Gregor Haas
> Sent: Saturday, June 29, 2024 2:27 AM
> To: qemu-devel@nongnu.org
> Cc: berra...@redhat.com; Yao, Xingtao/姚 幸涛 ;
> Gregor Haas
> Subject: [PATCH v4] hw/core/loader: allow loading larger ROMs
>
> The read() syscall is not guaranteed to return all data from a file. The
> default ROM loader implementation currently does not take this into account,
> instead failing if all bytes are not read at once. This change loads the ROM
> using g_file_get_contents() instead, which correctly reads all data using
> multiple calls to read() while also returning the loaded ROM size.
>
> Signed-off-by: Gregor Haas
> ---
> hw/core/loader.c | 30 +-
> 1 file changed, 5 insertions(+), 25 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 2f8105d7de..4a5714 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1075,8 +1075,7 @@ ssize_t rom_add_file(const char *file, const char
> *fw_dir,
> {
> MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> Rom *rom;
> -ssize_t rc;
> -int fd = -1;
> +g_autoptr(GError) gerr = NULL;
> char devpath[100];
>
> if (as && mr) {
> @@ -1094,35 +1093,19 @@ ssize_t rom_add_file(const char *file, const char
> *fw_dir,
> rom->path = g_strdup(file);
> }
>
> -fd = open(rom->path, O_RDONLY | O_BINARY);
> -if (fd == -1) {
> -fprintf(stderr, "Could not open option rom '%s': %s\n",
> -rom->path, strerror(errno));
> -goto err;
> -}
> -
> if (fw_dir) {
> rom->fw_dir = g_strdup(fw_dir);
> rom->fw_file = g_strdup(file);
> }
> rom->addr = addr;
> -rom->romsize = lseek(fd, 0, SEEK_END);
> -if (rom->romsize == -1) {
> -fprintf(stderr, "rom: file %-20s: get size error: %s\n",
> -rom->name, strerror(errno));
> +if (!g_file_get_contents(rom->path, (gchar **) &rom->data,
> + &rom->romsize, &gerr)) {
> +fprintf(stderr, "rom: file %-20s: error %s\n",
> +rom->name, gerr->message);
> goto err;
> }
>
> rom->datasize = rom->romsize;
> -rom->data = g_malloc0(rom->datasize);
> -lseek(fd, 0, SEEK_SET);
> -rc = read(fd, rom->data, rom->datasize);
> -if (rc != rom->datasize) {
> -fprintf(stderr, "rom: file %-20s: read error: rc=%zd (expected
> %zd)\n",
> -rom->name, rc, rom->datasize);
> -goto err;
> -}
> -close(fd);
> rom_insert(rom);
> if (rom->fw_file && fw_cfg) {
> const char *basename;
> @@ -1159,9 +1142,6 @@ ssize_t rom_add_file(const char *file, const char
> *fw_dir,
> return 0;
>
> err:
> -if (fd != -1)
> -close(fd);
> -
> rom_free(rom);
> return -1;
> }
> --
> 2.45.2
RE: [PATCH v5 3/7] plugins: extend API to get latest memory value accessed
Reviewed-by: Xingtao Yao
> -Original Message-
> From: qemu-devel-bounces+yaoxt.fnst=fujitsu@nongnu.org
> On Behalf Of
> Pierrick Bouvier
> Sent: Friday, July 5, 2024 8:34 AM
> To: qemu-devel@nongnu.org
> Cc: Alexandre Iooss ; Richard Henderson
> ; Marcel Apfelbaum
> ; Pierrick Bouvier ;
> Alex Bennée ; Paolo Bonzini ;
> Yanan Wang ; Mahmoud Mandour
> ; Eduardo Habkost ; Philippe
> Mathieu-Daudé
> Subject: [PATCH v5 3/7] plugins: extend API to get latest memory value
> accessed
>
> This value can be accessed only during a memory callback, using
> new qemu_plugin_mem_get_value function.
>
> Returned value can be extended when QEMU will support accesses wider
> than 128 bits.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1719
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2152
> Reviewed-by: Richard Henderson
> Signed-off-by: Pierrick Bouvier
> ---
> include/qemu/qemu-plugin.h | 32
>
> plugins/api.c| 33
> +
> plugins/qemu-plugins.symbols | 1 +
> 3 files changed, 66 insertions(+)
>
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index c71c705b699..649ce89815f 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -262,6 +262,29 @@ enum qemu_plugin_mem_rw {
> QEMU_PLUGIN_MEM_RW,
> };
>
> +enum qemu_plugin_mem_value_type {
> +QEMU_PLUGIN_MEM_VALUE_U8,
> +QEMU_PLUGIN_MEM_VALUE_U16,
> +QEMU_PLUGIN_MEM_VALUE_U32,
> +QEMU_PLUGIN_MEM_VALUE_U64,
> +QEMU_PLUGIN_MEM_VALUE_U128,
> +};
> +
> +/* typedef qemu_plugin_mem_value - value accessed during a load/store */
> +typedef struct {
> +enum qemu_plugin_mem_value_type type;
> +union {
> +uint8_t u8;
> +uint16_t u16;
> +uint32_t u32;
> +uint64_t u64;
> +struct {
> +uint64_t low;
> +uint64_t high;
> +} u128;
> +} data;
> +} qemu_plugin_mem_value;
> +
> /**
> * enum qemu_plugin_cond - condition to enable callback
> *
> @@ -551,6 +574,15 @@ bool
> qemu_plugin_mem_is_big_endian(qemu_plugin_meminfo_t info);
> QEMU_PLUGIN_API
> bool qemu_plugin_mem_is_store(qemu_plugin_meminfo_t info);
>
> +/**
> + * qemu_plugin_mem_get_mem_value() - return last value loaded/stored
> + * @info: opaque memory transaction handle
> + *
> + * Returns: memory value
> + */
> +QEMU_PLUGIN_API
> +qemu_plugin_mem_value
> qemu_plugin_mem_get_value(qemu_plugin_meminfo_t info);
> +
> /**
> * qemu_plugin_get_hwaddr() - return handle for memory operation
> * @info: opaque memory info structure
> diff --git a/plugins/api.c b/plugins/api.c
> index 2ff13d09de6..3316d4a04d4 100644
> --- a/plugins/api.c
> +++ b/plugins/api.c
> @@ -351,6 +351,39 @@ bool
> qemu_plugin_mem_is_store(qemu_plugin_meminfo_t info)
> return get_plugin_meminfo_rw(info) & QEMU_PLUGIN_MEM_W;
> }
>
> +qemu_plugin_mem_value
> qemu_plugin_mem_get_value(qemu_plugin_meminfo_t info)
> +{
> +uint64_t low = current_cpu->neg.plugin_mem_value_low;
> +qemu_plugin_mem_value value;
> +
> +switch (qemu_plugin_mem_size_shift(info)) {
> +case 0:
> +value.type = QEMU_PLUGIN_MEM_VALUE_U8;
> +value.data.u8 = (uint8_t)low;
> +break;
> +case 1:
> +value.type = QEMU_PLUGIN_MEM_VALUE_U16;
> +value.data.u16 = (uint16_t)low;
> +break;
> +case 2:
> +value.type = QEMU_PLUGIN_MEM_VALUE_U32;
> +value.data.u32 = (uint32_t)low;
> +break;
> +case 3:
> +value.type = QEMU_PLUGIN_MEM_VALUE_U64;
> +value.data.u64 = low;
> +break;
> +case 4:
> +value.type = QEMU_PLUGIN_MEM_VALUE_U128;
> +value.data.u128.low = low;
> +value.data.u128.high = current_cpu->neg.plugin_mem_value_high;
> +break;
> +default:
> +g_assert_not_reached();
> +}
> +return value;
> +}
> +
> /*
> * Virtual Memory queries
> */
> diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
> index ca773d8d9fe..eed9d8abd90 100644
> --- a/plugins/qemu-plugins.symbols
> +++ b/plugins/qemu-plugins.symbols
> @@ -13,6 +13,7 @@
>qemu_plugin_insn_size;
>qemu_plugin_insn_symbol;
>qemu_plugin_insn_vaddr;
> + qemu_plugin_mem_get_value;
>qemu_plugin_mem_is_big_endian;
>qemu_plugin_mem_is_sign_extended;
>qemu_plugin_mem_is_store;
> --
> 2.39.2
>
RE: [PATCH v5 6/7] tests/plugin/mem: add option to print memory accesses
Reviewed-by: Xingtao Yao
> -Original Message-
> From: qemu-devel-bounces+yaoxt.fnst=fujitsu@nongnu.org
> On Behalf Of
> Pierrick Bouvier
> Sent: Friday, July 5, 2024 8:34 AM
> To: qemu-devel@nongnu.org
> Cc: Alexandre Iooss ; Richard Henderson
> ; Marcel Apfelbaum
> ; Pierrick Bouvier ;
> Alex Bennée ; Paolo Bonzini ;
> Yanan Wang ; Mahmoud Mandour
> ; Eduardo Habkost ; Philippe
> Mathieu-Daudé
> Subject: [PATCH v5 6/7] tests/plugin/mem: add option to print memory accesses
>
> By using "print-accesses=true" option, mem plugin will now print every
> value accessed, with associated size, type (store vs load), symbol,
> instruction address and phys/virt address accessed.
>
> Reviewed-by: Richard Henderson
> Signed-off-by: Pierrick Bouvier
> ---
> tests/plugin/mem.c | 69
> +-
> 1 file changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/tests/plugin/mem.c b/tests/plugin/mem.c
> index b650dddcce1..086e6f5bdfc 100644
> --- a/tests/plugin/mem.c
> +++ b/tests/plugin/mem.c
> @@ -21,10 +21,15 @@ typedef struct {
> uint64_t io_count;
> } CPUCount;
>
> +typedef struct {
> +uint64_t vaddr;
> +const char *sym;
> +} InsnInfo;
> +
> static struct qemu_plugin_scoreboard *counts;
> static qemu_plugin_u64 mem_count;
> static qemu_plugin_u64 io_count;
> -static bool do_inline, do_callback;
> +static bool do_inline, do_callback, do_print_accesses;
> static bool do_haddr;
> static enum qemu_plugin_mem_rw rw = QEMU_PLUGIN_MEM_RW;
>
> @@ -60,6 +65,44 @@ static void vcpu_mem(unsigned int cpu_index,
> qemu_plugin_meminfo_t meminfo,
> }
> }
>
> +static void print_access(unsigned int cpu_index, qemu_plugin_meminfo_t
> meminfo,
> + uint64_t vaddr, void *udata)
> +{
> +InsnInfo *insn_info = udata;
> +unsigned size = 8 << qemu_plugin_mem_size_shift(meminfo);
> +const char *type = qemu_plugin_mem_is_store(meminfo) ? "store" : "load";
> +qemu_plugin_mem_value value = qemu_plugin_mem_get_value(meminfo);
> +uint64_t hwaddr =
> +qemu_plugin_hwaddr_phys_addr(qemu_plugin_get_hwaddr(meminfo,
> vaddr));
> +g_autoptr(GString) out = g_string_new("");
> +g_string_printf(out,
> +"0x%"PRIx64",%s,0x%"PRIx64",0x%"PRIx64",%d,%s,",
> +insn_info->vaddr, insn_info->sym,
> +vaddr, hwaddr, size, type);
> +switch (value.type) {
> +case QEMU_PLUGIN_MEM_VALUE_U8:
> +g_string_append_printf(out, "0x%02"PRIx8, value.data.u8);
> +break;
> +case QEMU_PLUGIN_MEM_VALUE_U16:
> +g_string_append_printf(out, "0x%04"PRIx16, value.data.u16);
> +break;
> +case QEMU_PLUGIN_MEM_VALUE_U32:
> +g_string_append_printf(out, "0x%08"PRIx32, value.data.u32);
> +break;
> +case QEMU_PLUGIN_MEM_VALUE_U64:
> +g_string_append_printf(out, "0x%016"PRIx64, value.data.u64);
> +break;
> +case QEMU_PLUGIN_MEM_VALUE_U128:
> +g_string_append_printf(out, "0x%016"PRIx64"%016"PRIx64,
> + value.data.u128.high, value.data.u128.low);
> +break;
> +default:
> +g_assert_not_reached();
> +}
> +g_string_append_printf(out, "\n");
> +qemu_plugin_outs(out->str);
> +}
> +
> static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
> {
> size_t n = qemu_plugin_tb_n_insns(tb);
> @@ -79,6 +122,16 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct
> qemu_plugin_tb *tb)
> QEMU_PLUGIN_CB_NO_REGS,
> rw, NULL);
> }
> +if (do_print_accesses) {
> +/* we leak this pointer, to avoid locking to keep track of it */
> +InsnInfo *insn_info = g_malloc(sizeof(InsnInfo));
> +const char *sym = qemu_plugin_insn_symbol(insn);
> +insn_info->sym = sym ? sym : "";
> +insn_info->vaddr = qemu_plugin_insn_vaddr(insn);
> +qemu_plugin_register_vcpu_mem_cb(insn, print_access,
> + QEMU_PLUGIN_CB_NO_REGS,
> + rw, (void *) insn_info);
> +}
> }
> }
>
> @@ -117,6 +170,12 @@ QEMU_PLUGIN_EXPORT int
> qemu_plugin_install(qemu_plugin_id_t id,
> fprintf(stderr, "boolean argument parsing failed: %s\n",
> opt);
> return -1;
> }
> +} else if (g_strcmp0(tokens[0], "print-accesses") == 0) {
> +if (!qemu_plugin_bool_parse(tokens[0], tokens[1],
> +&do_print_accesses)) {
> +fprintf(stderr, "boolean argument parsing failed: %s\n",
> opt);
> +return -1;
> +}
> } else {
> fprintf(stderr, "option parsing failed: %s\n", opt);
> return -1;
> @@ -129,6 +188,14 @@ QEMU_PLUGIN_EXPORT int
> qemu_plugin_insta
RE: [PATCH] hw/cxl/cxl-host: Fix guest crash when getting cxl-fmw property
> -Original Message-
> From: qemu-devel-bounces+yaoxt.fnst=fujitsu@nongnu.org
> On Behalf Of Zhao
> Liu
> Sent: Thursday, July 4, 2024 5:34 PM
> To: Jonathan Cameron ; Fan Ni
>
> Cc: qemu-devel@nongnu.org; qemu-sta...@nongnu.org; Zhao Liu
>
> Subject: [PATCH] hw/cxl/cxl-host: Fix guest crash when getting cxl-fmw
> property
>
> From: Zhao Liu
>
> Guest crashes (Segmentation fault) when getting cxl-fmw property via
> qmp:
>
> (QEMU) qom-get path=machine property=cxl-fmw
>
> This issue is caused by accessing wrong callback (opaque) type in
> machine_get_cfmw().
>
> cxl_machine_init() sets the callback as `CXLState *` type but
> machine_get_cfmw() treats the callback as
> `CXLFixedMemoryWindowOptionsList **`.
>
> Fix this error by casting opaque to `CXLState *` type in
> machine_get_cfmw().
>
> Fixes: 03b39fcf64bc ("hw/cxl: Make the CXL fixed memory window setup a
> machine parameter.")
> Signed-off-by: Zhao Liu
> ---
> hw/cxl/cxl-host.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/cxl/cxl-host.c b/hw/cxl/cxl-host.c
> index c5f5fcfd64d0..e9f2543c43c6 100644
> --- a/hw/cxl/cxl-host.c
> +++ b/hw/cxl/cxl-host.c
> @@ -315,7 +315,8 @@ static void machine_set_cxl(Object *obj, Visitor *v, const
> char *name,
> static void machine_get_cfmw(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> {
> -CXLFixedMemoryWindowOptionsList **list = opaque;
> +CXLState *state = opaque;
> +CXLFixedMemoryWindowOptionsList **list = &state->cfmw_list;
>
> visit_type_CXLFixedMemoryWindowOptionsList(v, name, list, errp);
> }
> --
> 2.34.1
>
Reviewed-by: Xingtao Yao
RE: [PATCH v4 5/7] tests/tcg: allow to check output of plugins
Tested-by: Xingtao Yao > -Original Message- > From: qemu-devel-bounces+yaoxt.fnst=fujitsu@nongnu.org > On Behalf Of > Pierrick Bouvier > Sent: Wednesday, July 3, 2024 2:45 AM > To: qemu-devel@nongnu.org > Cc: Alex Bennée ; Mahmoud Mandour > ; Pierrick Bouvier ; > Alexandre Iooss ; Philippe Mathieu-Daudé > ; Paolo Bonzini ; Richard Henderson > ; Eduardo Habkost > Subject: [PATCH v4 5/7] tests/tcg: allow to check output of plugins > > A specific plugin test can now read and check a plugin output, to ensure > it contains expected values. > > Signed-off-by: Pierrick Bouvier > --- > tests/tcg/Makefile.target | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target > index dc5c8b7a3b4..b78fd99c337 100644 > --- a/tests/tcg/Makefile.target > +++ b/tests/tcg/Makefile.target > @@ -90,6 +90,7 @@ CFLAGS= > LDFLAGS= > > QEMU_OPTS= > +CHECK_PLUGIN_OUTPUT_COMMAND=true > > > # If TCG debugging, or TCI is enabled things are a lot slower > @@ -180,6 +181,9 @@ run-plugin-%: > -plugin $(PLUGIN_LIB)/$(call > extract-plugin,$@)$(PLUGIN_ARGS) \ > -d plugin -D $*.pout \ >$(call strip-plugin,$<)) > + $(call quiet-command, $(CHECK_PLUGIN_OUTPUT_COMMAND) > $*.pout, \ > +TEST, check plugin $(call extract-plugin,$@) output \ > +with $(call strip-plugin,$<)) > else > run-%: % > $(call run-test, $<, \ > @@ -194,6 +198,9 @@ run-plugin-%: > -plugin $(PLUGIN_LIB)/$(call > extract-plugin,$@)$(PLUGIN_ARGS) \ > -d plugin -D $*.pout \ > $(QEMU_OPTS) $(call strip-plugin,$<)) > + $(call quiet-command, $(CHECK_PLUGIN_OUTPUT_COMMAND) > $*.pout, \ > +TEST, check plugin $(call extract-plugin,$@) output \ > +with $(call strip-plugin,$<)) > endif > > gdb-%: % > -- > 2.39.2 >
RE: [PATCH v4 4/7] tests/tcg: add mechanism to run specific tests with plugins
Tested-by: Xingtao Yao > -Original Message- > From: qemu-devel-bounces+yaoxt.fnst=fujitsu@nongnu.org > On Behalf Of > Pierrick Bouvier > Sent: Wednesday, July 3, 2024 2:45 AM > To: qemu-devel@nongnu.org > Cc: Alex Bennée ; Mahmoud Mandour > ; Pierrick Bouvier ; > Alexandre Iooss ; Philippe Mathieu-Daudé > ; Paolo Bonzini ; Richard Henderson > ; Eduardo Habkost > Subject: [PATCH v4 4/7] tests/tcg: add mechanism to run specific tests with > plugins > > Only multiarch tests are run with plugins, and we want to be able to run > per-arch test with plugins too. > > Reviewed-by: Richard Henderson > Signed-off-by: Pierrick Bouvier > --- > tests/tcg/Makefile.target | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target > index f21be50d3b2..dc5c8b7a3b4 100644 > --- a/tests/tcg/Makefile.target > +++ b/tests/tcg/Makefile.target > @@ -152,10 +152,11 @@ PLUGINS=$(patsubst %.c, lib%.so, $(notdir $(wildcard > $(PLUGIN_SRC)/*.c))) > # only expand MULTIARCH_TESTS which are common on most of our targets > # to avoid an exponential explosion as new tests are added. We also > # add some special helpers the run-plugin- rules can use below. > +# In more, extra tests can be added using PLUGINS_TESTS variable. > > ifneq ($(MULTIARCH_TESTS),) > $(foreach p,$(PLUGINS), \ > - $(foreach t,$(MULTIARCH_TESTS),\ > + $(foreach t,$(MULTIARCH_TESTS) $(PLUGINS_TESTS),\ > $(eval run-plugin-$(t)-with-$(p): $t $p) \ > $(eval RUN_TESTS+=run-plugin-$(t)-with-$(p > endif # MULTIARCH_TESTS > -- > 2.39.2 >
RE: [PATCH v4 6/7] tests/plugin/mem: add option to print memory accesses
Tested-by: Xingtao Yao
one small suggestion:
Keeping the addresses or values of fixed size in output message can improve the
readability of logs.
like:
> +case QEMU_PLUGIN_MEM_VALUE_U8:
> +g_string_append_printf(out, "0x%"PRIx8, value.data.u8);
> +break;
case QEMU_PLUGIN_MEM_VALUE_U8:
g_string_append_printf(out, "0x02%"PRIx8, value.data.u8);
break;
> -Original Message-
> From: qemu-devel-bounces+yaoxt.fnst=fujitsu@nongnu.org
> On Behalf Of
> Pierrick Bouvier
> Sent: Wednesday, July 3, 2024 2:45 AM
> To: qemu-devel@nongnu.org
> Cc: Alex Bennée ; Mahmoud Mandour
> ; Pierrick Bouvier ;
> Alexandre Iooss ; Philippe Mathieu-Daudé
> ; Paolo Bonzini ; Richard Henderson
> ; Eduardo Habkost
> Subject: [PATCH v4 6/7] tests/plugin/mem: add option to print memory accesses
>
> By using "print-accesses=true" option, mem plugin will now print every
> value accessed, with associated size, type (store vs load), symbol,
> instruction address and phys/virt address accessed.
>
> Signed-off-by: Pierrick Bouvier
> ---
> tests/plugin/mem.c | 69
> +-
> 1 file changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/tests/plugin/mem.c b/tests/plugin/mem.c
> index b650dddcce1..aecbad8e68d 100644
> --- a/tests/plugin/mem.c
> +++ b/tests/plugin/mem.c
> @@ -21,10 +21,15 @@ typedef struct {
> uint64_t io_count;
> } CPUCount;
>
> +typedef struct {
> +uint64_t vaddr;
> +const char *sym;
> +} InsnInfo;
> +
> static struct qemu_plugin_scoreboard *counts;
> static qemu_plugin_u64 mem_count;
> static qemu_plugin_u64 io_count;
> -static bool do_inline, do_callback;
> +static bool do_inline, do_callback, do_print_accesses;
> static bool do_haddr;
> static enum qemu_plugin_mem_rw rw = QEMU_PLUGIN_MEM_RW;
>
> @@ -60,6 +65,44 @@ static void vcpu_mem(unsigned int cpu_index,
> qemu_plugin_meminfo_t meminfo,
> }
> }
>
> +static void print_access(unsigned int cpu_index, qemu_plugin_meminfo_t
> meminfo,
> + uint64_t vaddr, void *udata)
> +{
> +InsnInfo *insn_info = udata;
> +unsigned size = 8 << qemu_plugin_mem_size_shift(meminfo);
> +const char *type = qemu_plugin_mem_is_store(meminfo) ? "store" : "load";
> +qemu_plugin_mem_value value = qemu_plugin_mem_get_value(meminfo);
> +uint64_t hwaddr =
> +qemu_plugin_hwaddr_phys_addr(qemu_plugin_get_hwaddr(meminfo,
> vaddr));
> +g_autoptr(GString) out = g_string_new("");
> +g_string_printf(out,
> +"0x%"PRIx64",%s,0x%"PRIx64",0x%"PRIx64",%d,%s,",
> +insn_info->vaddr, insn_info->sym,
> +vaddr, hwaddr, size, type);
> +switch (value.type) {
> +case QEMU_PLUGIN_MEM_VALUE_U8:
> +g_string_append_printf(out, "0x%"PRIx8, value.data.u8);
> +break;
> +case QEMU_PLUGIN_MEM_VALUE_U16:
> +g_string_append_printf(out, "0x%"PRIx16, value.data.u16);
> +break;
> +case QEMU_PLUGIN_MEM_VALUE_U32:
> +g_string_append_printf(out, "0x%"PRIx32, value.data.u32);
> +break;
> +case QEMU_PLUGIN_MEM_VALUE_U64:
> +g_string_append_printf(out, "0x%"PRIx64, value.data.u64);
> +break;
> +case QEMU_PLUGIN_MEM_VALUE_U128:
> +g_string_append_printf(out, "0x%.0"PRIx64"%"PRIx64,
> + value.data.u128.high, value.data.u128.low);
> +break;
> +default:
> +g_assert_not_reached();
> +}
> +g_string_append_printf(out, "\n");
> +qemu_plugin_outs(out->str);
> +}
> +
> static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
> {
> size_t n = qemu_plugin_tb_n_insns(tb);
> @@ -79,6 +122,16 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct
> qemu_plugin_tb *tb)
> QEMU_PLUGIN_CB_NO_REGS,
> rw, NULL);
> }
> +if (do_print_accesses) {
> +/* we leak this pointer, to avoid locking to keep track of it */
> +InsnInfo *insn_info = g_malloc(sizeof(InsnInfo));
> +const char *sym = qemu_plugin_insn_symbol(insn);
> +insn_info->sym = sym ? sym : "";
> +insn_info->vaddr = qemu_plugin_insn_vaddr(insn);
> +qemu_plugin_register_vcpu_mem_cb(insn, print_access,
> + QEMU_PLUGIN_CB_NO_REGS,
> + rw, (void *) insn_info);
> +}
> }
> }
>
> @@ -117,6 +170,12 @@ QEMU_PLUGIN_EXPORT int
> qemu_plugin_install(qemu_plugin_id_t id,
> fprintf(stderr, "boolean argument parsing failed: %s\n",
> opt);
> return -1;
> }
> +} else if (g_strcmp0(tokens[0], "print-accesses") == 0) {
> +if (!qemu_plugin_bool_parse(tokens[0], tokens[1],
> +&do_print_accesses)) {
> +fprintf(stderr,
RE: [PATCH v4 7/7] tests/tcg/x86_64: add test for plugin memory access
> -Original Message-
> From: qemu-devel-bounces+yaoxt.fnst=fujitsu@nongnu.org
> On Behalf Of
> Pierrick Bouvier
> Sent: Wednesday, July 3, 2024 2:45 AM
> To: qemu-devel@nongnu.org
> Cc: Alex Bennée ; Mahmoud Mandour
> ; Pierrick Bouvier ;
> Alexandre Iooss ; Philippe Mathieu-Daudé
> ; Paolo Bonzini ; Richard Henderson
> ; Eduardo Habkost
> Subject: [PATCH v4 7/7] tests/tcg/x86_64: add test for plugin memory access
>
> Add an explicit test to check expected memory values are read/written.
> For sizes 8, 16, 32, 64 and 128, we generate a load/store operation.
> For size 8 -> 64, we generate an atomic __sync_val_compare_and_swap too.
> For 128bits memory access, we rely on SSE2 instructions.
>
> By default, atomic accesses are non atomic if a single cpu is running,
> so we force creation of a second one by creating a new thread first.
>
> load/store helpers code path can't be triggered easily in user mode (no
> softmmu), so we can't test it here.
>
> Can be run with:
> make -C build/tests/tcg/x86_64-linux-user
> run-plugin-test-plugin-mem-access-with-libmem.so
>
> Signed-off-by: Pierrick Bouvier
> ---
> tests/tcg/x86_64/test-plugin-mem-access.c | 89
> +
> tests/tcg/x86_64/Makefile.target| 7 ++
> tests/tcg/x86_64/check-plugin-mem-access.sh | 48 +++
> 3 files changed, 144 insertions(+)
> create mode 100644 tests/tcg/x86_64/test-plugin-mem-access.c
> create mode 100755 tests/tcg/x86_64/check-plugin-mem-access.sh
>
> diff --git a/tests/tcg/x86_64/test-plugin-mem-access.c
> b/tests/tcg/x86_64/test-plugin-mem-access.c
> new file mode 100644
> index 000..7fdd6a55829
> --- /dev/null
> +++ b/tests/tcg/x86_64/test-plugin-mem-access.c
> @@ -0,0 +1,89 @@
> +#include
> +#include
> +#include
> +#include
> +
> +static void *data;
> +
> +#define DEFINE_STORE(name, type, value) \
> +static void store_##name(void) \
> +{ \
> +*((type *)data) = value;\
> +}
> +
> +#define DEFINE_ATOMIC_OP(name, type, value) \
> +static void atomic_op_##name(void) \
> +{ \
> +*((type *)data) = 0x42; \
> +__sync_val_compare_and_swap((type *)data, 0x42, value); \
> +}
> +
> +#define DEFINE_LOAD(name, type) \
> +static void load_##name(void) \
> +{ \
> +register type var asm("eax") = *((type *) data);\
> +(void)var; \
> +}
> +
> +DEFINE_STORE(u8, uint8_t, 0xf1)
> +DEFINE_ATOMIC_OP(u8, uint8_t, 0xf1)
> +DEFINE_LOAD(u8, uint8_t)
> +DEFINE_STORE(u16, uint16_t, 0xf123)
> +DEFINE_ATOMIC_OP(u16, uint16_t, 0xf123)
> +DEFINE_LOAD(u16, uint16_t)
> +DEFINE_STORE(u32, uint32_t, 0xff112233)
> +DEFINE_ATOMIC_OP(u32, uint32_t, 0xff112233)
> +DEFINE_LOAD(u32, uint32_t)
> +DEFINE_STORE(u64, uint64_t, 0xf123456789abcdef)
> +DEFINE_ATOMIC_OP(u64, uint64_t, 0xf123456789abcdef)
> +DEFINE_LOAD(u64, uint64_t)
> +
> +static void store_u128(void)
> +{
> +_mm_store_si128(data, _mm_set_epi32(0xf1223344, 0x55667788,
> +0xf1234567, 0x89abcdef));
> +}
> +
> +static void load_u128(void)
> +{
> +__m128i var = _mm_load_si128(data);
> +(void)var;
> +}
> +
> +static void *f(void *p)
> +{
> +return NULL;
> +}
> +
> +int main(void)
> +{
> +/*
> + * We force creation of a second thread to enable cpu flag CF_PARALLEL.
> + * This will generate atomic operations when needed.
> + */
> +pthread_t thread;
> +pthread_create(&thread, NULL, &f, NULL);
> +pthread_join(thread, NULL);
> +
> +data = malloc(sizeof(__m128i));
> +atomic_op_u8();
> +store_u8();
> +load_u8();
> +
> +atomic_op_u16();
> +store_u16();
> +load_u16();
> +
> +atomic_op_u32();
> +store_u32();
> +load_u32();
> +
> +atomic_op_u64();
> +store_u64();
> +load_u64();
> +
> +store_u128();
> +load_u128();
> +
> +free(data);
> +}
> diff --git a/tests/tcg/x86_64/Makefile.target
> b/tests/tcg/x86_64/Makefile.target
> index 5fedf221174..5f7015fd8b4 100644
> --- a/tests/tcg/x86_64/Makefile.target
> +++ b/tests/tcg/x86_64/Makefile.target
> @@ -14,6 +14,7 @@ X86_64_TESTS += noexec
> X86_64_TESTS += cmpxchg
> X86_64_TESTS += adox
> X86_64_TESTS += test-1648
> +PLUGINS_TESTS += test-plugin-mem-access
> TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64
> else
> TESTS=$(MULTIARCH_TESTS)
> @@ -24,6 +25,12 @@ adox: CFLAGS=-O2
> run-test-i386-ssse3: QEMU_OPTS += -cpu max
> run-plugin-test-i386-ssse3-%: QEMU_OPTS += -cpu max
>
> +run-plugin-test-plugin-mem-access-with-libmem.so: \
> + PLUGIN_ARGS=$(COMMA)print-accesses=true
> +run-plugin-test-plugin-mem-access-with-libmem.so: \
> + CHECK_PLUGIN_OUTPUT_COMMAND= \
> + $(SRC_PATH)/tests/tcg/x86_64
RE: [PATCH] hw/nvme: Fix memory leak in nvme_dsm
> -Original Message- > From: qemu-devel-bounces+yaoxt.fnst=fujitsu@nongnu.org > On Behalf Of Zheyu > Ma > Sent: Wednesday, July 3, 2024 7:13 AM > To: Keith Busch ; Klaus Jensen ; Jesper > Devantier > Cc: Zheyu Ma ; qemu-bl...@nongnu.org; > qemu-devel@nongnu.org > Subject: [PATCH] hw/nvme: Fix memory leak in nvme_dsm > > The allocated memory to hold LBA ranges leaks in the nvme_dsm function. This > happens because the allocated memory for iocb->range is not freed in all > error handling paths. > > Fix this by adding a free to ensure that the allocated memory is properly > freed. > > ASAN log: > ==3075137==ERROR: LeakSanitizer: detected memory leaks > > Direct leak of 480 byte(s) in 6 object(s) allocated from: > #0 0x55f1f8a0eddd in malloc > llvm/compiler-rt/lib/asan/asan_malloc_linux.cpp:129:3 > #1 0x7f531e0f6738 in g_malloc > (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x5e738) > #2 0x55f1faf1f091 in blk_aio_get block/block-backend.c:2583:12 > #3 0x55f1f945c74b in nvme_dsm hw/nvme/ctrl.c:2609:30 > #4 0x55f1f945831b in nvme_io_cmd hw/nvme/ctrl.c:4470:16 > #5 0x55f1f94561b7 in nvme_process_sq hw/nvme/ctrl.c:7039:29 > > Signed-off-by: Zheyu Ma > --- > hw/nvme/ctrl.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c > index 127c3d2383..cf610eab21 100644 > --- a/hw/nvme/ctrl.c > +++ b/hw/nvme/ctrl.c > @@ -2592,6 +2592,7 @@ next: > done: > iocb->aiocb = NULL; > iocb->common.cb(iocb->common.opaque, iocb->ret); > +g_free(iocb->range); > qemu_aio_unref(iocb); > } > > -- > 2.34.1 > Reviewed-by: Xingtao Yao
RE: [PATCH v3 1/7] plugins: fix mem callback array size
> -Original Message- > From: qemu-devel-bounces+yaoxt.fnst=fujitsu@nongnu.org > On Behalf Of > Pierrick Bouvier > Sent: Tuesday, July 2, 2024 9:10 AM > To: qemu-devel@nongnu.org > Cc: Eduardo Habkost ; Alex Bennée > ; Pierrick Bouvier ; > Paolo > Bonzini ; Philippe Mathieu-Daudé ; > Alexandre Iooss ; Richard Henderson > ; Mahmoud Mandour > Subject: [PATCH v3 1/7] plugins: fix mem callback array size > > data was correctly copied, but size of array was not set > (g_array_sized_new only reserves memory, but does not set size). > > As a result, callbacks were not called for code path relying on > plugin_register_vcpu_mem_cb(). > > Found when trying to trigger mem access callbacks for atomic > instructions. > > Signed-off-by: Pierrick Bouvier > --- > accel/tcg/plugin-gen.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c > index b6bae32b997..ec89a085b43 100644 > --- a/accel/tcg/plugin-gen.c > +++ b/accel/tcg/plugin-gen.c > @@ -85,8 +85,7 @@ static void gen_enable_mem_helper(struct qemu_plugin_tb > *ptb, > len = insn->mem_cbs->len; > arr = g_array_sized_new(false, false, > sizeof(struct qemu_plugin_dyn_cb), len); > -memcpy(arr->data, insn->mem_cbs->data, > - len * sizeof(struct qemu_plugin_dyn_cb)); > +g_array_append_vals(arr, insn->mem_cbs->data, len); > qemu_plugin_add_dyn_cb_arr(arr); > > tcg_gen_st_ptr(tcg_constant_ptr((intptr_t)arr), tcg_env, > -- > 2.39.2 > Reviewed-by: Xingtao Yao
[BUG REPORT] cxl process in infinity loop
Hi, all
When I did the cxl memory hot-plug test on QEMU, I accidentally connected
two memdev to the same downstream port, the command like below:
> -object memory-backend-ram,size=262144k,share=on,id=vmem0 \
> -object memory-backend-ram,size=262144k,share=on,id=vmem1 \
> -device pxb-cxl,bus_nr=12,bus=pcie.0,id=cxl.1 \
> -device cxl-rp,port=0,bus=cxl.1,id=root_port0,chassis=0,slot=0 \
> -device cxl-upstream,bus=root_port0,id=us0 \
> -device cxl-downstream,port=0,bus=us0,id=swport00,chassis=0,slot=5 \
> -device cxl-downstream,port=0,bus=us0,id=swport01,chassis=0,slot=7 \
same downstream port but has different slot!
> -device cxl-type3,bus=swport00,volatile-memdev=vmem0,id=cxl-vmem0 \
> -device cxl-type3,bus=swport01,volatile-memdev=vmem1,id=cxl-vmem1 \
> -M
> cxl-fmw.0.targets.0=cxl.1,cxl-fmw.0.size=64G,cxl-fmw.0.interleave-granularity=4k
> \
There is no error occurred when vm start, but when I executed the “cxl list”
command to view
the CXL objects info, the process can not end properly.
Then I used strace to trace the process, I found that the process is in
infinity loop:
# strace cxl list
..
clock_nanosleep(CLOCK_REALTIME, 0, {tv_sec=0, tv_nsec=100}, NULL) = 0
openat(AT_FDCWD, "/sys/bus/cxl/flush", O_WRONLY|O_CLOEXEC) = 3
write(3, "1\n\0", 3)= 3
close(3)= 0
access("/run/udev/queue", F_OK) = 0
clock_nanosleep(CLOCK_REALTIME, 0, {tv_sec=0, tv_nsec=100}, NULL) = 0
openat(AT_FDCWD, "/sys/bus/cxl/flush", O_WRONLY|O_CLOEXEC) = 3
write(3, "1\n\0", 3)= 3
close(3)= 0
access("/run/udev/queue", F_OK) = 0
clock_nanosleep(CLOCK_REALTIME, 0, {tv_sec=0, tv_nsec=100}, NULL) = 0
openat(AT_FDCWD, "/sys/bus/cxl/flush", O_WRONLY|O_CLOEXEC) = 3
write(3, "1\n\0", 3)= 3
close(3)= 0
access("/run/udev/queue", F_OK) = 0
clock_nanosleep(CLOCK_REALTIME, 0, {tv_sec=0, tv_nsec=100}, NULL) = 0
openat(AT_FDCWD, "/sys/bus/cxl/flush", O_WRONLY|O_CLOEXEC) = 3
write(3, "1\n\0", 3)= 3
close(3)= 0
access("/run/udev/queue", F_OK) = 0
clock_nanosleep(CLOCK_REALTIME, 0, {tv_sec=0, tv_nsec=100}, NULL) = 0
openat(AT_FDCWD, "/sys/bus/cxl/flush", O_WRONLY|O_CLOEXEC) = 3
write(3, "1\n\0", 3)= 3
close(3)= 0
access("/run/udev/queue", F_OK) = 0
clock_nanosleep(CLOCK_REALTIME, 0, {tv_sec=0, tv_nsec=100}, NULL) = 0
openat(AT_FDCWD, "/sys/bus/cxl/flush", O_WRONLY|O_CLOEXEC) = 3
write(3, "1\n\0", 3)= 3
close(3)= 0
access("/run/udev/queue", F_OK) = 0
clock_nanosleep(CLOCK_REALTIME, 0, {tv_sec=0, tv_nsec=100}, NULL) = 0
openat(AT_FDCWD, "/sys/bus/cxl/flush", O_WRONLY|O_CLOEXEC) = 3
write(3, "1\n\0", 3)= 3
close(3)= 0
access("/run/udev/queue", F_OK) = 0
clock_nanosleep(CLOCK_REALTIME, 0, {tv_sec=0, tv_nsec=100}, NULL) = 0
openat(AT_FDCWD, "/sys/bus/cxl/flush", O_WRONLY|O_CLOEXEC) = 3
write(3, "1\n\0", 3)= 3
close(3)= 0
access("/run/udev/queue", F_OK) = 0
[Environment]:
linux: V6.10-rc3
QEMU: V9.0.0
ndctl: v79
I know this is because of the wrong use of the QEMU command, but I think we
should
be aware of this error in one of the QEMU, OS or ndctl side at least.
Thanks
Xingtao
RE: [PATCH] hw/misc/bcm2835_thermal: Handle invalid address accesses gracefully
Hi, zheyu > -Original Message- > From: qemu-devel-bounces+yaoxt.fnst=fujitsu@nongnu.org > On Behalf Of Zheyu > Ma > Sent: Sunday, June 30, 2024 11:14 PM > To: Peter Maydell ; Philippe Mathieu-Daudé > > Cc: Zheyu Ma ; qemu-...@nongnu.org; > qemu-devel@nongnu.org > Subject: [PATCH] hw/misc/bcm2835_thermal: Handle invalid address accesses > gracefully > > This commit handles invalid address accesses gracefully in both read and write > functions. Instead of asserting and aborting, it logs an error message and > returns > a neutral value for read operations and does nothing for write operations. > > Error log: > ERROR:hw/misc/bcm2835_thermal.c:55:bcm2835_thermal_read: code should not > be reached > Bail out! ERROR:hw/misc/bcm2835_thermal.c:55:bcm2835_thermal_read: code > should not be reached > Aborted > > Reproducer: > cat << EOF | qemu-system-aarch64 -display \ > none -machine accel=qtest, -m 512M -machine raspi3b -m 1G -qtest stdio > readw 0x3f212003 > EOF > > Signed-off-by: Zheyu Ma > --- > hw/misc/bcm2835_thermal.c | 12 > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/hw/misc/bcm2835_thermal.c b/hw/misc/bcm2835_thermal.c > index ee7816b8a5..5c2a429d58 100644 > --- a/hw/misc/bcm2835_thermal.c > +++ b/hw/misc/bcm2835_thermal.c > @@ -51,8 +51,10 @@ static uint64_t bcm2835_thermal_read(void *opaque, > hwaddr addr, unsigned size) > val = FIELD_DP32(bcm2835_thermal_temp2adc(25), STAT, VALID, true); > break; > default: > -/* MemoryRegionOps are aligned, so this can not happen. */ > -g_assert_not_reached(); > +qemu_log_mask(LOG_GUEST_ERROR, > + "bcm2835_thermal_read: invalid address 0x%" > + HWADDR_PRIx "\n", addr); > +val = 0; > } > return val; > } > @@ -72,8 +74,10 @@ static void bcm2835_thermal_write(void *opaque, hwaddr > addr, > __func__, value, addr); > break; > default: > -/* MemoryRegionOps are aligned, so this can not happen. */ > -g_assert_not_reached(); > +qemu_log_mask(LOG_GUEST_ERROR, > + "bcm2835_thermal_write: invalid address 0x%" > + HWADDR_PRIx "\n", addr); > +break; > } > } the default branch will never be reached in normal access, so I think this modification is not needed. > > -- > 2.34.1 >
RE: [PATCH] hw/display/tcx: Fix out-of-bounds access in tcx_blit_writel
Hi, zheyu
> -Original Message-
> From: qemu-devel-bounces+yaoxt.fnst=fujitsu@nongnu.org
> On Behalf Of Zheyu
> Ma
> Sent: Sunday, June 30, 2024 9:04 PM
> To: Mark Cave-Ayland
> Cc: Zheyu Ma ; qemu-devel@nongnu.org
> Subject: [PATCH] hw/display/tcx: Fix out-of-bounds access in tcx_blit_writel
>
> This patch addresses a potential out-of-bounds memory access issue in the
> tcx_blit_writel function. It adds bounds checking to ensure that memory
> accesses do not exceed the allocated VRAM size. If an out-of-bounds access
> is detected, an error is logged using qemu_log_mask.
>
> ASAN log:
> ==2960379==ERROR: AddressSanitizer: SEGV on unknown address
> 0x7f524752fd01 (pc 0x7f525c2c4881 bp 0x7ffdaf87bfd0 sp 0x7ffdaf87b788 T0)
> ==2960379==The signal is caused by a READ memory access.
> #0 0x7f525c2c4881 in memcpy
> string/../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:222
> #1 0x55aa782bd5b1 in __asan_memcpy
> llvm/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:22:3
> #2 0x55aa7854dedd in tcx_blit_writel hw/display/tcx.c:590:13
>
> Reproducer:
> cat << EOF | qemu-system-sparc -display none \
> -machine accel=qtest, -m 512M -machine LX -m 256 -qtest stdio
> writel 0x562e98c4 0x3d92fd01
> EOF
>
> Signed-off-by: Zheyu Ma
> ---
> hw/display/tcx.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> index 99507e7638..af43bea7f2 100644
> --- a/hw/display/tcx.c
> +++ b/hw/display/tcx.c
> @@ -33,6 +33,7 @@
> #include "migration/vmstate.h"
> #include "qemu/error-report.h"
> #include "qemu/module.h"
> +#include "qemu/log.h"
> #include "qom/object.h"
>
> #define TCX_ROM_FILE "QEMU,tcx.bin"
> @@ -577,6 +578,14 @@ static void tcx_blit_writel(void *opaque, hwaddr addr,
> addr = (addr >> 3) & 0xf;
> adsr = val & 0xff;
> len = ((val >> 24) & 0x1f) + 1;
> +
> +if (addr + len > s->vram_size || adsr + len > s->vram_size) {
if adsr == 0xff, this condition may be always true, thus the branch “if
(adsr == 0xff) {” will be never
reached.
> +qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: VRAM access out of bounds. addr: 0x%lx, adsr:
> 0x%x, len: %u\n",
> + __func__, addr, adsr, len);
> +return;
> +}
> +
> if (adsr == 0xff) {
> memset(&s->vram[addr], s->tmpblit, len);
> if (s->depth == 24) {
> --
> 2.34.1
>
RE: [PATCH] hw/usb: Fix memory leak in musb_reset()
> -Original Message- > From: qemu-devel-bounces+yaoxt.fnst=fujitsu@nongnu.org > On Behalf Of Zheyu > Ma > Sent: Monday, July 1, 2024 12:32 AM > Cc: Zheyu Ma ; qemu-devel@nongnu.org > Subject: [PATCH] hw/usb: Fix memory leak in musb_reset() > > The musb_reset function was causing a memory leak by not properly freeing > the memory associated with USBPacket instances before reinitializing them. > This commit addresses the memory leak by adding calls to usb_packet_cleanup > for each USBPacket instance before reinitializing them with usb_packet_init. > > Asan log: > > =2970623==ERROR: LeakSanitizer: detected memory leaks > Direct leak of 256 byte(s) in 16 object(s) allocated from: > #0 0x561e20629c3d in malloc > llvm/compiler-rt/lib/asan/asan_malloc_linux.cpp:129:3 > #1 0x7fee91885738 in g_malloc > (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x5e738) > #2 0x561e21b4d0e1 in usb_packet_init hw/usb/core.c:531:5 > #3 0x561e21c5016b in musb_reset hw/usb/hcd-musb.c:372:9 > #4 0x561e21c502a9 in musb_init hw/usb/hcd-musb.c:385:5 > #5 0x561e21c893ef in tusb6010_realize hw/usb/tusb6010.c:827:15 > #6 0x561e23443355 in device_set_realized hw/core/qdev.c:510:13 > #7 0x561e2346ac1b in property_set_bool qom/object.c:2354:5 > #8 0x561e23463895 in object_property_set qom/object.c:1463:5 > #9 0x561e23477909 in object_property_set_qobject qom/qom-qobject.c:28:10 > #10 0x561e234645ed in object_property_set_bool qom/object.c:1533:15 > #11 0x561e2343c830 in qdev_realize hw/core/qdev.c:291:12 > #12 0x561e2343c874 in qdev_realize_and_unref hw/core/qdev.c:298:11 > #13 0x561e20ad5091 in sysbus_realize_and_unref hw/core/sysbus.c:261:12 > #14 0x561e22553283 in n8x0_usb_setup hw/arm/nseries.c:800:5 > #15 0x561e2254e99b in n8x0_init hw/arm/nseries.c:1356:5 > #16 0x561e22561170 in n810_init hw/arm/nseries.c:1418:5 > > Signed-off-by: Zheyu Ma > --- > hw/usb/hcd-musb.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/hw/usb/hcd-musb.c b/hw/usb/hcd-musb.c > index 6dca373cb1..0300aeaec6 100644 > --- a/hw/usb/hcd-musb.c > +++ b/hw/usb/hcd-musb.c > @@ -368,6 +368,8 @@ void musb_reset(MUSBState *s) > s->ep[i].maxp[1] = 0x40; > s->ep[i].musb = s; > s->ep[i].epnum = i; > +usb_packet_cleanup(&s->ep[i].packey[0].p); > +usb_packet_cleanup(&s->ep[i].packey[1].p); > usb_packet_init(&s->ep[i].packey[0].p); > usb_packet_init(&s->ep[i].packey[1].p); > } > -- > 2.34.1 > Reviewed-by: Xingtao Yao
RE: [PATCH v3] hw/core/loader: allow loading larger ROMs
> -Original Message-
> From: Gregor Haas
> Sent: Friday, June 28, 2024 9:51 AM
> To: qemu-devel@nongnu.org
> Cc: Yao, Xingtao/姚 幸涛 ; Gregor Haas
>
> Subject: [PATCH v3] hw/core/loader: allow loading larger ROMs
>
> The read() syscall is not guaranteed to return all data from a file. The
> default ROM loader implementation currently does not take this into account,
> instead failing if all bytes are not read at once. This change loads the ROM
> using load_image_size() instead, which correctly reads all data using
> multiple calls to read(). Also, the ROM size is now determined using the
> get_image_size() function rather than using manual lseek().
>
> Signed-off-by: Gregor Haas
> ---
> hw/core/loader.c | 17 ++---
> 1 file changed, 2 insertions(+), 15 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 2f8105d7de..c2c61158f1 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1076,7 +1076,6 @@ ssize_t rom_add_file(const char *file, const char
> *fw_dir,
> MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
> Rom *rom;
> ssize_t rc;
> -int fd = -1;
> char devpath[100];
>
> if (as && mr) {
> @@ -1094,19 +1093,12 @@ ssize_t rom_add_file(const char *file, const char
> *fw_dir,
> rom->path = g_strdup(file);
> }
>
> -fd = open(rom->path, O_RDONLY | O_BINARY);
> -if (fd == -1) {
> -fprintf(stderr, "Could not open option rom '%s': %s\n",
> -rom->path, strerror(errno));
> -goto err;
> -}
> -
> if (fw_dir) {
> rom->fw_dir = g_strdup(fw_dir);
> rom->fw_file = g_strdup(file);
> }
> rom->addr = addr;
> -rom->romsize = lseek(fd, 0, SEEK_END);
> +rom->romsize = get_image_size(rom->path);
> if (rom->romsize == -1) {
> fprintf(stderr, "rom: file %-20s: get size error: %s\n",
> rom->name, strerror(errno));
> @@ -1115,14 +1107,12 @@ ssize_t rom_add_file(const char *file, const char
> *fw_dir,
>
> rom->datasize = rom->romsize;
> rom->data = g_malloc0(rom->datasize);
> -lseek(fd, 0, SEEK_SET);
> -rc = read(fd, rom->data, rom->datasize);
> +rc = load_image_size(rom->path, rom->data, rom->datasize);
> if (rc != rom->datasize) {
> fprintf(stderr, "rom: file %-20s: read error: rc=%zd (expected
> %zd)\n",
> rom->name, rc, rom->datasize);
> goto err;
> }
> -close(fd);
> rom_insert(rom);
> if (rom->fw_file && fw_cfg) {
> const char *basename;
> @@ -1159,9 +1149,6 @@ ssize_t rom_add_file(const char *file, const char
> *fw_dir,
> return 0;
>
> err:
> -if (fd != -1)
> -close(fd);
> -
> rom_free(rom);
> return -1;
> }
> --
> 2.45.2
Reviewed-by: Xingtao Yao
RE: [PATCH v2] hw/core/loader: allow loading larger ROMs
> -Original Message-
> From: Gregor Haas
> Sent: Friday, June 28, 2024 8:58 AM
> To: qemu-devel@nongnu.org
> Cc: Yao, Xingtao/姚 幸涛 ; Gregor Haas
>
> Subject: [PATCH v2] hw/core/loader: allow loading larger ROMs
>
> The read() syscall is not guaranteed to return all data from a file. The
> default ROM loader implementation currently does not take this into account,
> instead failing if all bytes are not read at once. This change loads the ROM
> using load_image_size() instead, which correctly reads all data using
> multiple calls to read().
>
> Signed-off-by: Gregor Haas
> ---
> hw/core/loader.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 2f8105d7de..8216781a75 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1115,14 +1115,13 @@ ssize_t rom_add_file(const char *file, const char
> *fw_dir,
>
> rom->datasize = rom->romsize;
> rom->data = g_malloc0(rom->datasize);
> -lseek(fd, 0, SEEK_SET);
> -rc = read(fd, rom->data, rom->datasize);
> +close(fd);
> +rc = load_image_size(rom->path, rom->data, rom->datasize);
LGTM.
I think we may get romsize by get_image_size() and delete the redundant code
like below:
-fd = open(rom->path, O_RDONLY | O_BINARY);
-if (fd == -1) {
-fprintf(stderr, "Could not open option rom '%s': %s\n",
-rom->path, strerror(errno));
+rom->romesize = get_image_size(rom->path);
+if (rom->romsize == -1) {
+fprintf(stderr, "rom: file %-20s: get size error: %s\n",
+rom->name, strerror(errno));
goto err;
}
@@ -1106,16 +1106,9 @@ ssize_t rom_add_file(const char *file, const char
*fw_dir,
rom->fw_file = g_strdup(file);
}
rom->addr = addr;
-rom->romsize = lseek(fd, 0, SEEK_END);
-if (rom->romsize == -1) {
-fprintf(stderr, "rom: file %-20s: get size error: %s\n",
-rom->name, strerror(errno));
-goto err;
-}
rom->datasize = rom->romsize;
rom->data = g_malloc0(rom->datasize);
-lseek(fd, 0, SEEK_SET);
> if (rc != rom->datasize) {
> fprintf(stderr, "rom: file %-20s: read error: rc=%zd (expected
> %zd)\n",
> rom->name, rc, rom->datasize);
> goto err;
> }
> -close(fd);
> rom_insert(rom);
> if (rom->fw_file && fw_cfg) {
> const char *basename;
> --
> 2.45.2
RE: [PATCH] hw/core/loader: allow loading larger ROMs
Hi, Gregor
>rom: file fw_payload.bin: read error: rc=2147479552 (expected 2303760392)
>qemu-system-riscv64: could not load firmware 'fw_payload.bin'
Thanks, I was able to reproduce the problem when the images size is larger than
2147479552.
I found that in my test environment, the maximum value returned by a read
operation is 2147479552,
which was affected by the operating system.
We can find this limitation in the man page:
NOTES
The types size_t and ssize_t are, respectively, unsigned and signed
integer data types specified by POSIX.1.
On Linux, read() (and similar system calls) will transfer at most
0x7000 (2,147,479,552) bytes, returning the number of bytes actually
transferred. (This is true on both
32-bit and 64-bit systems.)
> +do {
> +rc = read(fd, &rom->data[sz], rom->datasize);
> +if (rc == -1) {
> +fprintf(stderr, "rom: file %-20s: read error: %s\n",
> +rom->name, strerror(errno));
> +goto err;
> +}
> +sz += rc;
> +} while (sz != rom->datasize);
I think we can use load_image_size() instead.
From: Gregor Haas
Sent: Friday, June 28, 2024 1:35 AM
To: Yao, Xingtao/姚 幸涛
Cc: qemu-devel@nongnu.org; phi...@linaro.org; richard.hender...@linaro.org
Subject: Re: [PATCH] hw/core/loader: allow loading larger ROMs
Hi Xingtao,
> Can you reproduce this issue?
Absolutely! I encountered this when trying to load an OpenSBI payload
firmware using the bios option for the QEMU RISC-V virt board. These
payload firmwares bundle the entire next boot stage, which in my case is a
build of the Linux kernel (which is a standard configuration, supported by
tools such as Buildroot [1]). My kernel (configured with the default 64-bit
RISC-V configuration) comes in at 9.8M, which is copied into the OpenSBI
firmware of final size 10M. Then, I run the following QEMU command:
qemu-system-riscv64 -machine virt -m 4G -nographic -bios fw_payload.bin
and get the following output:
rom: file fw_payload.bin: read error: rc=2147479552 (expected 2303760392)
qemu-system-riscv64: could not load firmware 'fw_payload.bin'
This is from my development machine, running Arch Linux with kernel 6.9.6
and root filesystem ZFS 2.2.4. Please let me know if you'd like me to make
a minimal reproducer for this, or if you need any more information.
Thanks,
Gregor
[1]
https://github.com/buildroot/buildroot/blob/master/boot/opensbi/Config.in#L95
On Wed, Jun 26, 2024 at 11:11 PM Xingtao Yao (Fujitsu)
mailto:yaoxt.f...@fujitsu.com>> wrote:
Hi, Gregor
>
> The read() syscall is not guaranteed to return all data from a file. The
> default ROM loader implementation currently does not take this into account,
> instead failing if all bytes are not read at once. This change wraps the
> read() syscall in a do/while loop to ensure all bytes of the ROM are read.
Can you reproduce this issue?
Thanks
Xingtao
RE: [PATCH] hw/core/loader: allow loading larger ROMs
Hi, Gregor > > The read() syscall is not guaranteed to return all data from a file. The > default ROM loader implementation currently does not take this into account, > instead failing if all bytes are not read at once. This change wraps the > read() syscall in a do/while loop to ensure all bytes of the ROM are read. Can you reproduce this issue? Thanks Xingtao
RE: [PATCH v2 6/7] tests/plugin/mem: add option to print memory accesses
> -Original Message-
> From: Pierrick Bouvier
> Sent: Thursday, June 27, 2024 1:29 PM
> To: Yao, Xingtao/姚 幸涛 ; qemu-devel@nongnu.org
> Cc: Alexandre Iooss ; Philippe Mathieu-Daudé
> ; Mahmoud Mandour ; Paolo
> Bonzini ; Eduardo Habkost ;
> Richard Henderson ; Alex Bennée
>
> Subject: Re: [PATCH v2 6/7] tests/plugin/mem: add option to print memory
> accesses
>
> Hi Xingtao,
>
> On 6/26/24 20:17, Xingtao Yao (Fujitsu) wrote:
> > Hi, Pierrick
> >
> >> +static void print_access(unsigned int cpu_index, qemu_plugin_meminfo_t
> >> meminfo,
> >> + uint64_t vaddr, void *udata)
> >> +{
> >> +unsigned size = 8 << qemu_plugin_mem_size_shift(meminfo);
> >> +const char *type = qemu_plugin_mem_is_store(meminfo) ? "store" :
> "load";
> >> +uint64_t upper = qemu_plugin_mem_get_value_upper_bits(meminfo);
> >> +uint64_t lower = qemu_plugin_mem_get_value_lower_bits(meminfo);
> >> +const char *sym = udata ? udata : "";
> >> +g_autoptr(GString) out = g_string_new("");
> >> +g_string_printf(out, "access: 0x%.0"PRIx64"%"PRIx64",%d,%s,%s\n",
> >> +upper, lower, size, type, sym);
> >> +qemu_plugin_outs(out->str);
> >> +}
> > I think it may be helpful to output the GVA and GPA, can you append these
> information?
> >
>
> You mean virtual and physical addresses?
Yes. currently we only known the memory value, appending these info may help us
to trace the
memory access.
>
> >
> > Thanks
> > Xingtao
RE: [PATCH v2 6/7] tests/plugin/mem: add option to print memory accesses
Hi, Pierrick
> +static void print_access(unsigned int cpu_index, qemu_plugin_meminfo_t
> meminfo,
> + uint64_t vaddr, void *udata)
> +{
> +unsigned size = 8 << qemu_plugin_mem_size_shift(meminfo);
> +const char *type = qemu_plugin_mem_is_store(meminfo) ? "store" : "load";
> +uint64_t upper = qemu_plugin_mem_get_value_upper_bits(meminfo);
> +uint64_t lower = qemu_plugin_mem_get_value_lower_bits(meminfo);
> +const char *sym = udata ? udata : "";
> +g_autoptr(GString) out = g_string_new("");
> +g_string_printf(out, "access: 0x%.0"PRIx64"%"PRIx64",%d,%s,%s\n",
> +upper, lower, size, type, sym);
> +qemu_plugin_outs(out->str);
> +}
I think it may be helpful to output the GVA and GPA, can you append these
information?
Thanks
Xingtao
RE: [PATCH v3] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
> -Original Message-
> From: Jonathan Cameron
> Sent: Friday, June 21, 2024 11:02 PM
> To: Yao, Xingtao/姚 幸涛
> Cc: fan...@samsung.com; qemu-devel@nongnu.org
> Subject: Re: [PATCH v3] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
>
> On Thu, 6 Jun 2024 08:07:27 +
> "Xingtao Yao (Fujitsu)" wrote:
>
> > ping again.
>
> Sorry for delay - I was waiting for some of the tree I'm carrying
> to get picked up before applying anything new.
>
> I haven't yet tested this as fully as I'd like for upstreaming, but
> with that in mind applied to my cxl staging tree.
many thanks, I can do some test if you need.
>
> Thanks,
>
> Jonathan
>
> >
> > > -Original Message-
> > > From: Yao, Xingtao/姚 幸涛
> > > Sent: Friday, May 24, 2024 5:31 PM
> > > To: Yao, Xingtao/姚 幸涛 ;
> > > jonathan.came...@huawei.com; fan...@samsung.com
> > > Cc: qemu-devel@nongnu.org
> > > Subject: RE: [PATCH v3] mem/cxl_type3: support 3, 6, 12 and 16 interleave
> ways
> > >
> > > ping.
> > >
> > > > -Original Message-
> > > > From: Yao Xingtao
> > > > Sent: Wednesday, May 8, 2024 8:53 AM
> > > > To: jonathan.came...@huawei.com; fan...@samsung.com
> > > > Cc: qemu-devel@nongnu.org; Yao, Xingtao/姚 幸涛
>
> > > > Subject: [PATCH v3] mem/cxl_type3: support 3, 6, 12 and 16 interleave
> > > > ways
> > > >
> > > > Since the kernel does not check the interleave capability, a
> > > > 3-way, 6-way, 12-way or 16-way region can be create normally.
> > > >
> > > > Applications can access the memory of 16-way region normally because
> > > > qemu can convert hpa to dpa correctly for the power of 2 interleave
> > > > ways, after kernel implementing the check, this kind of region will
> > > > not be created any more.
> > > >
> > > > For non power of 2 interleave ways, applications could not access the
> > > > memory normally and may occur some unexpected behaviors, such as
> > > > segmentation fault.
> > > >
> > > > So implements this feature is needed.
> > > >
> > > > Link:
> > > >
> > >
> https://lore.kernel.org/linux-cxl/3e84b919-7631-d1db-3e1d-33000f3f3868@fujits
> > > > u.com/
> > > > Signed-off-by: Yao Xingtao
> > > > ---
> > > > hw/cxl/cxl-component-utils.c | 9 +++--
> > > > hw/mem/cxl_type3.c | 15 +++
> > > > 2 files changed, 18 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > > > index cd116c0401..473895948b 100644
> > > > --- a/hw/cxl/cxl-component-utils.c
> > > > +++ b/hw/cxl/cxl-component-utils.c
> > > > @@ -243,8 +243,13 @@ static void hdm_init_common(uint32_t *reg_state,
> > > > uint32_t *write_msk,
> > > > ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > > > INTERLEAVE_4K, 1);
> > > > ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > > > POISON_ON_ERR_CAP, 0);
> > > > -ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > > > 3_6_12_WAY, 0);
> > > > -ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > > 16_WAY,
> > > > 0);
> > > > +if (type == CXL2_TYPE3_DEVICE) {
> > > > +ARRAY_FIELD_DP32(reg_state,
> CXL_HDM_DECODER_CAPABILITY,
> > > > 3_6_12_WAY, 1);
> > > > +ARRAY_FIELD_DP32(reg_state,
> CXL_HDM_DECODER_CAPABILITY,
> > > > 16_WAY, 1);
> > > > +} else {
> > > > +ARRAY_FIELD_DP32(reg_state,
> CXL_HDM_DECODER_CAPABILITY,
> > > > 3_6_12_WAY, 0);
> > > > +ARRAY_FIELD_DP32(reg_state,
> CXL_HDM_DECODER_CAPABILITY,
> > > > 16_WAY, 0);
> > > > +}
> > > > ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> UIO,
> > > 0);
> > > > ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > > > UIO_DECODER_COUNT, 0);
> > > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > > > index 3e42490b6c..b755318838 100644
> > > > --- a/hw/mem/cxl_type3.c
> > > > +++ b/hw/mem/cxl_type3.c
> > > > @@ -804,10 +804,17 @@ static bool cxl_type3_dpa(CXLType3Dev *ct3d,
> hwaddr
> > > > host_addr, uint64_t *dpa)
> > > > continue;
> > > > }
> > > >
> > > > -*dpa = dpa_base +
> > > > -((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> > > > - ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) &
> hpa_offset)
> > > > - >> iw));
> > > > +if (iw < 8) {
> > > > +*dpa = dpa_base +
> > > > +((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> > > > + ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) &
> > > hpa_offset)
> > > > + >> iw));
> > > > +} else {
> > > > +*dpa = dpa_base +
> > > > +((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> > > > + MAKE_64BIT_MASK(ig + iw, 64 - ig - iw) &
> hpa_offset)
> > > > + >> (ig + iw)) / 3) << (ig + 8)));
> > > > +}
> > > >
> > > > return true;
> > > > }
> > > > --
> > > > 2.37.3
> >
> >
RE: [PATCH v3] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
ping again.
> -Original Message-
> From: Yao, Xingtao/姚 幸涛
> Sent: Friday, May 24, 2024 5:31 PM
> To: Yao, Xingtao/姚 幸涛 ;
> jonathan.came...@huawei.com; fan...@samsung.com
> Cc: qemu-devel@nongnu.org
> Subject: RE: [PATCH v3] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
>
> ping.
>
> > -Original Message-
> > From: Yao Xingtao
> > Sent: Wednesday, May 8, 2024 8:53 AM
> > To: jonathan.came...@huawei.com; fan...@samsung.com
> > Cc: qemu-devel@nongnu.org; Yao, Xingtao/姚 幸涛
> > Subject: [PATCH v3] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
> >
> > Since the kernel does not check the interleave capability, a
> > 3-way, 6-way, 12-way or 16-way region can be create normally.
> >
> > Applications can access the memory of 16-way region normally because
> > qemu can convert hpa to dpa correctly for the power of 2 interleave
> > ways, after kernel implementing the check, this kind of region will
> > not be created any more.
> >
> > For non power of 2 interleave ways, applications could not access the
> > memory normally and may occur some unexpected behaviors, such as
> > segmentation fault.
> >
> > So implements this feature is needed.
> >
> > Link:
> >
> https://lore.kernel.org/linux-cxl/3e84b919-7631-d1db-3e1d-33000f3f3868@fujits
> > u.com/
> > Signed-off-by: Yao Xingtao
> > ---
> > hw/cxl/cxl-component-utils.c | 9 +++--
> > hw/mem/cxl_type3.c | 15 +++
> > 2 files changed, 18 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> > index cd116c0401..473895948b 100644
> > --- a/hw/cxl/cxl-component-utils.c
> > +++ b/hw/cxl/cxl-component-utils.c
> > @@ -243,8 +243,13 @@ static void hdm_init_common(uint32_t *reg_state,
> > uint32_t *write_msk,
> > ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > INTERLEAVE_4K, 1);
> > ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > POISON_ON_ERR_CAP, 0);
> > -ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > 3_6_12_WAY, 0);
> > -ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> 16_WAY,
> > 0);
> > +if (type == CXL2_TYPE3_DEVICE) {
> > +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > 3_6_12_WAY, 1);
> > +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > 16_WAY, 1);
> > +} else {
> > +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > 3_6_12_WAY, 0);
> > +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > 16_WAY, 0);
> > +}
> > ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, UIO,
> 0);
> > ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > UIO_DECODER_COUNT, 0);
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index 3e42490b6c..b755318838 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -804,10 +804,17 @@ static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr
> > host_addr, uint64_t *dpa)
> > continue;
> > }
> >
> > -*dpa = dpa_base +
> > -((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> > - ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) & hpa_offset)
> > - >> iw));
> > +if (iw < 8) {
> > +*dpa = dpa_base +
> > +((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> > + ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) &
> hpa_offset)
> > + >> iw));
> > +} else {
> > +*dpa = dpa_base +
> > +((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> > + MAKE_64BIT_MASK(ig + iw, 64 - ig - iw) & hpa_offset)
> > + >> (ig + iw)) / 3) << (ig + 8)));
> > +}
> >
> > return true;
> > }
> > --
> > 2.37.3
RE: [PATCH v3] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
ping.
> -Original Message-
> From: Yao Xingtao
> Sent: Wednesday, May 8, 2024 8:53 AM
> To: jonathan.came...@huawei.com; fan...@samsung.com
> Cc: qemu-devel@nongnu.org; Yao, Xingtao/姚 幸涛
> Subject: [PATCH v3] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
>
> Since the kernel does not check the interleave capability, a
> 3-way, 6-way, 12-way or 16-way region can be create normally.
>
> Applications can access the memory of 16-way region normally because
> qemu can convert hpa to dpa correctly for the power of 2 interleave
> ways, after kernel implementing the check, this kind of region will
> not be created any more.
>
> For non power of 2 interleave ways, applications could not access the
> memory normally and may occur some unexpected behaviors, such as
> segmentation fault.
>
> So implements this feature is needed.
>
> Link:
> https://lore.kernel.org/linux-cxl/3e84b919-7631-d1db-3e1d-33000f3f3868@fujits
> u.com/
> Signed-off-by: Yao Xingtao
> ---
> hw/cxl/cxl-component-utils.c | 9 +++--
> hw/mem/cxl_type3.c | 15 +++
> 2 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/hw/cxl/cxl-component-utils.c b/hw/cxl/cxl-component-utils.c
> index cd116c0401..473895948b 100644
> --- a/hw/cxl/cxl-component-utils.c
> +++ b/hw/cxl/cxl-component-utils.c
> @@ -243,8 +243,13 @@ static void hdm_init_common(uint32_t *reg_state,
> uint32_t *write_msk,
> ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> INTERLEAVE_4K, 1);
> ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> POISON_ON_ERR_CAP, 0);
> -ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> 3_6_12_WAY, 0);
> -ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, 16_WAY,
> 0);
> +if (type == CXL2_TYPE3_DEVICE) {
> +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> 3_6_12_WAY, 1);
> +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> 16_WAY, 1);
> +} else {
> +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> 3_6_12_WAY, 0);
> +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> 16_WAY, 0);
> +}
> ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY, UIO, 0);
> ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> UIO_DECODER_COUNT, 0);
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 3e42490b6c..b755318838 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -804,10 +804,17 @@ static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr
> host_addr, uint64_t *dpa)
> continue;
> }
>
> -*dpa = dpa_base +
> -((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> - ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) & hpa_offset)
> - >> iw));
> +if (iw < 8) {
> +*dpa = dpa_base +
> +((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> + ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) &
> hpa_offset)
> + >> iw));
> +} else {
> +*dpa = dpa_base +
> +((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> + MAKE_64BIT_MASK(ig + iw, 64 - ig - iw) & hpa_offset)
> + >> (ig + iw)) / 3) << (ig + 8)));
> +}
>
> return true;
> }
> --
> 2.37.3
RE: [PATCH v2] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
> -Original Message-
> From: Jonathan Cameron
> Sent: Wednesday, May 8, 2024 12:31 AM
> To: Yao, Xingtao/姚 幸涛
> Cc: fan...@samsung.com; qemu-devel@nongnu.org
> Subject: Re: [PATCH v2] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
>
> On Tue, 7 May 2024 00:22:00 +
> "Xingtao Yao (Fujitsu)" wrote:
>
> > > -Original Message-
> > > From: Jonathan Cameron
> > > Sent: Tuesday, April 30, 2024 10:43 PM
> > > To: Yao, Xingtao/姚 幸涛
> > > Cc: fan...@samsung.com; qemu-devel@nongnu.org
> > > Subject: Re: [PATCH v2] mem/cxl_type3: support 3, 6, 12 and 16 interleave
> ways
> > >
> > > On Wed, 24 Apr 2024 01:36:56 +
> > > "Xingtao Yao (Fujitsu)" wrote:
> > >
> > > > ping.
> > > >
> > > > > -Original Message-
> > > > > From: Yao Xingtao
> > > > > Sent: Sunday, April 7, 2024 11:07 AM
> > > > > To: jonathan.came...@huawei.com; fan...@samsung.com
> > > > > Cc: qemu-devel@nongnu.org; Yao, Xingtao/姚 幸涛
> > >
> > > > > Subject: [PATCH v2] mem/cxl_type3: support 3, 6, 12 and 16 interleave
> ways
> > > > >
> > > > > Since the kernel does not check the interleave capability, a
> > > > > 3-way, 6-way, 12-way or 16-way region can be create normally.
> > > > >
> > > > > Applications can access the memory of 16-way region normally because
> > > > > qemu can convert hpa to dpa correctly for the power of 2 interleave
> > > > > ways, after kernel implementing the check, this kind of region will
> > > > > not be created any more.
> > > > >
> > > > > For non power of 2 interleave ways, applications could not access the
> > > > > memory normally and may occur some unexpected behaviors, such as
> > > > > segmentation fault.
> > > > >
> > > > > So implements this feature is needed.
> > > > >
> > > > > Link:
> > > > >
> > >
> https://lore.kernel.org/linux-cxl/3e84b919-7631-d1db-3e1d-33000f3f3868@fujits
> > > > > u.com/
> > > > > Signed-off-by: Yao Xingtao
> > > > > ---
> > > > > hw/mem/cxl_type3.c | 18 ++
> > > > > 1 file changed, 14 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > > > > index b0a7e9f11b..d6ef784e96 100644
> > > > > --- a/hw/mem/cxl_type3.c
> > > > > +++ b/hw/mem/cxl_type3.c
> > > > > @@ -805,10 +805,17 @@ static bool cxl_type3_dpa(CXLType3Dev *ct3d,
> > > hwaddr
> > > > > host_addr, uint64_t *dpa)
> > > > > continue;
> > > > > }
> > > > >
> > > > > -*dpa = dpa_base +
> > > > > -((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> > > > > - ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) &
> hpa_offset)
> > > > > - >> iw));
> > > > > +if (iw < 8) {
> > > > > +*dpa = dpa_base +
> > > > > +((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> > > > > + ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) &
> > > hpa_offset)
> > > > > + >> iw));
> > > > > +} else {
> > > > > +*dpa = dpa_base +
> > > > > +((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> > > > > + MAKE_64BIT_MASK(ig + iw, 64 - ig - iw) &
> hpa_offset)
> > > > > + >> (ig + iw)) / 3) << (ig + 8)));
> > > > > +}
> > > > >
> > > > > return true;
> > > > > }
> > > > > @@ -906,6 +913,9 @@ static void ct3d_reset(DeviceState *dev)
> > > > > uint32_t *write_msk =
> > > ct3d->cxl_cstate.crb.cache_mem_regs_write_mask;
> > > > >
> > > > > cxl_component_register_init_common(reg_state, write_msk,
> > > > > CXL2_TYPE3_DEVICE);
> > > > > +ARRAY_FIELD_DP32(reg_state,
> CXL_HDM_DECODER_CAPABILITY,
> > > > > 3_6_12_WAY, 1);
> > > > > +ARRAY_FIELD_DP32(reg_state,
> CXL_HDM_DECODER_CAPABILITY,
> > > > > 16_WAY, 1);
> > > > > +
> > >
> > > Why here rather than in hdm_reg_init_common()?
> > > It's constant data and is currently being set to 0 in there.
> >
> > according to the CXL specifications (8.2.4.20.1 CXL HDM Decoder Capability
> Register (Offset 00h)),
> > this feature is only applicable to cxl.mem, upstream switch port and CXL
> > host
> bridges shall hardwrite
> > these bits to 0.
> >
> > so I think it would be more appropriate to set these bits here.
> I don't follow. hdm_init_common() (sorry wrong function name above)
> has some type specific stuff already to show how this can be done.
> I'd prefer to minimize what we set directly in the ct3d_reset() call
> because it loses the connection to the rest of the register setup.
thanks, got it.
>
> Jonathan
>
>
>
> Jonathan
>
>
> >
> > >
> > > > > cxl_device_register_init_t3(ct3d);
> > > > >
> > > > > /*
> > > > > --
> > > > > 2.37.3
> > > >
> >
RE: [PATCH v2] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
> -Original Message-
> From: Jonathan Cameron
> Sent: Tuesday, April 30, 2024 10:43 PM
> To: Yao, Xingtao/姚 幸涛
> Cc: fan...@samsung.com; qemu-devel@nongnu.org
> Subject: Re: [PATCH v2] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
>
> On Wed, 24 Apr 2024 01:36:56 +
> "Xingtao Yao (Fujitsu)" wrote:
>
> > ping.
> >
> > > -Original Message-
> > > From: Yao Xingtao
> > > Sent: Sunday, April 7, 2024 11:07 AM
> > > To: jonathan.came...@huawei.com; fan...@samsung.com
> > > Cc: qemu-devel@nongnu.org; Yao, Xingtao/姚 幸涛
>
> > > Subject: [PATCH v2] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
> > >
> > > Since the kernel does not check the interleave capability, a
> > > 3-way, 6-way, 12-way or 16-way region can be create normally.
> > >
> > > Applications can access the memory of 16-way region normally because
> > > qemu can convert hpa to dpa correctly for the power of 2 interleave
> > > ways, after kernel implementing the check, this kind of region will
> > > not be created any more.
> > >
> > > For non power of 2 interleave ways, applications could not access the
> > > memory normally and may occur some unexpected behaviors, such as
> > > segmentation fault.
> > >
> > > So implements this feature is needed.
> > >
> > > Link:
> > >
> https://lore.kernel.org/linux-cxl/3e84b919-7631-d1db-3e1d-33000f3f3868@fujits
> > > u.com/
> > > Signed-off-by: Yao Xingtao
> > > ---
> > > hw/mem/cxl_type3.c | 18 ++
> > > 1 file changed, 14 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > > index b0a7e9f11b..d6ef784e96 100644
> > > --- a/hw/mem/cxl_type3.c
> > > +++ b/hw/mem/cxl_type3.c
> > > @@ -805,10 +805,17 @@ static bool cxl_type3_dpa(CXLType3Dev *ct3d,
> hwaddr
> > > host_addr, uint64_t *dpa)
> > > continue;
> > > }
> > >
> > > -*dpa = dpa_base +
> > > -((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> > > - ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) &
> > > hpa_offset)
> > > - >> iw));
> > > +if (iw < 8) {
> > > +*dpa = dpa_base +
> > > +((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> > > + ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) &
> hpa_offset)
> > > + >> iw));
> > > +} else {
> > > +*dpa = dpa_base +
> > > +((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> > > + MAKE_64BIT_MASK(ig + iw, 64 - ig - iw) & hpa_offset)
> > > + >> (ig + iw)) / 3) << (ig + 8)));
> > > +}
> > >
> > > return true;
> > > }
> > > @@ -906,6 +913,9 @@ static void ct3d_reset(DeviceState *dev)
> > > uint32_t *write_msk =
> ct3d->cxl_cstate.crb.cache_mem_regs_write_mask;
> > >
> > > cxl_component_register_init_common(reg_state, write_msk,
> > > CXL2_TYPE3_DEVICE);
> > > +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > > 3_6_12_WAY, 1);
> > > +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> > > 16_WAY, 1);
> > > +
>
> Why here rather than in hdm_reg_init_common()?
> It's constant data and is currently being set to 0 in there.
according to the CXL specifications (8.2.4.20.1 CXL HDM Decoder Capability
Register (Offset 00h)),
this feature is only applicable to cxl.mem, upstream switch port and CXL host
bridges shall hardwrite
these bits to 0.
so I think it would be more appropriate to set these bits here.
>
> > > cxl_device_register_init_t3(ct3d);
> > >
> > > /*
> > > --
> > > 2.37.3
> >
RE: [PATCH v2] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
ping.
> -Original Message-
> From: Yao Xingtao
> Sent: Sunday, April 7, 2024 11:07 AM
> To: jonathan.came...@huawei.com; fan...@samsung.com
> Cc: qemu-devel@nongnu.org; Yao, Xingtao/姚 幸涛
> Subject: [PATCH v2] mem/cxl_type3: support 3, 6, 12 and 16 interleave ways
>
> Since the kernel does not check the interleave capability, a
> 3-way, 6-way, 12-way or 16-way region can be create normally.
>
> Applications can access the memory of 16-way region normally because
> qemu can convert hpa to dpa correctly for the power of 2 interleave
> ways, after kernel implementing the check, this kind of region will
> not be created any more.
>
> For non power of 2 interleave ways, applications could not access the
> memory normally and may occur some unexpected behaviors, such as
> segmentation fault.
>
> So implements this feature is needed.
>
> Link:
> https://lore.kernel.org/linux-cxl/3e84b919-7631-d1db-3e1d-33000f3f3868@fujits
> u.com/
> Signed-off-by: Yao Xingtao
> ---
> hw/mem/cxl_type3.c | 18 ++
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index b0a7e9f11b..d6ef784e96 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -805,10 +805,17 @@ static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr
> host_addr, uint64_t *dpa)
> continue;
> }
>
> -*dpa = dpa_base +
> -((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> - ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) & hpa_offset)
> - >> iw));
> +if (iw < 8) {
> +*dpa = dpa_base +
> +((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> + ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) &
> hpa_offset)
> + >> iw));
> +} else {
> +*dpa = dpa_base +
> +((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> + MAKE_64BIT_MASK(ig + iw, 64 - ig - iw) & hpa_offset)
> + >> (ig + iw)) / 3) << (ig + 8)));
> +}
>
> return true;
> }
> @@ -906,6 +913,9 @@ static void ct3d_reset(DeviceState *dev)
> uint32_t *write_msk = ct3d->cxl_cstate.crb.cache_mem_regs_write_mask;
>
> cxl_component_register_init_common(reg_state, write_msk,
> CXL2_TYPE3_DEVICE);
> +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> 3_6_12_WAY, 1);
> +ARRAY_FIELD_DP32(reg_state, CXL_HDM_DECODER_CAPABILITY,
> 16_WAY, 1);
> +
> cxl_device_register_init_t3(ct3d);
>
> /*
> --
> 2.37.3
RE: [PATCH] mem/cxl_type3: fix hpa to dpa logic
> -Original Message- > From: Jonathan Cameron > Sent: Saturday, April 6, 2024 12:46 AM > To: Jonathan Cameron via > Cc: Jonathan Cameron ; Yao, Xingtao/姚 幸涛 > ; fan...@samsung.com; Cao, Quanquan/曹 全全 > > Subject: Re: [PATCH] mem/cxl_type3: fix hpa to dpa logic > > On Mon, 1 Apr 2024 17:00:50 +0100 > Jonathan Cameron via wrote: > > > On Thu, 28 Mar 2024 06:24:24 + > > "Xingtao Yao (Fujitsu)" wrote: > > > > > Jonathan > > > > > > thanks for your reply! > > > > > > > -Original Message- > > > > From: Jonathan Cameron > > > > Sent: Wednesday, March 27, 2024 9:28 PM > > > > To: Yao, Xingtao/姚 幸涛 > > > > Cc: fan...@samsung.com; qemu-devel@nongnu.org; Cao, Quanquan/曹 全 > 全 > > > > > > > > Subject: Re: [PATCH] mem/cxl_type3: fix hpa to dpa logic > > > > > > > > On Tue, 26 Mar 2024 21:46:53 -0400 > > > > Yao Xingtao wrote: > > > > > > > > > In 3, 6, 12 interleave ways, we could not access cxl memory properly, > > > > > and when the process is running on it, a 'segmentation fault' error > > > > > will > > > > > occur. > > > > > > > > > > According to the CXL specification '8.2.4.20.13 Decoder Protection', > > > > > there are two branches to convert HPA to DPA: > > > > > b1: Decoder[m].IW < 8 (for 1, 2, 4, 8, 16 interleave ways) > > > > > b2: Decoder[m].IW >= 8 (for 3, 6, 12 interleave ways) > > > > > > > > > > but only b1 has been implemented. > > > > > > > > > > To solve this issue, we should implement b2: > > > > > DPAOffset[51:IG+8]=HPAOffset[51:IG+IW] / 3 > > > > > DPAOffset[IG+7:0]=HPAOffset[IG+7:0] > > > > > DPA=DPAOffset + Decoder[n].DPABase > > > > > > > > > > Links: > > > > > https://lore.kernel.org/linux-cxl/3e84b919-7631-d1db-3e1d-33000f3f3868@fujits > > > > u.com/ > > > > > Signed-off-by: Yao Xingtao > > > > > > > > Not implementing this was intentional (shouldn't seg fault obviously) > > > > but > > > > I thought we were not advertising EP support for 3, 6, 12? The HDM > > > > Decoder > > > > configuration checking is currently terrible so we don't prevent > > > > the bits being set (adding device side sanity checks for those decoders > > > > has been on the todo list for a long time). There are a lot of ways of > > > > programming those that will blow up. > > > > > > > > Can you confirm that the emulation reports they are supported. > > > > > https://elixir.bootlin.com/qemu/v9.0.0-rc1/source/hw/cxl/cxl-component-utils.c > > > > #L246 > > > > implies it shouldn't and so any software using them is broken. > > > yes, the feature is not supported by QEMU, but I can still create a > 6-interleave-ways region on kernel layer. > > > > > > I checked the source code of kernel, and found that the kernel did not > > > check > this bit when committing decoder. > > > we may add some check on kernel side. > > > > ouch. We definitely want that check! The decoder commit will fail > > anyway (which QEMU doesn't yet because we don't do all the sanity checks > > we should). However failing on commit is nasty as the reason should have > > been detected earlier. > > > > > > > > > > > > > The non power of 2 decodes always made me nervous as the maths is more > > > > complex and any changes to that decode will need careful checking. > > > > For the power of 2 cases it was a bunch of writes to edge conditions etc > > > > and checking the right data landed in the backing stores. > > > after applying this modification, I tested some command by using these > memory, like 'ls', 'top'.. > > > and they can be executed normally, maybe there are some other problems I > haven't met yet. > > > > I usually run a bunch of manual tests with devmem2 to ensure the edge cases > > are > handled > > correctly, but I've not really seen any errors that didn't also show up in > > running > > stressors (e.g. stressng) or just memhog on the memory. > > > Hi Yao, > > If you have time, please spin a v2 that also sets the relevant > flag to say the QEMU emulation suppo
RE: [PATCH] mem/cxl_type3: fix hpa to dpa logic
Jonathan
thanks for your reply!
> -Original Message-
> From: Jonathan Cameron
> Sent: Wednesday, March 27, 2024 9:28 PM
> To: Yao, Xingtao/姚 幸涛
> Cc: fan...@samsung.com; qemu-devel@nongnu.org; Cao, Quanquan/曹 全全
>
> Subject: Re: [PATCH] mem/cxl_type3: fix hpa to dpa logic
>
> On Tue, 26 Mar 2024 21:46:53 -0400
> Yao Xingtao wrote:
>
> > In 3, 6, 12 interleave ways, we could not access cxl memory properly,
> > and when the process is running on it, a 'segmentation fault' error will
> > occur.
> >
> > According to the CXL specification '8.2.4.20.13 Decoder Protection',
> > there are two branches to convert HPA to DPA:
> > b1: Decoder[m].IW < 8 (for 1, 2, 4, 8, 16 interleave ways)
> > b2: Decoder[m].IW >= 8 (for 3, 6, 12 interleave ways)
> >
> > but only b1 has been implemented.
> >
> > To solve this issue, we should implement b2:
> > DPAOffset[51:IG+8]=HPAOffset[51:IG+IW] / 3
> > DPAOffset[IG+7:0]=HPAOffset[IG+7:0]
> > DPA=DPAOffset + Decoder[n].DPABase
> >
> > Links:
> https://lore.kernel.org/linux-cxl/3e84b919-7631-d1db-3e1d-33000f3f3868@fujits
> u.com/
> > Signed-off-by: Yao Xingtao
>
> Not implementing this was intentional (shouldn't seg fault obviously) but
> I thought we were not advertising EP support for 3, 6, 12? The HDM Decoder
> configuration checking is currently terrible so we don't prevent
> the bits being set (adding device side sanity checks for those decoders
> has been on the todo list for a long time). There are a lot of ways of
> programming those that will blow up.
>
> Can you confirm that the emulation reports they are supported.
> https://elixir.bootlin.com/qemu/v9.0.0-rc1/source/hw/cxl/cxl-component-utils.c
> #L246
> implies it shouldn't and so any software using them is broken.
yes, the feature is not supported by QEMU, but I can still create a
6-interleave-ways region on kernel layer.
I checked the source code of kernel, and found that the kernel did not check
this bit when committing decoder.
we may add some check on kernel side.
>
> The non power of 2 decodes always made me nervous as the maths is more
> complex and any changes to that decode will need careful checking.
> For the power of 2 cases it was a bunch of writes to edge conditions etc
> and checking the right data landed in the backing stores.
after applying this modification, I tested some command by using these memory,
like 'ls', 'top'..
and they can be executed normally, maybe there are some other problems I
haven't met yet.
>
> Joanthan
>
>
> > ---
> > hw/mem/cxl_type3.c | 15 +++
> > 1 file changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> > index b0a7e9f11b..2c1218fb12 100644
> > --- a/hw/mem/cxl_type3.c
> > +++ b/hw/mem/cxl_type3.c
> > @@ -805,10 +805,17 @@ static bool cxl_type3_dpa(CXLType3Dev *ct3d, hwaddr
> host_addr, uint64_t *dpa)
> > continue;
> > }
> >
> > -*dpa = dpa_base +
> > -((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> > - ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) & hpa_offset)
> > - >> iw));
> > +if (iw < 8) {
> > +*dpa = dpa_base +
> > +((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> > + ((MAKE_64BIT_MASK(8 + ig + iw, 64 - 8 - ig - iw) &
> hpa_offset)
> > + >> iw));
> > +} else {
> > +*dpa = dpa_base +
> > +((MAKE_64BIT_MASK(0, 8 + ig) & hpa_offset) |
> > + MAKE_64BIT_MASK(ig + iw, 64 - ig - iw) & hpa_offset)
> > + >> (ig + iw)) / 3) << (ig + 8)));
> > +}
> >
> > return true;
> > }
RE: [PATCH v3] contrib/plugins/execlog: Fix compiler warning
> -Original Message-
> From: Philippe Mathieu-Daudé
> Sent: Tuesday, March 26, 2024 8:04 PM
> To: Peter Maydell
> Cc: Pierrick Bouvier ; Yao, Xingtao/姚 幸涛
> ; qemu-devel@nongnu.org; alex.ben...@linaro.org;
> erdn...@crans.org; ma.mando...@gmail.com
> Subject: Re: [PATCH v3] contrib/plugins/execlog: Fix compiler warning
>
> On 26/3/24 11:33, Peter Maydell wrote:
> > On Tue, 26 Mar 2024 at 09:54, Philippe Mathieu-Daudé
> wrote:
> >>
> >> On 26/3/24 04:33, Pierrick Bouvier wrote:
> >>> On 3/26/24 05:52, Yao Xingtao wrote:
> 1. The g_pattern_match_string() is deprecated when glib2 version >=
> 2.70.
> Use g_pattern_spec_match_string() instead to avoid this
> problem.
>
> 2. The type of second parameter in g_ptr_array_add() is
> 'gpointer' {aka 'void *'}, but the type of reg->name is 'const
> char*'.
> Cast the type of reg->name to 'gpointer' to avoid this problem.
>
> compiler warning message:
> /root/qemu/contrib/plugins/execlog.c:330:17: warning:
> ‘g_pattern_match_string’
> is deprecated: Use 'g_pattern_spec_match_string'
> instead [-Wdeprecated-declarations]
> 330 | if (g_pattern_match_string(pat, rd->name)
> ||
> | ^~
> In file included from /usr/include/glib-2.0/glib.h:67,
> from /root/qemu/contrib/plugins/execlog.c:9:
> /usr/include/glib-2.0/glib/gpattern.h:57:15: note: declared here
> 57 | gboolean g_pattern_match_string (GPatternSpec
> *pspec,
> | ^~
> /root/qemu/contrib/plugins/execlog.c:331:21: warning:
> ‘g_pattern_match_string’
> is deprecated: Use 'g_pattern_spec_match_string'
> instead [-Wdeprecated-declarations]
> 331 | g_pattern_match_string(pat, rd_lower))
> {
> | ^~
> /usr/include/glib-2.0/glib/gpattern.h:57:15: note: declared here
> 57 | gboolean g_pattern_match_string (GPatternSpec
> *pspec,
> | ^~
> /root/qemu/contrib/plugins/execlog.c:339:63: warning: passing
> argument
> 2 of
> ‘g_ptr_array_add’ discards ‘const’ qualifier from pointer target
> type [-Wdiscarded-qualifiers]
> 339 |
> g_ptr_array_add(all_reg_names,
> reg->name);
> |
> ~~~^~
> In file included from /usr/include/glib-2.0/glib.h:33:
> /usr/include/glib-2.0/glib/garray.h:198:62: note: expected
> ‘gpointer’ {aka ‘void *’} but argument is of type ‘const char *’
> 198 |gpointer
> data);
> |
> ~~^~~~
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2210
> Signed-off-by: Yao Xingtao
> ---
> contrib/plugins/execlog.c | 24 +---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c
> index a1dfd59ab7..fab18113d4 100644
> --- a/contrib/plugins/execlog.c
> +++ b/contrib/plugins/execlog.c
> @@ -311,6 +311,24 @@ static Register
> *init_vcpu_register(qemu_plugin_reg_descriptor *desc)
> return reg;
> }
> +/*
> + * g_pattern_match_string has been deprecated in Glib since 2.70
> +and
> + * will complain about it if you try to use it. Fortunately the
> + * signature of both functions is the same making it easy to work
> + * around.
> + */
> +static inline
> +gboolean g_pattern_spec_match_string_qemu(GPatternSpec *pspec,
> + const gchar *string) {
> +#if GLIB_CHECK_VERSION(2, 70, 0)
> +return g_pattern_spec_match_string(pspec, string); #else
> +return g_pattern_match_string(pspec, string); #endif };
> +#define g_pattern_spec_match_string(p, s)
> g_pattern_spec_match_string_qemu(p, s)
> +
> static GPtrArray *registers_init(int vcpu_index)
> {
> g_autoptr(GPtrArray) registers = g_ptr_array_new(); @@
> -327,8 +345,8 @@ static GPtrArray *registers_init(int vcpu_index)
> for (int p = 0; p < rmatches->len; p++) {
> g_autoptr(GPatternSpec) pat =
> g_pattern_spec_new(rmatches->pdata[p]);
> g_autofree gchar *rd_lower =
> g_utf8_strdown(rd->name, -1);
> -if (g_pattern_match_string(pat, rd->name) ||
> -g_pattern_match_string(pat, rd_lower)) {
> +if (g_pattern_spec_match_string(pat, rd->name) ||
> +g_pattern_spec_match_string(pat, rd_lower)) {
> Register *reg = init_vcpu_register(rd);
> g_ptr_array_add(registers,
RE: [PATCH] contrib/plugins/execlog: Fix compiler warning
Hi Pierrick,
thanks for your reply, and I will modify and push the patch later.
thanks
Xingtao
> -Original Message-
> From: Pierrick Bouvier
> Sent: Monday, March 25, 2024 12:31 PM
> To: Yao, Xingtao/姚 幸涛 ; Peter Maydell
>
> Cc: alex.ben...@linaro.org; erdn...@crans.org; ma.mando...@gmail.com;
> qemu-devel@nongnu.org
> Subject: Re: [PATCH] contrib/plugins/execlog: Fix compiler warning
>
> On 3/25/24 07:00, Xingtao Yao (Fujitsu) wrote:
> > Pete:
> > Thanks for your comment.
> >
> > I also find a similar patch written by Pierrick:
> > https://lore.kernel.org/qemu-devel/20240118032400.3762658-15-pierrick.
> > bouv...@linaro.org/ but for some reason, the patch was not merged yet.
> >
> > shall I need to continue tracking the fixes of this bug?
> >
>
> Hi Xingtao,
>
> you're doing the right thing here. In my original patch, there was no
> consideration for backward compatibility, to the opposite of what you did.
>
> Alex will be out for several weeks, so it might take some time to get this
> merged,
> but I'm definitely voting for this 👍.
>
> Pierrick
>
> >> -Original Message-
> >> From: Peter Maydell
> >> Sent: Friday, March 22, 2024 7:50 PM
> >> To: Yao, Xingtao/姚 幸涛
> >> Cc: alex.ben...@linaro.org; erdn...@crans.org; ma.mando...@gmail.com;
> >> pierrick.bouv...@linaro.org; qemu-devel@nongnu.org
> >> Subject: Re: [PATCH] contrib/plugins/execlog: Fix compiler warning
> >>
> >> On Wed, 20 Mar 2024 at 02:05, Yao Xingtao via
> >> wrote:
> >>>
> >>> 1. The g_pattern_match_string() is deprecated when glib2 version >=
> 2.70.
> >>> Use g_pattern_spec_match_string() instead to avoid this problem.
> >>>
> >>> 2. The type of second parameter in g_ptr_array_add() is
> >>> 'gpointer' {aka 'void *'}, but the type of reg->name is 'const char*'.
> >>> Cast the type of reg->name to 'gpointer' to avoid this problem.
> >>>
> >>> compiler warning message:
> >>> /root/qemu/contrib/plugins/execlog.c:330:17: warning:
> >> ‘g_pattern_match_string’
> >>> is deprecated: Use 'g_pattern_spec_match_string'
> >>> instead [-Wdeprecated-declarations]
> >>>330 | if (g_pattern_match_string(pat, rd->name) ||
> >>>| ^~
> >>> In file included from /usr/include/glib-2.0/glib.h:67,
> >>> from /root/qemu/contrib/plugins/execlog.c:9:
> >>> /usr/include/glib-2.0/glib/gpattern.h:57:15: note: declared here
> >>> 57 | gboolean g_pattern_match_string (GPatternSpec
> *pspec,
> >>>| ^~
> >>> /root/qemu/contrib/plugins/execlog.c:331:21: warning:
> >> ‘g_pattern_match_string’
> >>> is deprecated: Use 'g_pattern_spec_match_string'
> >>> instead [-Wdeprecated-declarations]
> >>>331 | g_pattern_match_string(pat, rd_lower)) {
> >>>| ^~
> >>> /usr/include/glib-2.0/glib/gpattern.h:57:15: note: declared here
> >>> 57 | gboolean g_pattern_match_string (GPatternSpec
> *pspec,
> >>>| ^~
> >>> /root/qemu/contrib/plugins/execlog.c:339:63: warning: passing
> >>> argument
> >>> 2 of ‘g_ptr_array_add’ discards ‘const’ qualifier from pointer
> >>> target type [-Wdiscarded-qualifiers]
> >>>339 | g_ptr_array_add(all_reg_names,
> >> reg->name);
> >>>|
> >> ~~~^~
> >>> In file included from /usr/include/glib-2.0/glib.h:33:
> >>> /usr/include/glib-2.0/glib/garray.h:198:62: note: expected ‘gpointer’
> >>> {aka ‘void *’} but argument is of type ‘const char *’
> >>>198 |gpointer
> >> data);
> >>>|
> >> ~~^~~~
> >>>
> >>
> >> Hi; thanks for this patch.
> >>
> >> This fixes a bug reported in the bug tracker so we should put
> >>
> >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2210
> >>
> >> in the commit message just above your signed-off-by tag.
> >>
> >>
> >>> Signed-off-by: Yao Xingtao
> > I will if needed.
> >
> >>> ---
> >
RE: [PATCH] contrib/plugins/execlog: Fix compiler warning
Pete:
Thanks for your comment.
I also find a similar patch written by Pierrick:
https://lore.kernel.org/qemu-devel/20240118032400.3762658-15-pierrick.bouv...@linaro.org/
but for some reason, the patch was not merged yet.
shall I need to continue tracking the fixes of this bug?
> -Original Message-
> From: Peter Maydell
> Sent: Friday, March 22, 2024 7:50 PM
> To: Yao, Xingtao/姚 幸涛
> Cc: alex.ben...@linaro.org; erdn...@crans.org; ma.mando...@gmail.com;
> pierrick.bouv...@linaro.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH] contrib/plugins/execlog: Fix compiler warning
>
> On Wed, 20 Mar 2024 at 02:05, Yao Xingtao via
> wrote:
> >
> > 1. The g_pattern_match_string() is deprecated when glib2 version >= 2.70.
> >Use g_pattern_spec_match_string() instead to avoid this problem.
> >
> > 2. The type of second parameter in g_ptr_array_add() is
> >'gpointer' {aka 'void *'}, but the type of reg->name is 'const char*'.
> >Cast the type of reg->name to 'gpointer' to avoid this problem.
> >
> > compiler warning message:
> > /root/qemu/contrib/plugins/execlog.c:330:17: warning:
> ‘g_pattern_match_string’
> > is deprecated: Use 'g_pattern_spec_match_string'
> > instead [-Wdeprecated-declarations]
> > 330 | if (g_pattern_match_string(pat, rd->name) ||
> > | ^~
> > In file included from /usr/include/glib-2.0/glib.h:67,
> > from /root/qemu/contrib/plugins/execlog.c:9:
> > /usr/include/glib-2.0/glib/gpattern.h:57:15: note: declared here
> >57 | gboolean g_pattern_match_string (GPatternSpec *pspec,
> > | ^~
> > /root/qemu/contrib/plugins/execlog.c:331:21: warning:
> ‘g_pattern_match_string’
> > is deprecated: Use 'g_pattern_spec_match_string'
> > instead [-Wdeprecated-declarations]
> > 331 | g_pattern_match_string(pat, rd_lower)) {
> > | ^~
> > /usr/include/glib-2.0/glib/gpattern.h:57:15: note: declared here
> >57 | gboolean g_pattern_match_string (GPatternSpec *pspec,
> > | ^~
> > /root/qemu/contrib/plugins/execlog.c:339:63: warning: passing argument
> > 2 of ‘g_ptr_array_add’ discards ‘const’ qualifier from pointer target
> > type [-Wdiscarded-qualifiers]
> > 339 | g_ptr_array_add(all_reg_names,
> reg->name);
> > |
> ~~~^~
> > In file included from /usr/include/glib-2.0/glib.h:33:
> > /usr/include/glib-2.0/glib/garray.h:198:62: note: expected ‘gpointer’
> > {aka ‘void *’} but argument is of type ‘const char *’
> > 198 |gpointer
> data);
> > |
> ~~^~~~
> >
>
> Hi; thanks for this patch.
>
> This fixes a bug reported in the bug tracker so we should put
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2210
>
> in the commit message just above your signed-off-by tag.
>
>
> > Signed-off-by: Yao Xingtao
I will if needed.
> > ---
> > contrib/plugins/execlog.c | 7 ++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c
> > index a1dfd59ab7..41f6774116 100644
> > --- a/contrib/plugins/execlog.c
> > +++ b/contrib/plugins/execlog.c
> > @@ -327,8 +327,13 @@ static GPtrArray *registers_init(int vcpu_index)
> > for (int p = 0; p < rmatches->len; p++) {
> > g_autoptr(GPatternSpec) pat =
> g_pattern_spec_new(rmatches->pdata[p]);
> > g_autofree gchar *rd_lower = g_utf8_strdown(rd->name,
> > -1);
> > +#if GLIB_VERSION_MAX_ALLOWED < G_ENCODE_VERSION(2, 70)
>
> Elsewhere we do glib version checks with
>
> #if GLIB_CHECK_VERSION(2, 70, 0)
> code for 2.70.0 and up;
> #else
> code for older versions
> #endif
>
> so I think we should probably do that here too.
>
> > if (g_pattern_match_string(pat, rd->name) ||
> > g_pattern_match_string(pat, rd_lower)) {
> > +#else
> > +if (g_pattern_spec_match_string(pat, rd->name) ||
> > +g_pattern_spec_match_string(pat, rd_lower)) {
> > +#endif
thanks, I got it.
>
> Rather than putting this ifdef in the middle of this function, I think it
> would be
> easier to read if we abstract out a function which does the pattern matching
> and whose body calls the right glib function depending on glib version. We
> generally call these functions the same as the glib function but with a _qemu
> suffix (compare the ones in include/glib-compat.h), so here that would be
> g_pattern_spec_match_string_qemu().
>
> > Register *reg = init_vcpu_register(rd);
> > g_ptr_array_add(registers, reg);
> >
> > @@ -336,7 +341,7 @@ static GPtrArray *registers_init(int vcpu_index)
> > if (disas_assist) {
> > g_mutex_lock(&add_reg_name_lock);
> >
