Re: udp guestfwd
On Fri, Oct 27, 2023 at 11:44 PM Louai Al-Khanji wrote: > Hi, > > I'm interested in having the guestfwd option work for udp. My > understanding is that currently it's restricted to only tcp. > > I'm not familiar with libslirp internals. What would need to be changed to > implement this? I'm potentially interested in doing the work. > > I did a tiny amount of digging around libslirp and saw this comment in > `udp.c': > > /* > * X Here, check if it's in udpexec_list, > * and if it is, do the fork_exec() etc. > */ > > I wonder whether that is related. In any case any help is much appreciated. > Felix has been working in this space and it may take time to get the CLs landed in libslirp and qemu. Patrick > > Thanks, > Louai Al-Khanji >
Re: [PATCH 1/1] hw/i2c: add pca9543 i2c-mux switch
On Tue, Nov 14, 2023 at 3:30 PM Corey Minyard wrote:
> On Mon, Nov 13, 2023 at 02:31:56PM +0800, Potin Lai wrote:
> > Add pca9543 2-channel i2c-mux switch support.
> >
> > Signed-off-by: Potin Lai
>
> Looks good to me.
>
> Acked-by: Corey Minyard
>
> > ---
> > hw/i2c/i2c_mux_pca954x.c | 12
> > include/hw/i2c/i2c_mux_pca954x.h | 1 +
> > 2 files changed, 13 insertions(+)
> >
> > diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
> > index db5db956a6..6aace0fc47 100644
> > --- a/hw/i2c/i2c_mux_pca954x.c
> > +++ b/hw/i2c/i2c_mux_pca954x.c
> > @@ -30,6 +30,7 @@
> >
> > #define PCA9548_CHANNEL_COUNT 8
> > #define PCA9546_CHANNEL_COUNT 4
> > +#define PCA9543_CHANNEL_COUNT 2
> >
> > /*
> > * struct Pca954xState - The pca954x state object.
> > @@ -172,6 +173,12 @@ I2CBus *pca954x_i2c_get_bus(I2CSlave *mux, uint8_t
> channel)
> > return pca954x->bus[channel];
> > }
> >
> > +static void pca9543_class_init(ObjectClass *klass, void *data)
> > +{
> > +Pca954xClass *s = PCA954X_CLASS(klass);
> > +s->nchans = PCA9543_CHANNEL_COUNT;
> > +}
> > +
> > static void pca9546_class_init(ObjectClass *klass, void *data)
> > {
> > Pca954xClass *s = PCA954X_CLASS(klass);
> > @@ -246,6 +253,11 @@ static const TypeInfo pca954x_info[] = {
> > .class_init= pca954x_class_init,
> > .abstract = true,
> > },
> > +{
> > +.name = TYPE_PCA9543,
> > +.parent= TYPE_PCA954X,
> > +.class_init= pca9543_class_init,
> > +},
> > {
> > .name = TYPE_PCA9546,
> > .parent= TYPE_PCA954X,
> > diff --git a/include/hw/i2c/i2c_mux_pca954x.h
> b/include/hw/i2c/i2c_mux_pca954x.h
> > index 3dd25ec983..1da5508ed5 100644
> > --- a/include/hw/i2c/i2c_mux_pca954x.h
> > +++ b/include/hw/i2c/i2c_mux_pca954x.h
> > @@ -3,6 +3,7 @@
> >
> > #include "hw/i2c/i2c.h"
> >
> > +#define TYPE_PCA9543 "pca9543"
> > #define TYPE_PCA9546 "pca9546"
> > #define TYPE_PCA9548 "pca9548"
> >
> > --
> > 2.31.1
> >
> >
Corey, can you include this in a pull email? I don't quite have that set up
at present, Titus is going to help me figure it out.
Patrick
Re: [PATCH v2] system/memory: use ldn_he_p/stn_he_p
On Mon, Dec 4, 2023 at 3:24 AM Philippe Mathieu-Daudé wrote: > Hi Patrick, > > On 3/12/23 16:42, Patrick Venture wrote: > > > Friendly ping? Is this going to be applied or do I need to add another > > CC or? I do think it should go into stable. > > I'll send a PR with this patch included. > Thanks! > > Regards, > > Phil. >
Re: [PATCH v2] system/memory: use ldn_he_p/stn_he_p
On Fri, Nov 17, 2023 at 12:43 AM David Hildenbrand wrote: > On 16.11.23 17:36, Patrick Venture wrote: > > Using direct pointer dereferencing can allow for unaligned accesses, > > which was seen during execution with sanitizers enabled. > > > > Reviewed-by: Chris Rauer > > Reviewed-by: Peter Foley > > Signed-off-by: Patrick Venture > > Cc: qemu-sta...@nongnu.org > > > > Reviewed-by: David Hildenbrand > > -- > Cheers, > > David / dhildenb > Friendly ping? Is this going to be applied or do I need to add another CC or? I do think it should go into stable.
[PATCH v2] system/memory: use ldn_he_p/stn_he_p
Using direct pointer dereferencing can allow for unaligned accesses,
which was seen during execution with sanitizers enabled.
Reviewed-by: Chris Rauer
Reviewed-by: Peter Foley
Signed-off-by: Patrick Venture
Cc: qemu-sta...@nongnu.org
---
v2: changed commit mesage to be more accurate and switched from using
memcpy to using the endian appropriate assignment load and store.
---
system/memory.c | 32 ++--
1 file changed, 2 insertions(+), 30 deletions(-)
diff --git a/system/memory.c b/system/memory.c
index 304fa843ea..affc7ea83c 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1339,22 +1339,7 @@ static uint64_t memory_region_ram_device_read(void
*opaque,
hwaddr addr, unsigned size)
{
MemoryRegion *mr = opaque;
-uint64_t data = (uint64_t)~0;
-
-switch (size) {
-case 1:
-data = *(uint8_t *)(mr->ram_block->host + addr);
-break;
-case 2:
-data = *(uint16_t *)(mr->ram_block->host + addr);
-break;
-case 4:
-data = *(uint32_t *)(mr->ram_block->host + addr);
-break;
-case 8:
-data = *(uint64_t *)(mr->ram_block->host + addr);
-break;
-}
+uint64_t data = ldn_he_p(mr->ram_block->host + addr, size);
trace_memory_region_ram_device_read(get_cpu_index(), mr, addr, data, size);
@@ -1368,20 +1353,7 @@ static void memory_region_ram_device_write(void *opaque,
hwaddr addr,
trace_memory_region_ram_device_write(get_cpu_index(), mr, addr, data,
size);
-switch (size) {
-case 1:
-*(uint8_t *)(mr->ram_block->host + addr) = (uint8_t)data;
-break;
-case 2:
-*(uint16_t *)(mr->ram_block->host + addr) = (uint16_t)data;
-break;
-case 4:
-*(uint32_t *)(mr->ram_block->host + addr) = (uint32_t)data;
-break;
-case 8:
-*(uint64_t *)(mr->ram_block->host + addr) = data;
-break;
-}
+stn_he_p(mr->ram_block->host + addr, size, data);
}
static const MemoryRegionOps ram_device_mem_ops = {
--
2.43.0.rc0.421.g78406f8d94-goog
Re: [PATCH] softmmu/memory: use memcpy for multi-byte accesses
On Wed, Nov 15, 2023 at 9:26 AM Patrick Venture wrote: > > > On Wed, Nov 15, 2023 at 9:02 AM Richard Henderson < > richard.hender...@linaro.org> wrote: > >> On 11/15/23 08:58, Patrick Venture wrote: >> > >> > >> > On Wed, Nov 15, 2023 at 2:35 AM Peter Maydell > > <mailto:peter.mayd...@linaro.org>> wrote: >> > >> > On Tue, 14 Nov 2023 at 20:55, Patrick Venture > > <mailto:vent...@google.com>> wrote: >> > > Avoids unaligned pointer issues. >> > > >> > >> > It would be nice to be more specific in the commit message here, by >> > describing what kind of guest behaviour or machine config runs into >> this >> > problem, and whether this happens in a situation users are likely to >> > run into. If the latter, we should consider tagging the commit >> > with "Cc: qemu-sta...@nongnu.org <mailto:qemu-sta...@nongnu.org>" >> to have it >> > backported to the >> > stable release branches. >> > >> > >> > Thanks! I'll update the commit message with v2. We were seeing this in >> our >> > infrastructure with unaligned accesses using the pointer dereference as >> there are no >> > guarantees on alignment of the incoming values. >> >> Which host cpu, for reference? There aren't many that generate unaligned >> traps these days... >> >> > Here's the sanitizer log/qemu log, the host-cpu was an amd64. > AMD ROME > > qemu-kvm-system-x86_64: warning: host doesn't support requested feature: > CPUID.01H:ECX.pcid [bit 17] > qemu-kvm-system-x86_64: warning: host doesn't support requested feature: > CPUID.07H:EBX.erms [bit 9] > qemu-kvm-system-x86_64: warning: host doesn't support requested feature: > CPUID.07H:EBX.invpcid [bit 10] > qemu-kvm-system-x86_64: warning: host doesn't support requested feature: > CPUID.01H:ECX.pcid [bit 17] > qemu-kvm-system-x86_64: warning: host doesn't support requested feature: > CPUID.07H:EBX.erms [bit 9] > qemu-kvm-system-x86_64: warning: host doesn't support requested feature: > CPUID.07H:EBX.invpcid [bit 10] > third_party/qemu/softmmu/memory.c:1341:16: runtime error: load of > misaligned address 0x52500020b10d for type 'uint32_t' (aka 'unsigned int'), > which requires 4 byte alignment > 0x52500020b10d: note: pointer points here > ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab > ab ab ab ab ab ab ab ab ab > ^ > #0 0x55b34f8ef9d8 in memory_region_ram_device_read > third_party/qemu/softmmu/memory.c:1341:16 > #1 0x55b34f8ee8a8 in memory_region_read_accessor > third_party/qemu/softmmu/memory.c:441:11 > #2 0x55b34f8e06db in access_with_adjusted_size > third_party/qemu/softmmu/memory.c:569:18 > #3 0x55b34f8dfcb4 in memory_region_dispatch_read1 > third_party/qemu/softmmu/memory.c > #4 0x55b34f8dfcb4 in memory_region_dispatch_read > third_party/qemu/softmmu/memory.c:1476:9 > #5 0x55b34f8fa8b0 in flatview_read_continue > third_party/qemu/softmmu/physmem.c:2744:23 > #6 0x55b34f8fb0db in flatview_read > third_party/qemu/softmmu/physmem.c:2786:12 > #7 0x55b34f8faefa in address_space_read_full > third_party/qemu/softmmu/physmem.c:2799:18 > #8 0x55b34f8fb5b4 in address_space_rw > third_party/qemu/softmmu/physmem.c:2827:16 > #9 0x55b34f71eab5 in kvm_cpu_exec > third_party/qemu/accel/kvm/kvm-all.c:3062:13 > #10 0x55b34f7172e3 in kvm_vcpu_thread_fn > third_party/qemu/accel/kvm/kvm-accel-ops.c:51:17 > #11 0x55b350467044 in qemu_thread_start > third_party/qemu/util/qemu-thread-posix.c:541:9 > #12 0x55b34f6dba10 in asan_thread_start(void*) > third_party/llvm/llvm-project/compiler-rt/lib/asan/asan_interceptors.cpp:234:28 > #13 0x7f5e1c81a7d8 in start_thread > (/usr/grte/v5/lib64/libpthread.so.0+0xb7d8) (BuildId: > 3ccc1600b9140e48da03ed16e0210354) > #14 0x7f5e1c77169e in clone (/usr/grte/v5/lib64/libc.so.6+0x13969e) > (BuildId: 280088eab084c30a3992a9bce5c35b44) > > SUMMARY: UndefinedBehaviorSanitizer: misaligned-pointer-use > third_party/qemu/softmmu/memory.c:1341:16 in > > > > >> >> r~ >> >>
Re: [PATCH] softmmu/memory: use memcpy for multi-byte accesses
On Wed, Nov 15, 2023 at 9:02 AM Richard Henderson < richard.hender...@linaro.org> wrote: > On 11/15/23 08:58, Patrick Venture wrote: > > > > > > On Wed, Nov 15, 2023 at 2:35 AM Peter Maydell > <mailto:peter.mayd...@linaro.org>> wrote: > > > > On Tue, 14 Nov 2023 at 20:55, Patrick Venture > <mailto:vent...@google.com>> wrote: > > > Avoids unaligned pointer issues. > > > > > > > It would be nice to be more specific in the commit message here, by > > describing what kind of guest behaviour or machine config runs into > this > > problem, and whether this happens in a situation users are likely to > > run into. If the latter, we should consider tagging the commit > > with "Cc: qemu-sta...@nongnu.org <mailto:qemu-sta...@nongnu.org>" > to have it > > backported to the > > stable release branches. > > > > > > Thanks! I'll update the commit message with v2. We were seeing this in > our > > infrastructure with unaligned accesses using the pointer dereference as > there are no > > guarantees on alignment of the incoming values. > > Which host cpu, for reference? There aren't many that generate unaligned > traps these days... > > Here's the sanitizer log/qemu log, the host-cpu was an amd64. qemu-kvm-system-x86_64: warning: host doesn't support requested feature: CPUID.01H:ECX.pcid [bit 17] qemu-kvm-system-x86_64: warning: host doesn't support requested feature: CPUID.07H:EBX.erms [bit 9] qemu-kvm-system-x86_64: warning: host doesn't support requested feature: CPUID.07H:EBX.invpcid [bit 10] qemu-kvm-system-x86_64: warning: host doesn't support requested feature: CPUID.01H:ECX.pcid [bit 17] qemu-kvm-system-x86_64: warning: host doesn't support requested feature: CPUID.07H:EBX.erms [bit 9] qemu-kvm-system-x86_64: warning: host doesn't support requested feature: CPUID.07H:EBX.invpcid [bit 10] third_party/qemu/softmmu/memory.c:1341:16: runtime error: load of misaligned address 0x52500020b10d for type 'uint32_t' (aka 'unsigned int'), which requires 4 byte alignment 0x52500020b10d: note: pointer points here ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ^ #0 0x55b34f8ef9d8 in memory_region_ram_device_read third_party/qemu/softmmu/memory.c:1341:16 #1 0x55b34f8ee8a8 in memory_region_read_accessor third_party/qemu/softmmu/memory.c:441:11 #2 0x55b34f8e06db in access_with_adjusted_size third_party/qemu/softmmu/memory.c:569:18 #3 0x55b34f8dfcb4 in memory_region_dispatch_read1 third_party/qemu/softmmu/memory.c #4 0x55b34f8dfcb4 in memory_region_dispatch_read third_party/qemu/softmmu/memory.c:1476:9 #5 0x55b34f8fa8b0 in flatview_read_continue third_party/qemu/softmmu/physmem.c:2744:23 #6 0x55b34f8fb0db in flatview_read third_party/qemu/softmmu/physmem.c:2786:12 #7 0x55b34f8faefa in address_space_read_full third_party/qemu/softmmu/physmem.c:2799:18 #8 0x55b34f8fb5b4 in address_space_rw third_party/qemu/softmmu/physmem.c:2827:16 #9 0x55b34f71eab5 in kvm_cpu_exec third_party/qemu/accel/kvm/kvm-all.c:3062:13 #10 0x55b34f7172e3 in kvm_vcpu_thread_fn third_party/qemu/accel/kvm/kvm-accel-ops.c:51:17 #11 0x55b350467044 in qemu_thread_start third_party/qemu/util/qemu-thread-posix.c:541:9 #12 0x55b34f6dba10 in asan_thread_start(void*) third_party/llvm/llvm-project/compiler-rt/lib/asan/asan_interceptors.cpp:234:28 #13 0x7f5e1c81a7d8 in start_thread (/usr/grte/v5/lib64/libpthread.so.0+0xb7d8) (BuildId: 3ccc1600b9140e48da03ed16e0210354) #14 0x7f5e1c77169e in clone (/usr/grte/v5/lib64/libc.so.6+0x13969e) (BuildId: 280088eab084c30a3992a9bce5c35b44) SUMMARY: UndefinedBehaviorSanitizer: misaligned-pointer-use third_party/qemu/softmmu/memory.c:1341:16 in > > r~ > >
Re: [PATCH] softmmu/memory: use memcpy for multi-byte accesses
On Wed, Nov 15, 2023 at 2:35 AM Peter Maydell wrote: > On Tue, 14 Nov 2023 at 20:55, Patrick Venture wrote: > > Avoids unaligned pointer issues. > > > > It would be nice to be more specific in the commit message here, by > describing what kind of guest behaviour or machine config runs into this > problem, and whether this happens in a situation users are likely to > run into. If the latter, we should consider tagging the commit > with "Cc: qemu-sta...@nongnu.org" to have it backported to the > stable release branches. > Thanks! I'll update the commit message with v2. We were seeing this in our infrastructure with unaligned accesses using the pointer dereference as there are no guarantees on alignment of the incoming values. > > thanks > -- PMM >
Re: [PATCH] softmmu/memory: use memcpy for multi-byte accesses
On Wed, Nov 15, 2023 at 2:30 AM Peter Maydell
wrote:
> On Tue, 14 Nov 2023 at 21:18, Richard Henderson
> wrote:
> >
> > On 11/14/23 12:55, Patrick Venture wrote:
> > > Avoids unaligned pointer issues.
> > >
> > > Reviewed-by: Chris Rauer
> > > Reviewed-by: Peter Foley
> > > Signed-off-by: Patrick Venture
> > > ---
> > > system/memory.c | 16
> > > 1 file changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/system/memory.c b/system/memory.c
> > > index 304fa843ea..02c97d5187 100644
> > > --- a/system/memory.c
> > > +++ b/system/memory.c
> > > @@ -1343,16 +1343,16 @@ static uint64_t
> memory_region_ram_device_read(void *opaque,
> > >
> > > switch (size) {
> > > case 1:
> > > -data = *(uint8_t *)(mr->ram_block->host + addr);
> > > +memcpy(, mr->ram_block->host + addr, sizeof(uint8_t));
> >
> >
> > This is incorrect, especially for big-endian hosts.
> >
> > You want to use "qemu/bswap.h", ld*_he_p(), st*_he_p().
>
> More specifically, we have a ldn_he_p() and stn_he_p() that
> take the size in bytes of the data to read, so we should be
> able to replace the switch-on-size in these functions with
> a single call to the appropriate one of those.
>
Thanks!
>
> thanks
> -- PMM
>
Re: [PATCH 1/1] hw/i2c: add pca9543 i2c-mux switch
On Sun, Nov 12, 2023 at 10:34 PM Potin Lai wrote:
> Add pca9543 2-channel i2c-mux switch support.
>
> Signed-off-by: Potin Lai
>
Reviewed-by: Patrick Venture
> ---
> hw/i2c/i2c_mux_pca954x.c | 12
> include/hw/i2c/i2c_mux_pca954x.h | 1 +
> 2 files changed, 13 insertions(+)
>
> diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
> index db5db956a6..6aace0fc47 100644
> --- a/hw/i2c/i2c_mux_pca954x.c
> +++ b/hw/i2c/i2c_mux_pca954x.c
> @@ -30,6 +30,7 @@
>
> #define PCA9548_CHANNEL_COUNT 8
> #define PCA9546_CHANNEL_COUNT 4
> +#define PCA9543_CHANNEL_COUNT 2
>
> /*
> * struct Pca954xState - The pca954x state object.
> @@ -172,6 +173,12 @@ I2CBus *pca954x_i2c_get_bus(I2CSlave *mux, uint8_t
> channel)
> return pca954x->bus[channel];
> }
>
> +static void pca9543_class_init(ObjectClass *klass, void *data)
> +{
> +Pca954xClass *s = PCA954X_CLASS(klass);
> +s->nchans = PCA9543_CHANNEL_COUNT;
> +}
> +
> static void pca9546_class_init(ObjectClass *klass, void *data)
> {
> Pca954xClass *s = PCA954X_CLASS(klass);
> @@ -246,6 +253,11 @@ static const TypeInfo pca954x_info[] = {
> .class_init= pca954x_class_init,
> .abstract = true,
> },
> +{
> +.name = TYPE_PCA9543,
> +.parent= TYPE_PCA954X,
> +.class_init= pca9543_class_init,
> +},
> {
> .name = TYPE_PCA9546,
> .parent= TYPE_PCA954X,
> diff --git a/include/hw/i2c/i2c_mux_pca954x.h
> b/include/hw/i2c/i2c_mux_pca954x.h
> index 3dd25ec983..1da5508ed5 100644
> --- a/include/hw/i2c/i2c_mux_pca954x.h
> +++ b/include/hw/i2c/i2c_mux_pca954x.h
> @@ -3,6 +3,7 @@
>
> #include "hw/i2c/i2c.h"
>
> +#define TYPE_PCA9543 "pca9543"
> #define TYPE_PCA9546 "pca9546"
> #define TYPE_PCA9548 "pca9548"
>
> --
> 2.31.1
>
>
Re: [PATCH] softmmu/memory: use memcpy for multi-byte accesses
On Tue, Nov 14, 2023 at 1:18 PM Richard Henderson <
richard.hender...@linaro.org> wrote:
> On 11/14/23 12:55, Patrick Venture wrote:
> > Avoids unaligned pointer issues.
> >
> > Reviewed-by: Chris Rauer
> > Reviewed-by: Peter Foley
> > Signed-off-by: Patrick Venture
> > ---
> > system/memory.c | 16
> > 1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/system/memory.c b/system/memory.c
> > index 304fa843ea..02c97d5187 100644
> > --- a/system/memory.c
> > +++ b/system/memory.c
> > @@ -1343,16 +1343,16 @@ static uint64_t
> memory_region_ram_device_read(void *opaque,
> >
> > switch (size) {
> > case 1:
> > -data = *(uint8_t *)(mr->ram_block->host + addr);
> > +memcpy(, mr->ram_block->host + addr, sizeof(uint8_t));
>
>
> This is incorrect, especially for big-endian hosts.
>
> You want to use "qemu/bswap.h", ld*_he_p(), st*_he_p().
>
Thanks, I'll take a look.
>
>
> r~
>
[PATCH] softmmu/memory: use memcpy for multi-byte accesses
Avoids unaligned pointer issues.
Reviewed-by: Chris Rauer
Reviewed-by: Peter Foley
Signed-off-by: Patrick Venture
---
system/memory.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/system/memory.c b/system/memory.c
index 304fa843ea..02c97d5187 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1343,16 +1343,16 @@ static uint64_t memory_region_ram_device_read(void
*opaque,
switch (size) {
case 1:
-data = *(uint8_t *)(mr->ram_block->host + addr);
+memcpy(, mr->ram_block->host + addr, sizeof(uint8_t));
break;
case 2:
-data = *(uint16_t *)(mr->ram_block->host + addr);
+memcpy(, mr->ram_block->host + addr, sizeof(uint16_t));
break;
case 4:
-data = *(uint32_t *)(mr->ram_block->host + addr);
+memcpy(, mr->ram_block->host + addr, sizeof(uint32_t));
break;
case 8:
-data = *(uint64_t *)(mr->ram_block->host + addr);
+memcpy(, mr->ram_block->host + addr, sizeof(uint64_t));
break;
}
@@ -1370,16 +1370,16 @@ static void memory_region_ram_device_write(void
*opaque, hwaddr addr,
switch (size) {
case 1:
-*(uint8_t *)(mr->ram_block->host + addr) = (uint8_t)data;
+memcpy(mr->ram_block->host + addr, , sizeof(uint8_t));
break;
case 2:
-*(uint16_t *)(mr->ram_block->host + addr) = (uint16_t)data;
+memcpy(mr->ram_block->host + addr, , sizeof(uint16_t));
break;
case 4:
-*(uint32_t *)(mr->ram_block->host + addr) = (uint32_t)data;
+memcpy(mr->ram_block->host + addr, , sizeof(uint32_t));
break;
case 8:
-*(uint64_t *)(mr->ram_block->host + addr) = data;
+memcpy(mr->ram_block->host + addr, , sizeof(uint64_t));
break;
}
}
--
2.43.0.rc0.421.g78406f8d94-goog
Re: [PATCH RESEND v2] hw/i2c: Enable an id for the pca954x devices
On Wed, Mar 22, 2023 at 2:40 PM Philippe Mathieu-Daudé wrote: > On 22/3/23 22:19, Corey Minyard wrote: > > On Wed, Mar 22, 2023 at 10:21:36AM -0700, Patrick Venture wrote: > >> This allows the devices to be more readily found and specified. > >> Without setting the name field, they can only be found by device type > >> name, which doesn't let you specify the second of the same device type > >> behind a bus. > >> > >> Tested: Verified that by default the device was findable with the name > >> 'pca954x[77]', for an instance attached at that address. > > > > This looks good to me. > > > > Acked-by: Corey Minyard > > > > if you are taking this in through another tree. Or do you want me to > > take this? > > Since I have to send a MIPS PR, I'll take this one; > to alleviate you and the CI minutes. > I don't see this patch yet, did it got lost in the shuffle? thanks, Patrick
Re: ssl fips self check fails with 7.2.0 on x86 TCG
On Tue, May 9, 2023 at 9:51 AM Michael Tokarev wrote: > 09.05.2023 17:39, Philippe Mathieu-Daudé пишет: > ..> Should be fixed in v7.2-stable: > > > > $ git log --oneline --grep=1d0b9261 v7.2.2 > > c45d10f655 target/i386: fix ADOX followed by ADCX > > 6809dbc5c5 target/i386: Fix C flag for BLSI, BLSMSK, BLSR > > 8d3c9fc439 target/i386: Fix BEXTR instruction > > Unfortunately it is still not released, - > I haven't heard anything from Michael Roth since Apr-22 (when 7.2.2 > planned). > Thanks, I have it cherry-picked into our repo. :) > > /mjt >
Re: ssl fips self check fails with 7.2.0 on x86 TCG
Verified it was https://gitlab.com/qemu-project/qemu/-/issues/1471 On Thu, May 4, 2023 at 12:03 PM Patrick Venture wrote: > Hi, > > I just finished rebasing my team onto 7.2.0 and now I'm seeing > https://boringssl.googlesource.com/boringssl/+/master/crypto/fipsmodule/self_check/self_check.c#361 > fail. > > I applied > https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg00260.html and > it's still failing. > > Is anyone else seeing this issue or have suggestions on how to debug it? > > I haven't yet tried with 8.0.0 but that's my next step, although it also > needs the float32_exp3 patch. > > Patrick >
ssl fips self check fails with 7.2.0 on x86 TCG
Hi, I just finished rebasing my team onto 7.2.0 and now I'm seeing https://boringssl.googlesource.com/boringssl/+/master/crypto/fipsmodule/self_check/self_check.c#361 fail. I applied https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg00260.html and it's still failing. Is anyone else seeing this issue or have suggestions on how to debug it? I haven't yet tried with 8.0.0 but that's my next step, although it also needs the float32_exp3 patch. Patrick
Re: [PATCH v2] hw/net: npcm7xx_emc: set MAC in register space
On Thu, Oct 6, 2022 at 6:18 AM Peter Maydell wrote: > On Mon, 3 Oct 2022 at 18:38, Patrick Venture wrote: > > > > The MAC address set from Qemu wasn't being saved into the register space. > > > > Reviewed-by: Hao Wu > > Signed-off-by: Patrick Venture > > --- > > v2: only set the registers from qemu on reset > > once registers set, only read and write to them > > > > Applied to target-arm.next, thanks. > I think this was missed. Please take a look. > > -- PMM >
[PATCH] hw/arm/virt: support both pl011 and 16550 uart
From: Shu-Chun Weng
Select uart for virt machine from pl011 and ns16550a with
-M virt,uart={pl011|ns16550a}.
Signed-off-by: Shu-Chun Weng
Signed-off-by: Patrick Venture
---
hw/arm/virt.c | 85 ++-
include/hw/arm/virt.h | 6 +++
2 files changed, 89 insertions(+), 2 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index ac626b3bef..84b335a5d7 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -80,6 +80,7 @@
#include "hw/virtio/virtio-iommu.h"
#include "hw/char/pl011.h"
#include "qemu/guest-random.h"
+#include "hw/char/serial.h"
#define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
@@ -847,8 +848,37 @@ static void create_gic(VirtMachineState *vms, MemoryRegion
*mem)
}
}
-static void create_uart(const VirtMachineState *vms, int uart,
-MemoryRegion *mem, Chardev *chr)
+static void create_uart_ns16550a(const VirtMachineState *vms, int uart,
+ MemoryRegion *mem, Chardev *chr)
+{
+char *nodename;
+hwaddr base = vms->memmap[uart].base;
+hwaddr size = vms->memmap[uart].size;
+int irq = vms->irqmap[uart];
+const char compat[] = "ns16550a";
+
+serial_mm_init(get_system_memory(), base, 0,
+ qdev_get_gpio_in(vms->gic, irq), 19200, serial_hd(0),
+ DEVICE_LITTLE_ENDIAN);
+
+nodename = g_strdup_printf("/serial@%" PRIx64, base);
+
+MachineState *ms = MACHINE(vms);
+
+qemu_fdt_add_subnode(ms->fdt, nodename);
+qemu_fdt_setprop(ms->fdt, nodename, "compatible", compat, sizeof(compat));
+qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg", 2, base, 2, size);
+qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "clock-frequency",
+ 1, 0x825f0);
+qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts",
+ GIC_FDT_IRQ_TYPE_SPI, irq,
+ GIC_FDT_IRQ_FLAGS_LEVEL_HI);
+
+g_free(nodename);
+}
+
+static void create_uart_pl011(const VirtMachineState *vms, int uart,
+ MemoryRegion *mem, Chardev *chr)
{
char *nodename;
hwaddr base = vms->memmap[uart].base;
@@ -895,6 +925,16 @@ static void create_uart(const VirtMachineState *vms, int
uart,
g_free(nodename);
}
+static void create_uart(const VirtMachineState *vms, int uart,
+MemoryRegion *mem, Chardev *chr)
+{
+if (vms->uart == UART_NS16550A) {
+create_uart_ns16550a(vms, uart, mem, chr);
+} else {
+create_uart_pl011(vms, uart, mem, chr);
+}
+}
+
static void create_rtc(const VirtMachineState *vms)
{
char *nodename;
@@ -2601,6 +2641,39 @@ static void virt_set_gic_version(Object *obj, const char
*value, Error **errp)
}
}
+static char *virt_get_uart_type(Object *obj, Error **errp)
+{
+VirtMachineState *vms = VIRT_MACHINE(obj);
+const char *val = NULL;
+
+switch (vms->uart) {
+case UART_PL011:
+val = "pl011";
+break;
+case UART_NS16550A:
+val = "ns16550a";
+break;
+default:
+error_setg(errp, "Invalid uart value");
+}
+
+return g_strdup(val);
+}
+
+static void virt_set_uart_type(Object *obj, const char *value, Error **errp)
+{
+VirtMachineState *vms = VIRT_MACHINE(obj);
+
+if (!strcmp(value, "pl011")) {
+vms->uart = UART_PL011;
+} else if (!strcmp(value, "ns16550a")) {
+vms->uart = UART_NS16550A;
+} else {
+error_setg(errp, "Invalid uart type");
+error_append_hint(errp, "Valid values are pl011, and ns16550a.\n");
+}
+}
+
static char *virt_get_iommu(Object *obj, Error **errp)
{
VirtMachineState *vms = VIRT_MACHINE(obj);
@@ -3172,6 +3245,14 @@ static void virt_instance_init(Object *obj)
vms->highmem_compact = !vmc->no_highmem_compact;
vms->gic_version = VIRT_GIC_VERSION_NOSEL;
+/* Default uart type is pl011 */
+vms->uart = UART_PL011;
+object_property_add_str(obj, "uart", virt_get_uart_type,
+virt_set_uart_type);
+object_property_set_description(obj, "uart",
+"Set uart type. "
+"Valid values are pl011 and ns16550a");
+
vms->highmem_ecam = !vmc->no_highmem_ecam;
vms->highmem_mmio = true;
vms->highmem_redists = true;
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index e1ddbea96b..04539f347d 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -122,6 +122,11 @@ typedef enum VirtGICType {
#define VIRT_GIC_VERSION_3_MASK BIT(VIRT_GIC_VERSION
[PATCH RESEND v2] hw/i2c: Enable an id for the pca954x devices
This allows the devices to be more readily found and specified.
Without setting the name field, they can only be found by device type
name, which doesn't let you specify the second of the same device type
behind a bus.
Tested: Verified that by default the device was findable with the name
'pca954x[77]', for an instance attached at that address.
Signed-off-by: Patrick Venture
Reviewed-by: Hao Wu
Reviewed-by: Philippe Mathieu-Daudé
---
v2: s/id/name/g to use name as the identifier field. left 'id' in subject for
email chain.
---
hw/i2c/i2c_mux_pca954x.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
index 3945de795c..76e69bebc5 100644
--- a/hw/i2c/i2c_mux_pca954x.c
+++ b/hw/i2c/i2c_mux_pca954x.c
@@ -20,6 +20,7 @@
#include "hw/i2c/i2c_mux_pca954x.h"
#include "hw/i2c/smbus_slave.h"
#include "hw/qdev-core.h"
+#include "hw/qdev-properties.h"
#include "hw/sysbus.h"
#include "qemu/log.h"
#include "qemu/module.h"
@@ -43,6 +44,8 @@ typedef struct Pca954xState {
bool enabled[PCA9548_CHANNEL_COUNT];
I2CBus *bus[PCA9548_CHANNEL_COUNT];
+
+char *name;
} Pca954xState;
/*
@@ -181,6 +184,17 @@ static void pca9548_class_init(ObjectClass *klass, void
*data)
s->nchans = PCA9548_CHANNEL_COUNT;
}
+static void pca954x_realize(DeviceState *dev, Error **errp)
+{
+Pca954xState *s = PCA954X(dev);
+DeviceState *d = DEVICE(s);
+if (s->name) {
+d->id = g_strdup(s->name);
+} else {
+d->id = g_strdup_printf("pca954x[%x]", s->parent.i2c.address);
+}
+}
+
static void pca954x_init(Object *obj)
{
Pca954xState *s = PCA954X(obj);
@@ -197,6 +211,11 @@ static void pca954x_init(Object *obj)
}
}
+static Property pca954x_props[] = {
+DEFINE_PROP_STRING("nane", Pca954xState, name),
+DEFINE_PROP_END_OF_LIST()
+};
+
static void pca954x_class_init(ObjectClass *klass, void *data)
{
I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
@@ -209,9 +228,12 @@ static void pca954x_class_init(ObjectClass *klass, void
*data)
rc->phases.enter = pca954x_enter_reset;
dc->desc = "Pca954x i2c-mux";
+dc->realize = pca954x_realize;
k->write_data = pca954x_write_data;
k->receive_byte = pca954x_read_byte;
+
+device_class_set_props(dc, pca954x_props);
}
static const TypeInfo pca954x_info[] = {
--
2.40.0.rc1.284.g88254d51c5-goog
Re: [PATCH RESEND] hw/i2c: Enable an id for the pca954x devices
On Tue, Mar 21, 2023 at 6:41 PM Corey Minyard wrote:
> On Tue, Mar 21, 2023 at 11:27:44AM -0700, Patrick Venture wrote:
> > This allows the devices to be more readily found and specified.
> > Without setting the id field, they can only be found by device type
> > name, which doesn't let you specify the second of the same device type
> > behind a bus.
>
> So basically, this helps you find a specific device if you have more
> than one of them. Yeah, probably a good idea in this case.
>
> >
> > Tested: Verified that by default the device was findable with the id
> > 'pca954x[77]', for an instance attached at that address.
> >
> > Signed-off-by: Patrick Venture
> > Reviewed-by: Hao Wu
> > Reviewed-by: Philippe Mathieu-Daudé
> > ---
> > hw/i2c/i2c_mux_pca954x.c | 22 ++
> > 1 file changed, 22 insertions(+)
> >
> > diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
> > index a9517b612a..4f8c2d6ae1 100644
> > --- a/hw/i2c/i2c_mux_pca954x.c
> > +++ b/hw/i2c/i2c_mux_pca954x.c
> > @@ -20,6 +20,7 @@
> > #include "hw/i2c/i2c_mux_pca954x.h"
> > #include "hw/i2c/smbus_slave.h"
> > #include "hw/qdev-core.h"
> > +#include "hw/qdev-properties.h"
> > #include "hw/sysbus.h"
> > #include "qemu/log.h"
> > #include "qemu/module.h"
> > @@ -43,6 +44,8 @@ typedef struct Pca954xState {
> >
> > bool enabled[PCA9548_CHANNEL_COUNT];
> > I2CBus *bus[PCA9548_CHANNEL_COUNT];
> > +
> > +char *id;
> > } Pca954xState;
> >
> > /*
> > @@ -181,6 +184,17 @@ static void pca9548_class_init(ObjectClass *klass,
> void *data)
> > s->nchans = PCA9548_CHANNEL_COUNT;
> > }
> >
> > +static void pca954x_realize(DeviceState *dev, Error **errp)
> > +{
> > +Pca954xState *s = PCA954X(dev);
> > +DeviceState *d = DEVICE(s);
> > +if (s->id) {
> > +d->id = g_strdup(s->id);
> > +} else {
> > +d->id = g_strdup_printf("pca954x[%x]", s->parent.i2c.address);
> > +}
> > +}
> > +
> > static void pca954x_init(Object *obj)
> > {
> > Pca954xState *s = PCA954X(obj);
> > @@ -197,6 +211,11 @@ static void pca954x_init(Object *obj)
> > }
> > }
> >
> > +static Property pca954x_props[] = {
> > +DEFINE_PROP_STRING("id", Pca954xState, id),
> > +DEFINE_PROP_END_OF_LIST()
> > +};
>
> There is already an "id=" thing in qemu for doing links between front
> ends and back ends. That's probably not the best name to use. Several
> devices, like network devices, use "name" for identification in the
> monitor.
>
So I should change it to name? I'm ok with that. I think bus would be
slightly confusing. "id" was short and easy. Will send a v2.
>
> -corey
>
> > +
> > static void pca954x_class_init(ObjectClass *klass, void *data)
> > {
> > I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
> > @@ -209,9 +228,12 @@ static void pca954x_class_init(ObjectClass *klass,
> void *data)
> > rc->phases.enter = pca954x_enter_reset;
> >
> > dc->desc = "Pca954x i2c-mux";
> > +dc->realize = pca954x_realize;
> >
> > k->write_data = pca954x_write_data;
> > k->receive_byte = pca954x_read_byte;
> > +
> > +device_class_set_props(dc, pca954x_props);
> > }
> >
> > static const TypeInfo pca954x_info[] = {
> > --
> > 2.35.1.894.gb6a874cedc-goog
> >
>
[PATCH RESEND] hw/i2c: Enable an id for the pca954x devices
This allows the devices to be more readily found and specified.
Without setting the id field, they can only be found by device type
name, which doesn't let you specify the second of the same device type
behind a bus.
Tested: Verified that by default the device was findable with the id
'pca954x[77]', for an instance attached at that address.
Signed-off-by: Patrick Venture
Reviewed-by: Hao Wu
Reviewed-by: Philippe Mathieu-Daud??
---
hw/i2c/i2c_mux_pca954x.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
index a9517b612a..4f8c2d6ae1 100644
--- a/hw/i2c/i2c_mux_pca954x.c
+++ b/hw/i2c/i2c_mux_pca954x.c
@@ -20,6 +20,7 @@
#include "hw/i2c/i2c_mux_pca954x.h"
#include "hw/i2c/smbus_slave.h"
#include "hw/qdev-core.h"
+#include "hw/qdev-properties.h"
#include "hw/sysbus.h"
#include "qemu/log.h"
#include "qemu/module.h"
@@ -43,6 +44,8 @@ typedef struct Pca954xState {
bool enabled[PCA9548_CHANNEL_COUNT];
I2CBus *bus[PCA9548_CHANNEL_COUNT];
+
+char *id;
} Pca954xState;
/*
@@ -181,6 +184,17 @@ static void pca9548_class_init(ObjectClass *klass, void
*data)
s->nchans = PCA9548_CHANNEL_COUNT;
}
+static void pca954x_realize(DeviceState *dev, Error **errp)
+{
+Pca954xState *s = PCA954X(dev);
+DeviceState *d = DEVICE(s);
+if (s->id) {
+d->id = g_strdup(s->id);
+} else {
+d->id = g_strdup_printf("pca954x[%x]", s->parent.i2c.address);
+}
+}
+
static void pca954x_init(Object *obj)
{
Pca954xState *s = PCA954X(obj);
@@ -197,6 +211,11 @@ static void pca954x_init(Object *obj)
}
}
+static Property pca954x_props[] = {
+DEFINE_PROP_STRING("id", Pca954xState, id),
+DEFINE_PROP_END_OF_LIST()
+};
+
static void pca954x_class_init(ObjectClass *klass, void *data)
{
I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
@@ -209,9 +228,12 @@ static void pca954x_class_init(ObjectClass *klass, void
*data)
rc->phases.enter = pca954x_enter_reset;
dc->desc = "Pca954x i2c-mux";
+dc->realize = pca954x_realize;
k->write_data = pca954x_write_data;
k->receive_byte = pca954x_read_byte;
+
+device_class_set_props(dc, pca954x_props);
}
static const TypeInfo pca954x_info[] = {
--
2.35.1.894.gb6a874cedc-goog
qtest failure mode
Hi; Recently I was debugging why a patch broke all my qtests, and the issue it turns out was that qemu was dying in runtime. The qtests complaint was that the assertion failure on not having valid sockets. I was curious if anyone else has experienced this, and if there's a plan (or a different target) that would have the qtest detect qemu crash and report that failure somewhere? Patrick
Re: Weird qtest FileNotFoundError errors
On Wed, Nov 9, 2022 at 11:20 AM Patrick Venture wrote: > Hi all, > > I've been trying to debug qtest problems recently. I have seen the assert > socket failures a bunch now and am digging into why. I've also seen this a > lot and I'm curious if anyone has any ideas: > > ./configure --target-list=aarch64-softmmu,aarch64-linux-user > make -i check-report-qtest-aarch64.junit.xml > > Exception in callback TestHarness._run_tests..test_done( finishe...r directory')>) at > /usr/local/google/git/workspaces/qemu_71/meson/mesonbuild/mtest.py:1843 > handle: .test_done( finishe...r directory')>) at > /usr/local/google/git/workspaces/qemu_71/meson/mesonbuild/mtest.py:1843> > Traceback (most recent call last): > File "/usr/lib/python3.10/asyncio/events.py", line 80, in _run > self._context.run(self._callback, *self._args) > File > "/usr/local/google/git/workspaces/qemu_71/meson/mesonbuild/mtest.py", line > 1845, in test_done > f.result() > File > "/usr/local/google/git/workspaces/qemu_71/meson/mesonbuild/mtest.py", line > 1840, in run_test > res = await test.run(self) > File > "/usr/local/google/git/workspaces/qemu_71/meson/mesonbuild/mtest.py", line > 1384, in run > await self._run_cmd(harness, cmd) > File > "/usr/local/google/git/workspaces/qemu_71/meson/mesonbuild/mtest.py", line > 1438, in _run_cmd > p = await self._run_subprocess(cmd + extra_cmd, > File > "/usr/local/google/git/workspaces/qemu_71/meson/mesonbuild/mtest.py", line > 1412, in _run_subprocess > p = await asyncio.create_subprocess_exec(*args, > File "/usr/lib/python3.10/asyncio/subprocess.py", line 218, in > create_subprocess_exec > transport, protocol = await loop.subprocess_exec( > File "/usr/lib/python3.10/asyncio/base_events.py", line 1667, in > subprocess_exec > transport = await self._make_subprocess_transport( > File "/usr/lib/python3.10/asyncio/unix_events.py", line 207, in > _make_subprocess_transport > transp = _UnixSubprocessTransport(self, protocol, args, shell, > File "/usr/lib/python3.10/asyncio/base_subprocess.py", line 36, in > __init__ > self._start(args=args, shell=shell, stdin=stdin, stdout=stdout, > File "/usr/lib/python3.10/asyncio/unix_events.py", line 799, in _start > self._proc = subprocess.Popen( > File "/usr/lib/python3.10/subprocess.py", line 969, in __init__ > self._execute_child(args, executable, preexec_fn, close_fds, > File "/usr/lib/python3.10/subprocess.py", line 1845, in _execute_child > raise child_exception_type(errno_num, err_msg, err_filename) > FileNotFoundError: [Errno 2] No such file or directory: > '/usr/local/google/git/workspaces/qemu_71/build/tests/qtest/arm-cpu-features' > > In the past I've seen it unable to find other qtest files. I'm a bit > stumped as to why this would be flaky about finding the test files it needs. > If the problem is that I didn't run the regular make first. Which it looks like it, since I Just ran the normal "make" and saw: [864/867] Compiling C object tests/qtest/arm-cpu-features.p/arm-cpu-features.c.o [865/867] Linking target tests/qtest/arm-cpu-features I'm going to be sad. I don't know why making the test target wouldn't trigger the thing to be built, but I'm definitely starting to think that's my problem. > > Patrick >
Weird qtest FileNotFoundError errors
Hi all, I've been trying to debug qtest problems recently. I have seen the assert socket failures a bunch now and am digging into why. I've also seen this a lot and I'm curious if anyone has any ideas: ./configure --target-list=aarch64-softmmu,aarch64-linux-user make -i check-report-qtest-aarch64.junit.xml Exception in callback TestHarness._run_tests..test_done() at /usr/local/google/git/workspaces/qemu_71/meson/mesonbuild/mtest.py:1843 handle: .test_done() at /usr/local/google/git/workspaces/qemu_71/meson/mesonbuild/mtest.py:1843> Traceback (most recent call last): File "/usr/lib/python3.10/asyncio/events.py", line 80, in _run self._context.run(self._callback, *self._args) File "/usr/local/google/git/workspaces/qemu_71/meson/mesonbuild/mtest.py", line 1845, in test_done f.result() File "/usr/local/google/git/workspaces/qemu_71/meson/mesonbuild/mtest.py", line 1840, in run_test res = await test.run(self) File "/usr/local/google/git/workspaces/qemu_71/meson/mesonbuild/mtest.py", line 1384, in run await self._run_cmd(harness, cmd) File "/usr/local/google/git/workspaces/qemu_71/meson/mesonbuild/mtest.py", line 1438, in _run_cmd p = await self._run_subprocess(cmd + extra_cmd, File "/usr/local/google/git/workspaces/qemu_71/meson/mesonbuild/mtest.py", line 1412, in _run_subprocess p = await asyncio.create_subprocess_exec(*args, File "/usr/lib/python3.10/asyncio/subprocess.py", line 218, in create_subprocess_exec transport, protocol = await loop.subprocess_exec( File "/usr/lib/python3.10/asyncio/base_events.py", line 1667, in subprocess_exec transport = await self._make_subprocess_transport( File "/usr/lib/python3.10/asyncio/unix_events.py", line 207, in _make_subprocess_transport transp = _UnixSubprocessTransport(self, protocol, args, shell, File "/usr/lib/python3.10/asyncio/base_subprocess.py", line 36, in __init__ self._start(args=args, shell=shell, stdin=stdin, stdout=stdout, File "/usr/lib/python3.10/asyncio/unix_events.py", line 799, in _start self._proc = subprocess.Popen( File "/usr/lib/python3.10/subprocess.py", line 969, in __init__ self._execute_child(args, executable, preexec_fn, close_fds, File "/usr/lib/python3.10/subprocess.py", line 1845, in _execute_child raise child_exception_type(errno_num, err_msg, err_filename) FileNotFoundError: [Errno 2] No such file or directory: '/usr/local/google/git/workspaces/qemu_71/build/tests/qtest/arm-cpu-features' In the past I've seen it unable to find other qtest files. I'm a bit stumped as to why this would be flaky about finding the test files it needs. Patrick
[PATCH RESEND] hw/i2c: Enable an id for the pca954x devices
This allows the devices to be more readily found and specified.
Without setting the id field, they can only be found by device type
name, which doesn't let you specify the second of the same device type
behind a bus.
Tested: Verified that by default the device was findable with the id
'pca954x[77]', for an instance attached at that address.
Signed-off-by: Patrick Venture
Reviewed-by: Hao Wu
---
hw/i2c/i2c_mux_pca954x.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
index a9517b612a..4f8c2d6ae1 100644
--- a/hw/i2c/i2c_mux_pca954x.c
+++ b/hw/i2c/i2c_mux_pca954x.c
@@ -20,6 +20,7 @@
#include "hw/i2c/i2c_mux_pca954x.h"
#include "hw/i2c/smbus_slave.h"
#include "hw/qdev-core.h"
+#include "hw/qdev-properties.h"
#include "hw/sysbus.h"
#include "qemu/log.h"
#include "qemu/module.h"
@@ -43,6 +44,8 @@ typedef struct Pca954xState {
bool enabled[PCA9548_CHANNEL_COUNT];
I2CBus *bus[PCA9548_CHANNEL_COUNT];
+
+char *id;
} Pca954xState;
/*
@@ -181,6 +184,17 @@ static void pca9548_class_init(ObjectClass *klass, void
*data)
s->nchans = PCA9548_CHANNEL_COUNT;
}
+static void pca954x_realize(DeviceState *dev, Error **errp)
+{
+Pca954xState *s = PCA954X(dev);
+DeviceState *d = DEVICE(s);
+if (s->id) {
+d->id = g_strdup(s->id);
+} else {
+d->id = g_strdup_printf("pca954x[%x]", s->parent.i2c.address);
+}
+}
+
static void pca954x_init(Object *obj)
{
Pca954xState *s = PCA954X(obj);
@@ -197,6 +211,11 @@ static void pca954x_init(Object *obj)
}
}
+static Property pca954x_props[] = {
+DEFINE_PROP_STRING("id", Pca954xState, id),
+DEFINE_PROP_END_OF_LIST()
+};
+
static void pca954x_class_init(ObjectClass *klass, void *data)
{
I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
@@ -209,9 +228,12 @@ static void pca954x_class_init(ObjectClass *klass, void
*data)
rc->phases.enter = pca954x_enter_reset;
dc->desc = "Pca954x i2c-mux";
+dc->realize = pca954x_realize;
k->write_data = pca954x_write_data;
k->receive_byte = pca954x_read_byte;
+
+device_class_set_props(dc, pca954x_props);
}
static const TypeInfo pca954x_info[] = {
--
2.35.1.894.gb6a874cedc-goog
Re: [PATCH] tests/qtest: npcm7xx-emc-test: Skip checking MAC
On Thu, Oct 6, 2022 at 10:58 AM Patrick Venture wrote: > > > On Tue, Sep 20, 2022 at 12:00 AM Thomas Huth wrote: > >> On 20/09/2022 00.37, Patrick Venture wrote: >> > >> > >> > On Mon, Sep 19, 2022 at 5:44 AM Thomas Huth > > <mailto:th...@redhat.com>> wrote: >> > >> > On 06/09/2022 18.31, Patrick Venture wrote: >> > > The register tests walks all the registers to verify they are >> initially >> > > 0 when appropriate. However, if the MAC address is set in the >> register >> > >space, this should not be checked against 0. >> > > >> > > Reviewed-by: Hao Wu > wuhao...@google.com>> >> > > Change-Id: I02426e39bdab33ceedd42c49d233e8680d4ec058 >> > >> > What's that change-id good for? >> > >> > >> > Oops, sorry about that. I can send out a v2 without it, or during >> > application someone can nicely trim it? :) >> >> I can take the patch through my qtest branch - I'll drop the line there. >> >> > Basically ack, but one question: Where should that non-zero MAC >> address >> > come >> > from / when did you hit a problem here? If QEMU is started without >> any mac >> > settings at all (like it is done here), the register never contains >> a >> > non-zero value, does it? >> > >> > >> > So, there's a bug in the emc device presently where that value isn't >> set >> > when it should be. I have that bug fixed, but for whatever reason, >> probably >> > not enough caffeine, I didn't bundle the two patches together. >> >> OK, makes sense now, thanks for the explanation! >> > > The follow-on patch was just applied to arm.next, so I wanted to check if > this was applied to your .next or if you wanted a v2. > Nevermind, sorry for the spam - I already saw it in a PULL but forgot to update my internal tracking. Thanks! > > >> >> Thomas >> >> >>
Re: [PATCH] tests/qtest: npcm7xx-emc-test: Skip checking MAC
On Tue, Sep 20, 2022 at 12:00 AM Thomas Huth wrote: > On 20/09/2022 00.37, Patrick Venture wrote: > > > > > > On Mon, Sep 19, 2022 at 5:44 AM Thomas Huth > <mailto:th...@redhat.com>> wrote: > > > > On 06/09/2022 18.31, Patrick Venture wrote: > > > The register tests walks all the registers to verify they are > initially > > > 0 when appropriate. However, if the MAC address is set in the > register > > >space, this should not be checked against 0. > > > > > > Reviewed-by: Hao Wu wuhao...@google.com>> > > > Change-Id: I02426e39bdab33ceedd42c49d233e8680d4ec058 > > > > What's that change-id good for? > > > > > > Oops, sorry about that. I can send out a v2 without it, or during > > application someone can nicely trim it? :) > > I can take the patch through my qtest branch - I'll drop the line there. > > > Basically ack, but one question: Where should that non-zero MAC > address > > come > > from / when did you hit a problem here? If QEMU is started without > any mac > > settings at all (like it is done here), the register never contains a > > non-zero value, does it? > > > > > > So, there's a bug in the emc device presently where that value isn't set > > when it should be. I have that bug fixed, but for whatever reason, > probably > > not enough caffeine, I didn't bundle the two patches together. > > OK, makes sense now, thanks for the explanation! > The follow-on patch was just applied to arm.next, so I wanted to check if this was applied to your .next or if you wanted a v2. > > Thomas > > >
[PATCH v2] hw/net: npcm7xx_emc: set MAC in register space
The MAC address set from Qemu wasn't being saved into the register space.
Reviewed-by: Hao Wu
Signed-off-by: Patrick Venture
---
v2: only set the registers from qemu on reset
once registers set, only read and write to them
---
hw/net/npcm7xx_emc.c | 30 +++---
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/hw/net/npcm7xx_emc.c b/hw/net/npcm7xx_emc.c
index 7c86bb52e5..a33f8c7b23 100644
--- a/hw/net/npcm7xx_emc.c
+++ b/hw/net/npcm7xx_emc.c
@@ -112,6 +112,16 @@ static void emc_reset(NPCM7xxEMCState *emc)
emc->tx_active = false;
emc->rx_active = false;
+
+/* Set the MAC address in the register space. */
+uint32_t value = (emc->conf.macaddr.a[0] << 24) |
+(emc->conf.macaddr.a[1] << 16) |
+(emc->conf.macaddr.a[2] << 8) |
+emc->conf.macaddr.a[3];
+emc->regs[REG_CAMM_BASE] = value;
+
+value = (emc->conf.macaddr.a[4] << 24) | (emc->conf.macaddr.a[5] << 16);
+emc->regs[REG_CAML_BASE] = value;
}
static void npcm7xx_emc_reset(DeviceState *dev)
@@ -432,13 +442,25 @@ static bool emc_receive_filter1(NPCM7xxEMCState *emc,
const uint8_t *buf,
}
case ETH_PKT_UCAST: {
bool matches;
+uint32_t value;
+struct MACAddr mac;
if (emc->regs[REG_CAMCMR] & REG_CAMCMR_AUP) {
return true;
}
+
+value = emc->regs[REG_CAMM_BASE];
+mac.a[0] = value >> 24;
+mac.a[1] = value >> 16;
+mac.a[2] = value >> 8;
+mac.a[3] = value >> 0;
+value = emc->regs[REG_CAML_BASE];
+mac.a[4] = value >> 24;
+mac.a[5] = value >> 16;
+
matches = ((emc->regs[REG_CAMCMR] & REG_CAMCMR_ECMP) &&
/* We only support one CAM register, CAM0. */
(emc->regs[REG_CAMEN] & (1 << 0)) &&
- memcmp(buf, emc->conf.macaddr.a, ETH_ALEN) == 0);
+ memcmp(buf, mac.a, ETH_ALEN) == 0);
if (emc->regs[REG_CAMCMR] & REG_CAMCMR_CCAM) {
*fail_reason = "MACADDR matched, comparison complemented";
return !matches;
@@ -661,15 +683,9 @@ static void npcm7xx_emc_write(void *opaque, hwaddr offset,
break;
case REG_CAMM_BASE + 0:
emc->regs[reg] = value;
-emc->conf.macaddr.a[0] = value >> 24;
-emc->conf.macaddr.a[1] = value >> 16;
-emc->conf.macaddr.a[2] = value >> 8;
-emc->conf.macaddr.a[3] = value >> 0;
break;
case REG_CAML_BASE + 0:
emc->regs[reg] = value;
-emc->conf.macaddr.a[4] = value >> 24;
-emc->conf.macaddr.a[5] = value >> 16;
break;
case REG_MCMDR: {
uint32_t prev;
--
2.38.0.rc1.362.ged0d419d3c-goog
Re: [PATCH] hw/net: npcm7xx_emc: set MAC in register space
On Thu, Sep 29, 2022 at 8:54 AM Peter Maydell wrote: > On Thu, 29 Sept 2022 at 16:28, Patrick Venture wrote: > > On Mon, Sep 26, 2022 at 8:45 AM Patrick Venture > wrote: > >>> I think what Jason is suggesting is that that should depend on what > >>> the real hardware does. On a physical board, if the guest sets the > >>> MAC address, and then you power-cycle the hardware, does the MAC > >>> that it set still persist after powercycle ? Does the guest writing > >>> to these MAC registers correspond to writing to an EEPROM ? > >> > >> > >> Thanks, Peter - we've reached out to the vendor off-list to seek out > some details, I'll update this with a v2 when I get an answer. > > > "No, The EMC driver reset the MAC address registers during boot > cycle/reset." > > OK, I guess that's clear enough. In a real full-software-stack > setup is the MAC address setup usually done by a BIOS/firmware, > or is it done by Linux ? > The MAC address is usually set up in u-boot or by linux. The openbmc stack currently doesn't actually read what the device thinks its mac address is, but it could :) It just sets it blindly. We found this register issue because we were debugging why the MAC address we were setting in the qemu command line had no effect. Turns out, per this patch, we weren't using that value anywhere. But even with that fixed, the firmware didn't care. :) > > > So in that case, we should disregard the value the user sets in > > reset and use the value provided through Qemu. Or, should we just > > not allow Qemu to set the MAC address at all? > > I think that the behaviour for QEMU's model should be that > on reset we should reset the MAC address registers to the > user-provided value. That is, if the guest writes to the > MAC address registers then that does have an effect, but > only until the next reset. > > That gives you reasonably plausible behaviour for both: > (1) you're emulating some guest that always sets up its > own MAC address when it starts up (eg if it's done by > some BIOS-level code that you're running in the guest) > (2) you're booting a guest kernel directly that expects > that the firmware/BIOS/whatever has already set up > a MAC address -- then the MAC address provided by QEMU/the > user fills that role > > More concretely: > * on reset, set the emc->regs[] fields from emc->conf.macaddr > * when using the MAC address, always use emc->regs[], never >emc->conf.macaddr > * to handle the guest writes to the MAC registers, set >emc->regs[], but not emc->conf.macaddr > > Thanks! That actually made it much easier to parse what you wanted to match the behavior :) > Assuming that doesn't break your existing booting workloads, > of course :-) > > thanks > -- PMM >
Re: [PATCH] hw/net: npcm7xx_emc: set MAC in register space
On Mon, Sep 26, 2022 at 8:45 AM Patrick Venture wrote: > > > On Sat, Sep 24, 2022 at 2:10 AM Peter Maydell > wrote: > >> On Sat, 24 Sept 2022 at 00:42, Patrick Venture >> wrote: >> > On Thu, Sep 22, 2022 at 8:21 PM Jason Wang wrote: >> >> >> >> On Thu, Sep 22, 2022 at 8:35 PM Peter Maydell < >> peter.mayd...@linaro.org> wrote: >> >> > A question to which I don't know the answer: if the guest writes to >> >> > the device to change the MAC address, should that persist across >> >> > reset, or should reset revert the device to the original MAC address >> >> > as specified by the user on the command line or whatever ? At the >> >> > moment you have the former behaviour (and end up storing the MAC >> >> > address in two places as a result -- it would be neater to either >> >> > keep it in only one place, or else have emc->regs[] be the current >> >> > programmed MAC address and emc->conf.macaddr the value to reset to). >> >> > >> >> > I'm not sure we're consistent between device models about that, >> >> > eg the e1000 seems to reset to the initial MAC addr, but the >> >> > imx_fec keeps the guest-set value over resets. Jason, is there >> >> > a recommended "right way" to handle guest-programmable MAC addresses >> >> > over device reset ? >> >> >> >> I think it depends on the NIC. >> >> >> >> E1000 has a EEPROM interface for providing the MAC address for the >> >> ethernet controller before it can be accessed by the driver during >> >> reset. For modern Intel NICs like E810, it has similar semantics but >> >> using NVM instead of EEPROM. So the current e1000 behaviour seems to >> >> be correct (treat the initiali MAC as the one stored in the EEPROM). >> >> >> >> I guess most NIC should behave the same as having a persistent storage >> >> for MAC for the controller during reset, but I'm not sure this is the >> >> case for imx_fec. >> >> > So the first time the NIC is realized, it should take the value from >> > the command line. Then later if the guest OS updates it, it should >> > always on reset use that provided value? >> >> I think what Jason is suggesting is that that should depend on what >> the real hardware does. On a physical board, if the guest sets the >> MAC address, and then you power-cycle the hardware, does the MAC >> that it set still persist after powercycle ? Does the guest writing >> to these MAC registers correspond to writing to an EEPROM ? >> > > Thanks, Peter - we've reached out to the vendor off-list to seek out some > details, I'll update this with a v2 when I get an answer. > "No, The EMC driver reset the MAC address registers during boot cycle/reset." So in that case, we should disregard the value the user sets in reset and use the value provided through Qemu. Or, should we just not allow Qemu to set the MAC address at all? > >> >> thanks >> -- PMM >> >
Re: [PATCH] hw/net: npcm7xx_emc: set MAC in register space
On Sat, Sep 24, 2022 at 2:10 AM Peter Maydell wrote: > On Sat, 24 Sept 2022 at 00:42, Patrick Venture wrote: > > On Thu, Sep 22, 2022 at 8:21 PM Jason Wang wrote: > >> > >> On Thu, Sep 22, 2022 at 8:35 PM Peter Maydell > wrote: > >> > A question to which I don't know the answer: if the guest writes to > >> > the device to change the MAC address, should that persist across > >> > reset, or should reset revert the device to the original MAC address > >> > as specified by the user on the command line or whatever ? At the > >> > moment you have the former behaviour (and end up storing the MAC > >> > address in two places as a result -- it would be neater to either > >> > keep it in only one place, or else have emc->regs[] be the current > >> > programmed MAC address and emc->conf.macaddr the value to reset to). > >> > > >> > I'm not sure we're consistent between device models about that, > >> > eg the e1000 seems to reset to the initial MAC addr, but the > >> > imx_fec keeps the guest-set value over resets. Jason, is there > >> > a recommended "right way" to handle guest-programmable MAC addresses > >> > over device reset ? > >> > >> I think it depends on the NIC. > >> > >> E1000 has a EEPROM interface for providing the MAC address for the > >> ethernet controller before it can be accessed by the driver during > >> reset. For modern Intel NICs like E810, it has similar semantics but > >> using NVM instead of EEPROM. So the current e1000 behaviour seems to > >> be correct (treat the initiali MAC as the one stored in the EEPROM). > >> > >> I guess most NIC should behave the same as having a persistent storage > >> for MAC for the controller during reset, but I'm not sure this is the > >> case for imx_fec. > > > So the first time the NIC is realized, it should take the value from > > the command line. Then later if the guest OS updates it, it should > > always on reset use that provided value? > > I think what Jason is suggesting is that that should depend on what > the real hardware does. On a physical board, if the guest sets the > MAC address, and then you power-cycle the hardware, does the MAC > that it set still persist after powercycle ? Does the guest writing > to these MAC registers correspond to writing to an EEPROM ? > Thanks, Peter - we've reached out to the vendor off-list to seek out some details, I'll update this with a v2 when I get an answer. > > thanks > -- PMM >
Re: [PATCH] hw/net: npcm7xx_emc: set MAC in register space
On Thu, Sep 22, 2022 at 8:21 PM Jason Wang wrote: > On Thu, Sep 22, 2022 at 8:35 PM Peter Maydell > wrote: > > > > On Thu, 22 Sept 2022 at 00:47, Patrick Venture > wrote: > > > > > > The MAC address set from Qemu wasn't being saved into the register > space. > > > > > > Reviewed-by: Hao Wu > > > Signed-off-by: Patrick Venture > > > > > @@ -112,6 +115,18 @@ static void emc_reset(NPCM7xxEMCState *emc) > > > > > > emc->tx_active = false; > > > emc->rx_active = false; > > > + > > > +/* Set the MAC address in the register space. */ > > > +uint32_t value = (emc->conf.macaddr.a[0] << 24) | > > > +(emc->conf.macaddr.a[1] << 16) | > > > +(emc->conf.macaddr.a[2] << 8) | > > > +emc->conf.macaddr.a[3]; > > > +npcm7xx_emc_write(emc, REG_CAMM_BASE * sizeof(uint32_t), value, > > > + sizeof(uint32_t)); > > > + > > > +value = (emc->conf.macaddr.a[4] << 24) | (emc->conf.macaddr.a[5] > << 16); > > > +npcm7xx_emc_write(emc, REG_CAML_BASE * sizeof(uint32_t), value, > > > + sizeof(uint32_t)); > > > > If I understand correctly, the issue here is that > emc->regs[REG_CAMM_BASE] > > and emc->regs[REG_CAML_BASE] aren't being reset correctly. If so, > > I think the better approach is to simply reset them here, without > > going through the register-write function, the same way we already > > do for the handful of other registers which have non-zero reset values. > > That's the way other devices seem to do it. > > > > A question to which I don't know the answer: if the guest writes to > > the device to change the MAC address, should that persist across > > reset, or should reset revert the device to the original MAC address > > as specified by the user on the command line or whatever ? At the > > moment you have the former behaviour (and end up storing the MAC > > address in two places as a result -- it would be neater to either > > keep it in only one place, or else have emc->regs[] be the current > > programmed MAC address and emc->conf.macaddr the value to reset to). > > > > I'm not sure we're consistent between device models about that, > > eg the e1000 seems to reset to the initial MAC addr, but the > > imx_fec keeps the guest-set value over resets. Jason, is there > > a recommended "right way" to handle guest-programmable MAC addresses > > over device reset ? > > I think it depends on the NIC. > > E1000 has a EEPROM interface for providing the MAC address for the > ethernet controller before it can be accessed by the driver during > reset. For modern Intel NICs like E810, it has similar semantics but > using NVM instead of EEPROM. So the current e1000 behaviour seems to > be correct (treat the initiali MAC as the one stored in the EEPROM). > > I guess most NIC should behave the same as having a persistent storage > for MAC for the controller during reset, but I'm not sure this is the > case for imx_fec. > So the first time the NIC is realized, it should take the value from the command line. Then later if the guest OS updates it, it should always on reset use that provided value? > > (Anyhow, if we change the behaviour we need keep migration compatibility) > > Thanks > > > > > thanks > > -- PMM > > > >
[PATCH] hw/net: npcm7xx_emc: set MAC in register space
The MAC address set from Qemu wasn't being saved into the register space.
Reviewed-by: Hao Wu
Signed-off-by: Patrick Venture
---
hw/net/npcm7xx_emc.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/hw/net/npcm7xx_emc.c b/hw/net/npcm7xx_emc.c
index 7c86bb52e5..6be1008529 100644
--- a/hw/net/npcm7xx_emc.c
+++ b/hw/net/npcm7xx_emc.c
@@ -96,6 +96,9 @@ static const char *emc_reg_name(int regno)
#undef REG
}
+static void npcm7xx_emc_write(void *opaque, hwaddr offset,
+ uint64_t v, unsigned size);
+
static void emc_reset(NPCM7xxEMCState *emc)
{
trace_npcm7xx_emc_reset(emc->emc_num);
@@ -112,6 +115,18 @@ static void emc_reset(NPCM7xxEMCState *emc)
emc->tx_active = false;
emc->rx_active = false;
+
+/* Set the MAC address in the register space. */
+uint32_t value = (emc->conf.macaddr.a[0] << 24) |
+(emc->conf.macaddr.a[1] << 16) |
+(emc->conf.macaddr.a[2] << 8) |
+emc->conf.macaddr.a[3];
+npcm7xx_emc_write(emc, REG_CAMM_BASE * sizeof(uint32_t), value,
+ sizeof(uint32_t));
+
+value = (emc->conf.macaddr.a[4] << 24) | (emc->conf.macaddr.a[5] << 16);
+npcm7xx_emc_write(emc, REG_CAML_BASE * sizeof(uint32_t), value,
+ sizeof(uint32_t));
}
static void npcm7xx_emc_reset(DeviceState *dev)
--
2.37.3.998.g577e59143f-goog
Re: [PATCH] tests/qtest: npcm7xx-emc-test: Skip checking MAC
On Mon, Sep 19, 2022 at 5:44 AM Thomas Huth wrote:
> On 06/09/2022 18.31, Patrick Venture wrote:
> > The register tests walks all the registers to verify they are initially
> > 0 when appropriate. However, if the MAC address is set in the register
> >space, this should not be checked against 0.
> >
> > Reviewed-by: Hao Wu
> > Change-Id: I02426e39bdab33ceedd42c49d233e8680d4ec058
>
> What's that change-id good for?
>
Oops, sorry about that. I can send out a v2 without it, or during
application someone can nicely trim it? :)
>
> > Signed-off-by: Patrick Venture
> > ---
> > tests/qtest/npcm7xx_emc-test.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/qtest/npcm7xx_emc-test.c
> b/tests/qtest/npcm7xx_emc-test.c
> > index 7c435ac915..207d8515b7 100644
> > --- a/tests/qtest/npcm7xx_emc-test.c
> > +++ b/tests/qtest/npcm7xx_emc-test.c
> > @@ -378,7 +378,8 @@ static void test_init(gconstpointer test_data)
> >
> > #undef CHECK_REG
> >
> > -for (i = 0; i < NUM_CAMML_REGS; ++i) {
> > +/* Skip over the MAC address registers, which is BASE+0 */
> > +for (i = 1; i < NUM_CAMML_REGS; ++i) {
> > g_assert_cmpuint(emc_read(qts, mod, REG_CAMM_BASE + i * 2), ==,
> >0);
> > g_assert_cmpuint(emc_read(qts, mod, REG_CAML_BASE + i * 2), ==,
>
> Basically ack, but one question: Where should that non-zero MAC address
> come
> from / when did you hit a problem here? If QEMU is started without any mac
> settings at all (like it is done here), the register never contains a
> non-zero value, does it?
>
So, there's a bug in the emc device presently where that value isn't set
when it should be. I have that bug fixed, but for whatever reason,
probably not enough caffeine, I didn't bundle the two patches together.
>
> Thomas
>
>
Re: Seeing qtest assertion failure with 7.1
On Wed, Sep 7, 2022 at 10:40 AM Peter Maydell wrote: > On Wed, 7 Sept 2022 at 16:39, Patrick Venture wrote: > > > > # Start of nvme tests > > # Start of pci-device tests > > # Start of pci-device-tests tests > > # starting QEMU: exec ./qemu-system-aarch64 -qtest > unix:/tmp/qtest-1431.sock -qtest-log /dev/null -chardev > socket,path=/tmp/qtest-1431.qmp,id=char0 -mon chardev=char0,mode=control > -display none -M virt, -cpu max -drive > id=drv0,if=none,file=null-co://,file.read-zeroes=on,format=raw -object > memory-backend-ram,id=pmr0,share=on,size=8 -device > nvme,addr=04.0,drive=drv0,serial=foo -accel qtest > > > > # > ERROR:../../src/qemu/tests/qtest/libqtest.c:338:qtest_init_without_qmp_handshake: > assertion failed: (s->fd >= 0 && s->qmp_fd >= 0) > > stderr: > > double free or corruption (out) > > socket_accept failed: Resource temporarily unavailable > > ** > > > ERROR:../../src/qemu/tests/qtest/libqtest.c:338:qtest_init_without_qmp_handshake: > assertion failed: (s->fd >= 0 && s->qmp_fd >= 0) > > ../../src/qemu/tests/qtest/libqtest.c:165: kill_qemu() detected QEMU > death from signal 6 (Aborted) (core dumped) > > > > I'm not seeing this reliably, and we haven't done a lot of digging yet, > such as enabling sanitizers, so I'll reply back to this thread with details > as I have them. > > > > Has anyone seen this before or something like it? > > Have a look in the source at what exactly the assertion > failure in libqtest.c is checking for -- IIRC it's a pretty > basic "did we open a socket fd" one. I think sometimes I > used to see something like this if there's an old stale socket > lying around in the test directory and the randomly generated > socket filename happens to clash with it. > Thanks for the debugging tip! I can't reproduce it at this point. I saw it 2-3 times, and now not at all. So more than likely it's exactly what you're describing. > > Everything after that is probably follow-on errors from the > tests not being terribly clean about error handling. > > Are you running 'make check' with a -j option for parallel? > (This is supposed to work, and it's the standard way I run > 'make check', so if it's flaky we need to fix it, but it > would be interesting to know if the issue repros at -j1.) > Since it's not reproducing reliably -- and I haven't actually seen it since the first few instances (and it was unrelated to those patches in flight), I'll have to sit on further debug until we reproduce it and then I can let you know, but this seems to be flaky at the point where it's hard to detect. > > -- PMM >
Re: [PATCH] crypto/block-luks: always set splitkeylen to 0
On Wed, Sep 7, 2022 at 9:34 AM Daniel P. Berrangé wrote: > On Wed, Sep 07, 2022 at 09:21:25AM -0700, Patrick Venture wrote: > > This was caught by a sanitized build, that was perhaps oversensitive. > > > > Signed-off-by: Patrick Venture > > --- > > crypto/block-luks.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/crypto/block-luks.c b/crypto/block-luks.c > > index f62be6836b..8633fb7e9f 100644 > > --- a/crypto/block-luks.c > > +++ b/crypto/block-luks.c > > @@ -729,7 +729,7 @@ qcrypto_block_luks_store_key(QCryptoBlock *block, > > QCryptoBlockLUKS *luks = block->opaque; > > QCryptoBlockLUKSKeySlot *slot; > > g_autofree uint8_t *splitkey = NULL; > > -size_t splitkeylen; > > +size_t splitkeylen = 0; > > g_autofree uint8_t *slotkey = NULL; > > g_autoptr(QCryptoCipher) cipher = NULL; > > g_autoptr(QCryptoIVGen) ivgen = NULL; > > @@ -901,7 +901,7 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, > > QCryptoBlockLUKS *luks = block->opaque; > > const QCryptoBlockLUKSKeySlot *slot; > > g_autofree uint8_t *splitkey = NULL; > > -size_t splitkeylen; > > +size_t splitkeylen = 0; > > g_autofree uint8_t *possiblekey = NULL; > > int rv; > > g_autoptr(QCryptoCipher) cipher = NULL; > > @@ -1147,7 +1147,7 @@ qcrypto_block_luks_erase_key(QCryptoBlock *block, > > QCryptoBlockLUKS *luks = block->opaque; > > QCryptoBlockLUKSKeySlot *slot; > > g_autofree uint8_t *garbagesplitkey = NULL; > > -size_t splitkeylen; > > +size_t splitkeylen = 0; > > size_t i; > > Error *local_err = NULL; > > int ret; > > In all three cases, splitkeylen is initialized a few lines later. > > In qcrypto_block_luks_store_key there is a 'goto cleanup' before > the initialization. The 'cleanup' code can use 'splitkeylen', but > only if 'splitkey != NULL' & this isn't possible if splitkeylen is > uninitialized. > > The other two methods have no code path where splitkeylen can be > used uninitialized. > > The tool is reporting non-existant problems AFAICT > I'm 100% certain it is. I just needed to make the changes to get a sanitized build of Qemu to build. Presumably if someone else tries building qemu with `--enable-sanitizers` they'll have to make the same non-sense adjustment. > > With regards, > Daniel > -- > |: https://berrange.com -o- > https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- > https://fstop138.berrange.com :| > |: https://entangle-photo.org-o- > https://www.instagram.com/dberrange :| > >
[PATCH] crypto/block-luks: always set splitkeylen to 0
This was caught by a sanitized build, that was perhaps oversensitive. Signed-off-by: Patrick Venture --- crypto/block-luks.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crypto/block-luks.c b/crypto/block-luks.c index f62be6836b..8633fb7e9f 100644 --- a/crypto/block-luks.c +++ b/crypto/block-luks.c @@ -729,7 +729,7 @@ qcrypto_block_luks_store_key(QCryptoBlock *block, QCryptoBlockLUKS *luks = block->opaque; QCryptoBlockLUKSKeySlot *slot; g_autofree uint8_t *splitkey = NULL; -size_t splitkeylen; +size_t splitkeylen = 0; g_autofree uint8_t *slotkey = NULL; g_autoptr(QCryptoCipher) cipher = NULL; g_autoptr(QCryptoIVGen) ivgen = NULL; @@ -901,7 +901,7 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, QCryptoBlockLUKS *luks = block->opaque; const QCryptoBlockLUKSKeySlot *slot; g_autofree uint8_t *splitkey = NULL; -size_t splitkeylen; +size_t splitkeylen = 0; g_autofree uint8_t *possiblekey = NULL; int rv; g_autoptr(QCryptoCipher) cipher = NULL; @@ -1147,7 +1147,7 @@ qcrypto_block_luks_erase_key(QCryptoBlock *block, QCryptoBlockLUKS *luks = block->opaque; QCryptoBlockLUKSKeySlot *slot; g_autofree uint8_t *garbagesplitkey = NULL; -size_t splitkeylen; +size_t splitkeylen = 0; size_t i; Error *local_err = NULL; int ret; -- 2.37.2.789.g6183377224-goog
Seeing qtest assertion failure with 7.1
# Start of nvme tests # Start of pci-device tests # Start of pci-device-tests tests # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-1431.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-1431.qmp,id=char0 -mon chardev=char0,mode=control -display none -M virt, -cpu max -drive id=drv0,if=none,file=null-co://,file.read-zeroes=on,format=raw -object memory-backend-ram,id=pmr0,share=on,size=8 -device nvme,addr=04.0,drive=drv0,serial=foo -accel qtest # ERROR:../../src/qemu/tests/qtest/libqtest.c:338:qtest_init_without_qmp_handshake: assertion failed: (s->fd >= 0 && s->qmp_fd >= 0) stderr: double free or corruption (out) socket_accept failed: Resource temporarily unavailable ** ERROR:../../src/qemu/tests/qtest/libqtest.c:338:qtest_init_without_qmp_handshake: assertion failed: (s->fd >= 0 && s->qmp_fd >= 0) ../../src/qemu/tests/qtest/libqtest.c:165: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped) I'm not seeing this reliably, and we haven't done a lot of digging yet, such as enabling sanitizers, so I'll reply back to this thread with details as I have them. Has anyone seen this before or something like it? Patrick
[PATCH] tests/qtest: npcm7xx-emc-test: Skip checking MAC
The register tests walks all the registers to verify they are initially
0 when appropriate. However, if the MAC address is set in the register
space, this should not be checked against 0.
Reviewed-by: Hao Wu
Change-Id: I02426e39bdab33ceedd42c49d233e8680d4ec058
Signed-off-by: Patrick Venture
---
tests/qtest/npcm7xx_emc-test.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/npcm7xx_emc-test.c b/tests/qtest/npcm7xx_emc-test.c
index 7c435ac915..207d8515b7 100644
--- a/tests/qtest/npcm7xx_emc-test.c
+++ b/tests/qtest/npcm7xx_emc-test.c
@@ -378,7 +378,8 @@ static void test_init(gconstpointer test_data)
#undef CHECK_REG
-for (i = 0; i < NUM_CAMML_REGS; ++i) {
+/* Skip over the MAC address registers, which is BASE+0 */
+for (i = 1; i < NUM_CAMML_REGS; ++i) {
g_assert_cmpuint(emc_read(qts, mod, REG_CAMM_BASE + i * 2), ==,
0);
g_assert_cmpuint(emc_read(qts, mod, REG_CAML_BASE + i * 2), ==,
--
2.37.2.789.g6183377224-goog
[PATCH v2] hw/nvram: Add at24c-eeprom support for small eeproms
Tested: Verified at24c02 driver happy and contents matched
expectations.
Signed-off-by: Patrick Venture
Reviewed-by: Hao Wu
---
v2: Added comment describing the new behavior.
---
hw/nvram/eeprom_at24c.c | 56 -
1 file changed, 39 insertions(+), 17 deletions(-)
diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index d695f6ae89..eb91ec6813 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -45,6 +45,8 @@ struct EEPROMState {
bool changed;
/* during WRITE, # of address bytes transfered */
uint8_t haveaddr;
+/* whether it's 8-bit addressed or 16-bit */
+bool eightbit;
uint8_t *mem;
@@ -87,7 +89,7 @@ uint8_t at24c_eeprom_recv(I2CSlave *s)
EEPROMState *ee = AT24C_EE(s);
uint8_t ret;
-if (ee->haveaddr == 1) {
+if (ee->haveaddr == 1 && !ee->eightbit) {
return 0xff;
}
@@ -104,26 +106,35 @@ int at24c_eeprom_send(I2CSlave *s, uint8_t data)
{
EEPROMState *ee = AT24C_EE(s);
-if (ee->haveaddr < 2) {
-ee->cur <<= 8;
-ee->cur |= data;
+if (ee->haveaddr < 1) {
+ee->cur = data;
ee->haveaddr++;
-if (ee->haveaddr == 2) {
-ee->cur %= ee->rsize;
+if (ee->eightbit) {
DPRINTK("Set pointer %04x\n", ee->cur);
}
+return 0;
+} else if (ee->haveaddr < 2) {
+if (!ee->eightbit) {
+ee->cur <<= 8;
+ee->cur |= data;
+ee->haveaddr++;
+if (ee->haveaddr == 2) {
+ee->cur %= ee->rsize;
+DPRINTK("Set pointer %04x\n", ee->cur);
+}
-} else {
-if (ee->writable) {
-DPRINTK("Send %02x\n", data);
-ee->mem[ee->cur] = data;
-ee->changed = true;
-} else {
-DPRINTK("Send error %02x read-only\n", data);
+return 0;
}
-ee->cur = (ee->cur + 1u) % ee->rsize;
+}
+if (ee->writable) {
+DPRINTK("Send %02x\n", data);
+ee->mem[ee->cur] = data;
+ee->changed = true;
+} else {
+DPRINTK("Send error %02x read-only\n", data);
}
+ee->cur = (ee->cur + 1u) % ee->rsize;
return 0;
}
@@ -133,12 +144,16 @@ static void at24c_eeprom_realize(DeviceState *dev, Error
**errp)
EEPROMState *ee = AT24C_EE(dev);
if (ee->blk) {
+/* blk length is a minimum of 1 sector. */
int64_t len = blk_getlength(ee->blk);
if (len != ee->rsize) {
-error_setg(errp, "%s: Backing file size %" PRId64 " != %u",
- TYPE_AT24C_EE, len, ee->rsize);
-return;
+/* for the at24c01 and at24c02, they are smaller than a sector. */
+if (ee->rsize >= BDRV_SECTOR_SIZE) {
+error_setg(errp, "%s: Backing file size %" PRId64 " != %u",
+ TYPE_AT24C_EE, len, ee->rsize);
+return;
+}
}
if (blk_set_perm(ee->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
@@ -150,6 +165,13 @@ static void at24c_eeprom_realize(DeviceState *dev, Error
**errp)
}
}
+/*
+ * The eeprom sizes are 2**x based, so if it's strictly less than 512, we
+ * expect it to be 256 or 128.
+ */
+if (ee->rsize < 512) {
+ee->eightbit = true;
+}
ee->mem = g_malloc0(ee->rsize);
}
--
2.37.0.144.g8ac04bfd2-goog
Re: [PATCH 1/8] hw/i2c/pca954x: Add method to get channels
On Mon, Jul 4, 2022 at 10:46 PM Cédric Le Goater wrote:
> On 7/4/22 23:54, Peter Delevoryas wrote:
> > I added this helper in the Aspeed machine file a while ago to help
> > initialize fuji-bmc i2c devices. This moves it to the official pca954x
> > file so that other files can use it.
> >
> > This does something very similar to pca954x_i2c_get_bus, but I think
> > this is useful when you have a very complicated dts with a lot of
> > switches, like the fuji dts.
> >
> > This convenience method lets you write code that produces a flat array
> > of I2C buses that matches the naming in the dts. After that you can just
> > add individual sensors using the flat array of I2C buses.
> >
> > See fuji_bmc_i2c_init to understand this point further.
> >
> > The fuji dts is here for reference:
> >
> >
> https://github.com/torvalds/linux/blob/40cb6373b46/arch/arm/boot/dts/aspeed-bmc-facebook-fuji.dts
> >
> > Signed-off-by: Peter Delevoryas
>
> Reviewed-by: Cédric Le Goater
>
Reviewed-by: Patrick Venture
>
> Thanks,
>
>
Neato :)
> C.
>
>
> > ---
> > hw/arm/aspeed.c | 29 +
> > hw/i2c/i2c_mux_pca954x.c | 10 ++
> > include/hw/i2c/i2c_mux_pca954x.h | 13 +
> > 3 files changed, 32 insertions(+), 20 deletions(-)
> >
> > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> > index 6fe9b13548..bee8a748ec 100644
> > --- a/hw/arm/aspeed.c
> > +++ b/hw/arm/aspeed.c
> > @@ -793,15 +793,6 @@ static void rainier_bmc_i2c_init(AspeedMachineState
> *bmc)
> > create_pca9552(soc, 15, 0x60);
> > }
> >
> > -static void get_pca9548_channels(I2CBus *bus, uint8_t mux_addr,
> > - I2CBus **channels)
> > -{
> > -I2CSlave *mux = i2c_slave_create_simple(bus, "pca9548", mux_addr);
> > -for (int i = 0; i < 8; i++) {
> > -channels[i] = pca954x_i2c_get_bus(mux, i);
> > -}
> > -}
> > -
> > #define TYPE_LM75 TYPE_TMP105
> > #define TYPE_TMP75 TYPE_TMP105
> > #define TYPE_TMP422 "tmp422"
> > @@ -814,20 +805,18 @@ static void fuji_bmc_i2c_init(AspeedMachineState
> *bmc)
> > for (int i = 0; i < 16; i++) {
> > i2c[i] = aspeed_i2c_get_bus(>i2c, i);
> > }
> > -I2CBus *i2c180 = i2c[2];
> > -I2CBus *i2c480 = i2c[8];
> > -I2CBus *i2c600 = i2c[11];
> >
> > -get_pca9548_channels(i2c180, 0x70, [16]);
> > -get_pca9548_channels(i2c480, 0x70, [24]);
> > +pca954x_i2c_get_channels(i2c[2], 0x70, "pca9548", [16]);
> > +pca954x_i2c_get_channels(i2c[8], 0x70, "pca9548", [24]);
> > /* NOTE: The device tree skips [32, 40) in the alias numbering */
> > -get_pca9548_channels(i2c600, 0x77, [40]);
> > -get_pca9548_channels(i2c[24], 0x71, [48]);
> > -get_pca9548_channels(i2c[25], 0x72, [56]);
> > -get_pca9548_channels(i2c[26], 0x76, [64]);
> > -get_pca9548_channels(i2c[27], 0x76, [72]);
> > +pca954x_i2c_get_channels(i2c[11], 0x77, "pca9548", [40]);
> > +pca954x_i2c_get_channels(i2c[24], 0x71, "pca9548", [48]);
> > +pca954x_i2c_get_channels(i2c[25], 0x72, "pca9548", [56]);
> > +pca954x_i2c_get_channels(i2c[26], 0x76, "pca9548", [64]);
> > +pca954x_i2c_get_channels(i2c[27], 0x76, "pca9548", [72]);
> > for (int i = 0; i < 8; i++) {
> > -get_pca9548_channels(i2c[40 + i], 0x76, [80 + i * 8]);
> > +pca954x_i2c_get_channels(i2c[40 + i], 0x76, "pca9548",
> > + [80 + i * 8]);
> > }
> >
> > i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4c);
> > diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
> > index 3945de795c..6b07804546 100644
> > --- a/hw/i2c/i2c_mux_pca954x.c
> > +++ b/hw/i2c/i2c_mux_pca954x.c
> > @@ -169,6 +169,16 @@ I2CBus *pca954x_i2c_get_bus(I2CSlave *mux, uint8_t
> channel)
> > return pca954x->bus[channel];
> > }
> >
> > +void pca954x_i2c_get_channels(I2CBus *bus, uint8_t address,
> > + const char *type_name, I2CBus **channels)
> > +{
> > +I2CSlave *mux = i2c_slave_create_simple(bus, type_name, address);
> > +Pca954xClass *pc = PCA954X_GET_CLASS(mux);
> > +Pca954xState *pca954x = PCA954X(mux);
> > +
> > +memcpy(channels, pca954x->bus, pc->nchans * sizeof(channels[0]));
> > +}
> > +
>
Re: [PATCH v2 09/13] hw/i2c/pmbus: Add read-only IC_DEVICE_ID support
On Wed, Jun 29, 2022 at 11:34 AM Peter Delevoryas wrote:
>
>
> > On Jun 29, 2022, at 11:04 AM, Titus Rwantare wrote:
> >
> > On Tue, 28 Jun 2022 at 20:36, Peter Delevoryas
> > wrote:
> >>
> >> Signed-off-by: Peter Delevoryas
> >> ---
> >
> >> --- a/hw/i2c/pmbus_device.c
> >> +++ b/hw/i2c/pmbus_device.c
> >> @@ -984,6 +984,11 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd)
> >> }
> >> break;
> >>
> >> +case PMBUS_IC_DEVICE_ID:
> >> +pmbus_send(pmdev, pmdev->pages[index].ic_device_id,
> >> + sizeof(pmdev->pages[index].ic_device_id));
> >> +break;
> >> +
> >
> > I don't think it's a good idea to add this here because this sends 16
> > bytes for all PMBus devices. I have at least one device that formats
> > IC_DEVICE_ID differently that I've not got permission to upstream.
> > The spec leaves the size and format up to the manufacturer, so this is
> > best done in isl_pmbus_vr.c in isl_pmbus_vr_read_byte().
> > Look at the adm1272_read_byte() which is more interesting than
> > isl_pmbus_vr one as an example.
>
> Argh, yes, you’re absolutely right. I didn’t read the spec in very
> much detail, I see now that the length is device-specific. For the
> ISL69259 I see that it’s 4 bytes, which makes sense now. This
> is not the exact datasheet for the ISL69259, but I think the IC_DEVICE_ID
> part is the same.
>
>
> https://www.renesas.com/us/en/document/dst/isl68229-isl68239-datasheet
>
> Putting the logic in isl_pmbus_vr_read_byte() is a good idea, I hadn’t
> seen the implementation in adm1272_read_byte(), that looks great,
> I’ll just add a switch on the command code and move the error message
> to the default case.
>
> >
> >
> >> case PMBUS_CLEAR_FAULTS: /* Send Byte */
> >> case PMBUS_PAGE_PLUS_WRITE: /* Block Write-only */
> >> case PMBUS_STORE_DEFAULT_ALL: /* Send Byte */
> >> diff --git a/hw/sensor/isl_pmbus_vr.c b/hw/sensor/isl_pmbus_vr.c
> >> index e11e028884..b12c46ab6d 100644
> >> --- a/hw/sensor/isl_pmbus_vr.c
> >> +++ b/hw/sensor/isl_pmbus_vr.c
> >> @@ -218,6 +218,28 @@ static void isl_pmbus_vr_class_init(ObjectClass
> *klass, void *data,
> >> k->device_num_pages = pages;
> >> }
> >>
> >> +static void isl69259_init(Object *obj)
> >> +{
> >> +static const uint8_t ic_device_id[] = {0x04, 0x00, 0x81, 0xD2,
> 0x49};
> >> +PMBusDevice *pmdev = PMBUS_DEVICE(obj);
> >> +int i;
> >> +
> >> +raa22xx_init(obj);
> >> +for (i = 0; i < pmdev->num_pages; i++) {
> >> +memcpy(pmdev->pages[i].ic_device_id, ic_device_id,
> >> + sizeof(ic_device_id));
> >> +}
> >> +}
> >> +
> >
> > We tend to set default register values in exit_reset() calls. You can
> > do something like in raa228000_exit_reset()
>
> Oh got it. If I can move the logic to isl_pmbus_vr_read_byte perhaps
> I can avoid this whole function though.
>
> >
> >
> >> diff --git a/include/hw/i2c/pmbus_device.h
> b/include/hw/i2c/pmbus_device.h
> >> index 0f4d6b3fad..aed7809841 100644
> >> --- a/include/hw/i2c/pmbus_device.h
> >> +++ b/include/hw/i2c/pmbus_device.h
> >> @@ -407,6 +407,7 @@ typedef struct PMBusPage {
> >> uint16_t mfr_max_temp_1; /* R/W word */
> >> uint16_t mfr_max_temp_2; /* R/W word */
> >> uint16_t mfr_max_temp_3; /* R/W word */
> >> +uint8_t ic_device_id[16]; /* Read-Only block-read */
> >
> > You wouldn't be able to do this here either, since the size could be
> > anything for other devices.
>
> Right, yeah I see what you mean.
>
> >
> > Thanks for the new device. It helps me see where to expand on PMBus.
>
> Thanks for adding the whole pmbus infrastructure! It’s really useful.
> And thanks for the review.
>
> Off-topic, but I’ve been meaning to reach out to you guys (Google
> engineers working on QEMU for OpenBMC) about your Nuvoton NPCM845R
> series, my team is interested in using it. I was just curious about
> the status of it and if there’s any features missing and what plans
> you have for the future, maybe we can collaborate.
>
Peter, feel free to reach out to me, or Titus, and we can sync up. I used
to work with Patrick who's now at Fb on OpenBMC stuff. We sent a bunch of
the Nuvoton patches up for review recently, and we're actively adding more
devices, etc.
We also have quite a few patches downstream we're looking to upstream,
including PECI, and sensors, etc, etc that we've seen on BMC servers.
Patrick
>
> Thanks!
> Peter
>
> >
> >
> > Titus
>
>
Re: [PATCH 3/9] hw/arm/aspeed: qcom-dc-scm-v1: add block backed FRU device
On Thu, Jun 23, 2022 at 10:16 AM Cédric Le Goater wrote:
> On 6/23/22 17:28, Patrick Venture wrote:
> >
> >
> > On Wed, Jun 22, 2022 at 10:48 AM Jae Hyun Yoo <mailto:quic_jaeh...@quicinc.com>> wrote:
> >
> > From: Graeme Gregory quic_ggreg...@quicinc.com>>
> >
> > The FRU device uses the index 0 device on bus IF_NONE.
> >
> > -drive file=$FRU,format=raw,if=none
> >
> > file must match FRU size of 128k
> >
> > Signed-off-by: Graeme Gregory quic_ggreg...@quicinc.com>>
> > ---
> > hw/arm/aspeed.c | 22 +-
> > 1 file changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> > index 785cc543d046..36d6b2c33e48 100644
> > --- a/hw/arm/aspeed.c
> > +++ b/hw/arm/aspeed.c
> > @@ -992,17 +992,29 @@ static void fby35_i2c_init(AspeedMachineState
> *bmc)
> >*/
> > }
> >
> > +static void qcom_dc_scm_fru_init(I2CBus *bus, uint8_t addr,
> uint32_t rsize)
> > +{
> > +I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", addr);
> > +DeviceState *dev = DEVICE(i2c_dev);
> > +/* Use First Index for DC-SCM FRU */
> > +DriveInfo *dinfo = drive_get(IF_NONE, 0, 0);
> > +
> > +qdev_prop_set_uint32(dev, "rom-size", rsize);
> > +
> > +if (dinfo) {
> > +qdev_prop_set_drive(dev, "drive",
> blk_by_legacy_dinfo(dinfo));
> > +}
> > +
> > +i2c_slave_realize_and_unref(i2c_dev, bus, _abort);
> > +}
> >
> >
> > We've sent a similar patch up for the at24c but in its own file -- but
> looking at this, we could likely expand it to suite our cases as well - is
> there a reason it's named qcom_dc_scm_fru_init? Presumably that's to
> handle the drive_get parameters. If you make it use `drive_get(IF_NONE,
> bus, unit);` You'll be able to associate a drive via parameters like you
> aim to.
>
>
> I have seen various attempts to populate the Aspeed eeproms with
> data. The simplest is the g220a_bmc_i2c_init() approach. Some are
> generating C code from the .bin files and compiling the result in
> QEMU. Some were generating elf sections, linking them in QEMU and
> passing the pointer as an eeprom buf.
>
> The drive approach is the best I think, but the device/drive
> association depends on the drive order on the command line when
> devices are created by the platform.
>
> We could may be extend the GenericLoaderState for eeproms ?
> or introduce a routine which would look for a file under a (pc-bios)
> per-machine directory and fill the eeprom buf with its contents ?
>
> Any idea ?
>
So we have this in our eeprom_at24c module because we use it with Aspeed
and Nuvoton:
void at24c_eeprom_init_one(I2CBus *i2c_bus, int bus, uint8_t addr,
uint32_t rsize, int unit_number)
{
I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", addr);
DeviceState *dev = DEVICE(i2c_dev);
BlockInterfaceType type = IF_NONE;
DriveInfo *dinfo;
dinfo = drive_get(type, bus, unit_number);
if (dinfo) {
qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
}
qdev_prop_set_uint32(dev, "rom-size", rsize);
i2c_slave_realize_and_unref(i2c_dev, i2c_bus, _abort);
}
With this style call in the board:
snippet from downstream version of
https://github.com/qemu/qemu/blob/master/hw/arm/npcm7xx_boards.c#L305 that
we still need to finish upstreaming:
/* mb_fru */
at24c_eeprom_init_one(npcm7xx_i2c_get_bus(soc, 5), 5, 0x50, 8192, 0);
/* fan_fru */
at24c_eeprom_init_one(pca954x_i2c_get_bus(i2c_mux, 2), 5, 0x51, 8192,
1);
/* hsbp_fru */
at24c_eeprom_init_one(pca954x_i2c_get_bus(i2c_mux, 3), 5, 0x52, 8192,
2);
And then we call it with the bus and unit, the pair of those has to be
unique for the drive parameter to work:
-drive file=/export/hda3/tmp/mb_fru@50
,if=none,bus=5,unit=0,snapshot=off,format=raw
-drive file=/export/hda3/tmp/fan_fru@51
,if=none,bus=5,unit=1,snapshot=off,format=raw
-drive file=/export/hda3/tmp/hsbp_fru@52
,if=none,bus=5,unit=2,snapshot=off,format=raw
The above code snippet is what, I'm fairly certain we had sent up for
review before, and we can send it again if you want.
> Thanks,
>
> C.
>
Re: [PATCH 3/9] hw/arm/aspeed: qcom-dc-scm-v1: add block backed FRU device
On Wed, Jun 22, 2022 at 10:48 AM Jae Hyun Yoo
wrote:
> From: Graeme Gregory
>
> The FRU device uses the index 0 device on bus IF_NONE.
>
> -drive file=$FRU,format=raw,if=none
>
> file must match FRU size of 128k
>
> Signed-off-by: Graeme Gregory
> ---
> hw/arm/aspeed.c | 22 +-
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 785cc543d046..36d6b2c33e48 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -992,17 +992,29 @@ static void fby35_i2c_init(AspeedMachineState *bmc)
> */
> }
>
> +static void qcom_dc_scm_fru_init(I2CBus *bus, uint8_t addr, uint32_t
> rsize)
> +{
> +I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", addr);
> +DeviceState *dev = DEVICE(i2c_dev);
> +/* Use First Index for DC-SCM FRU */
> +DriveInfo *dinfo = drive_get(IF_NONE, 0, 0);
> +
> +qdev_prop_set_uint32(dev, "rom-size", rsize);
> +
> +if (dinfo) {
> +qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
> +}
> +
> +i2c_slave_realize_and_unref(i2c_dev, bus, _abort);
> +}
>
We've sent a similar patch up for the at24c but in its own file -- but
looking at this, we could likely expand it to suite our cases as well - is
there a reason it's named qcom_dc_scm_fru_init? Presumably that's to
handle the drive_get parameters. If you make it use `drive_get(IF_NONE,
bus, unit);` You'll be able to associate a drive via parameters like you
aim to.
> +
> static void qcom_dc_scm_bmc_i2c_init(AspeedMachineState *bmc)
> {
> AspeedSoCState *soc = >soc;
>
> i2c_slave_create_simple(aspeed_i2c_get_bus(>i2c, 15), "tmp105",
> 0x4d);
>
> -uint8_t *eeprom_buf = g_malloc0(128 * 1024);
> -if (eeprom_buf) {
> -smbus_eeprom_init_one(aspeed_i2c_get_bus(>i2c, 15), 0x53,
> - eeprom_buf);
> -}
> +qcom_dc_scm_fru_init(aspeed_i2c_get_bus(>i2c, 15), 0x53, 128 *
> 1024);
> }
>
> static bool aspeed_get_mmio_exec(Object *obj, Error **errp)
> --
> 2.25.1
>
>
>
Re: misaligned-pointer-use libslirp/src/tcp_input.c
On Tue, Jun 21, 2022 at 10:17 AM Peter Foley wrote: > The upstream fixes in > https://gitlab.freedesktop.org/slirp/libslirp/-/commit/6489ebbc691f5d97221ad154d570a231e30fb369 > and > https://gitlab.freedesktop.org/slirp/libslirp/-/commit/cc20d9ac578aec5502dcb26557765d3e9433cb26 > resolved the failure we were seeing in our internal test-case. > Thanks! > Thanks for posting the resolution commits! > > On Tue, Jun 21, 2022 at 12:47 PM Patrick Venture > wrote: > >> >> >> On Fri, Jun 17, 2022 at 7:37 AM Alexander Bulekov wrote: >> >>> On 220617 1217, Thomas Huth wrote: >>> > On 16/06/2022 21.03, Alexander Bulekov wrote: >>> > > On 220616 0930, Patrick Venture wrote: >>> > > > On Thu, Jun 16, 2022 at 6:31 AM Alexander Bulekov >>> wrote: >>> > > > >>> > > > > Is this an --enable-sanitizers build? The virtual-device fuzzer >>> catches >>> > > > > >>> > > > >>> > > > Yeah - it should be reproducible with a sanitizers build from HEAD >>> -- I can >>> > > > try to get a manual instance going again without automation to try >>> and >>> > > > reproduce it. We're testing on v7.0.0 which is when we started >>> seeing >>> > > > this, I don't think we saw it in 6.2.0. >>> > > >>> > > Here are a few reproducers (run with --enable-sanitizers): >>> > > >>> > > This one complains about misalignments in ip_header, ipasfrag, qlink, >>> > > ip... >>> > > >>> > > cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, >>> -m \ >>> > > 512M,slots=4,maxmem=0x -machine q35 -nodefaults >>> -device \ >>> > > vmxnet3,netdev=net0 -netdev user,id=net0 -object \ >>> > > memory-backend-ram,id=mem1,size=10M -device \ >>> > > pc-dimm,id=nv1,memdev=mem1,addr=0xba19ff -object \ >>> > > memory-backend-ram,id=mem2,size=10M -device \ >>> > > pc-dimm,id=nv2,memdev=mem2,addr=0xbe53e14abaa0 -object \ >>> > > memory-backend-ram,id=mem3,size=10M -device \ >>> > > pc-dimm,id=nv3,memdev=mem3,addr=0xfee9cae0 -object \ >>> > > memory-backend-ram,id=mem4,size=10M -device \ >>> > > pc-dimm,id=nv4,memdev=mem4,addr=0xf0f0f0f -qtest stdio >>> > > outl 0xcf8 0x8810 >>> > > outl 0xcfc 0xe000 >>> > > outl 0xcf8 0x8814 >>> > > outl 0xcfc 0xe0001000 >>> > > outl 0xcf8 0x8804 >>> > > outw 0xcfc 0x06 >>> > > write 0x3e 0x1 0x02 >>> > > write 0x39 0x1 0x20 >>> > > write 0x29 0x1 0x10 >>> > > write 0x2c 0x1 0x0f >>> > > write 0x2d 0x1 0x0f >>> > > write 0x2e 0x1 0x0f >>> > > write 0x2f 0x1 0x0f >>> > > write 0xf0f0f0f1012 0x1 0xfe >>> > > write 0xf0f0f0f1013 0x1 0xca >>> > > write 0xf0f0f0f1014 0x1 0xe9 >>> > > write 0xf0f0f0f1017 0x1 0xfe >>> > > write 0xf0f0f0f103a 0x1 0x01 >>> > > write 0xfee9cafe0009 0x1 0x40 >>> > > write 0xfee9cafe0019 0x1 0x40 >>> > > write 0x0 0x1 0xe1 >>> > > write 0x1 0x1 0xfe >>> > > write 0x2 0x1 0xbe >>> > > write 0x3 0x1 0xba >>> > > writel 0xe0001020 0xcafe >>> > > write 0xfee9cafe0029 0x1 0x40 >>> > > write 0xfee9cafe0039 0x1 0x40 >>> > > write 0xfee9cafe0049 0x1 0x40 >>> > > write 0xfee9cafe0059 0x1 0x40 >>> > > write 0x1f65190b 0x1 0x08 >>> > > write 0x1f65190d 0x1 0x46 >>> > > write 0x1f65190e 0x1 0x03 >>> > > write 0x1f651915 0x1 0x01 >>> > > write 0xfee9cafe0069 0x1 0x40 >>> > > write 0xfee9cafe0079 0x1 0x40 >>> > > write 0xfee9cafe0089 0x1 0x40 >>> > > write 0xfee9cafe0099 0x1 0x40 >>> > > write 0xfee9cafe009d 0x1 0x10 >>> > > write 0xfee9cafe00a0 0x1 0xff >>> > > write 0xfee9cafe00a1 0x1 0x18 >>> > > write 0xfee9cafe00a2 0x1 0x65 >>> > > write 0xfee9cafe00a3 0x1 0x1f >>> > > write 0xfee9cafe00a9 0x1 0x40 >>> > > write 0xfee9cafe00ad 0x1 0x1c >>> > > write 0xe602 0x1 0x00 >>> > > EOF >>> > >
Re: misaligned-pointer-use libslirp/src/tcp_input.c
On Fri, Jun 17, 2022 at 7:37 AM Alexander Bulekov wrote: > On 220617 1217, Thomas Huth wrote: > > On 16/06/2022 21.03, Alexander Bulekov wrote: > > > On 220616 0930, Patrick Venture wrote: > > > > On Thu, Jun 16, 2022 at 6:31 AM Alexander Bulekov > wrote: > > > > > > > > > Is this an --enable-sanitizers build? The virtual-device fuzzer > catches > > > > > > > > > > > > > Yeah - it should be reproducible with a sanitizers build from HEAD > -- I can > > > > try to get a manual instance going again without automation to try > and > > > > reproduce it. We're testing on v7.0.0 which is when we started > seeing > > > > this, I don't think we saw it in 6.2.0. > > > > > > Here are a few reproducers (run with --enable-sanitizers): > > > > > > This one complains about misalignments in ip_header, ipasfrag, qlink, > > > ip... > > > > > > cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m > \ > > > 512M,slots=4,maxmem=0x -machine q35 -nodefaults > -device \ > > > vmxnet3,netdev=net0 -netdev user,id=net0 -object \ > > > memory-backend-ram,id=mem1,size=10M -device \ > > > pc-dimm,id=nv1,memdev=mem1,addr=0xba19ff -object \ > > > memory-backend-ram,id=mem2,size=10M -device \ > > > pc-dimm,id=nv2,memdev=mem2,addr=0xbe53e14abaa0 -object \ > > > memory-backend-ram,id=mem3,size=10M -device \ > > > pc-dimm,id=nv3,memdev=mem3,addr=0xfee9cae0 -object \ > > > memory-backend-ram,id=mem4,size=10M -device \ > > > pc-dimm,id=nv4,memdev=mem4,addr=0xf0f0f0f -qtest stdio > > > outl 0xcf8 0x8810 > > > outl 0xcfc 0xe000 > > > outl 0xcf8 0x8814 > > > outl 0xcfc 0xe0001000 > > > outl 0xcf8 0x8804 > > > outw 0xcfc 0x06 > > > write 0x3e 0x1 0x02 > > > write 0x39 0x1 0x20 > > > write 0x29 0x1 0x10 > > > write 0x2c 0x1 0x0f > > > write 0x2d 0x1 0x0f > > > write 0x2e 0x1 0x0f > > > write 0x2f 0x1 0x0f > > > write 0xf0f0f0f1012 0x1 0xfe > > > write 0xf0f0f0f1013 0x1 0xca > > > write 0xf0f0f0f1014 0x1 0xe9 > > > write 0xf0f0f0f1017 0x1 0xfe > > > write 0xf0f0f0f103a 0x1 0x01 > > > write 0xfee9cafe0009 0x1 0x40 > > > write 0xfee9cafe0019 0x1 0x40 > > > write 0x0 0x1 0xe1 > > > write 0x1 0x1 0xfe > > > write 0x2 0x1 0xbe > > > write 0x3 0x1 0xba > > > writel 0xe0001020 0xcafe > > > write 0xfee9cafe0029 0x1 0x40 > > > write 0xfee9cafe0039 0x1 0x40 > > > write 0xfee9cafe0049 0x1 0x40 > > > write 0xfee9cafe0059 0x1 0x40 > > > write 0x1f65190b 0x1 0x08 > > > write 0x1f65190d 0x1 0x46 > > > write 0x1f65190e 0x1 0x03 > > > write 0x1f651915 0x1 0x01 > > > write 0xfee9cafe0069 0x1 0x40 > > > write 0xfee9cafe0079 0x1 0x40 > > > write 0xfee9cafe0089 0x1 0x40 > > > write 0xfee9cafe0099 0x1 0x40 > > > write 0xfee9cafe009d 0x1 0x10 > > > write 0xfee9cafe00a0 0x1 0xff > > > write 0xfee9cafe00a1 0x1 0x18 > > > write 0xfee9cafe00a2 0x1 0x65 > > > write 0xfee9cafe00a3 0x1 0x1f > > > write 0xfee9cafe00a9 0x1 0x40 > > > write 0xfee9cafe00ad 0x1 0x1c > > > write 0xe602 0x1 0x00 > > > EOF > > > > > > This one complains about misalignments in ip6_header, ip6_hdrctl... > > > > > > cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m > \ > > > 512M,slots=1,maxmem=0x -machine q35 -nodefaults > -device \ > > > vmxnet3,netdev=net0 -netdev user,id=net0 -object \ > > > memory-backend-ram,id=mem1,size=4M -device \ > > > pc-dimm,id=nv1,memdev=mem1,addr=0x1dd8600 -qtest stdio > > > outl 0xcf8 0x8810 > > > outl 0xcfc 0xe000 > > > outl 0xcf8 0x8814 > > > outl 0xcfc 0xe0001000 > > > outl 0xcf8 0x8804 > > > outw 0xcfc 0x06 > > > write 0x0 0x1 0xe1 > > > write 0x1 0x1 0xfe > > > write 0x2 0x1 0xbe > > > write 0x3 0x1 0xba > > > write 0x3e 0x1 0x01 > > > write 0x39 0x1 0x01 > > > write 0x28 0x1 0x01 > > > write 0x29 0x1 0x01 > > > write 0x2d 0x1 0x86 > > > write 0x2e 0x1 0xdd > > > write 0x2f 0x1 0x01 > > > write 0x1dd86000112 0x1 0x10 > > > write 0x1dd8600013c 0x1 0x02 > >
Re: misaligned-pointer-use libslirp/src/tcp_input.c
On Thu, Jun 16, 2022 at 6:31 AM Alexander Bulekov wrote: > Is this an --enable-sanitizers build? The virtual-device fuzzer catches > Yeah - it should be reproducible with a sanitizers build from HEAD -- I can try to get a manual instance going again without automation to try and reproduce it. We're testing on v7.0.0 which is when we started seeing this, I don't think we saw it in 6.2.0. > these periodically while fuzzing network-devices. However I don't think > OSS-Fuzz creates reports for them for some reason. I can create qtest > reproducers, if that is useful. > -Alex > > On 220615 0942, Patrick Venture wrote: > > Hey - I wanted to ask if someone else has seen this or has suggestions on > > how to fix it in libslirp / qemu. > > > > libslirp version: 3ad1710a96678fe79066b1469cead4058713a1d9 > > > > The blow is line: > > > https://gitlab.freedesktop.org/slirp/libslirp/-/blob/master/src/tcp_input.c#L310 > > > > I0614 13:44:44.3040872040 bytestream.cc:22] QEMU: > > third_party/libslirp/src/tcp_input.c:310:56: runtime error: member access > > within misaligned address 0x9a4000f4 for type 'struct qlink', which > > requires 8 byte alignment > > I0614 13:44:44.3041562040 bytestream.cc:22] QEMU: 0x9a4000f4: > note: > > pointer points here > > I0614 13:44:44.3041842040 bytestream.cc:22] QEMU: 00 00 00 00 00 00 > > 00 02 20 02 0a 00 00 01 42 01 0a 00 02 02 42 01 0a 00 00 01 86 dd 60 > 02 > > dd 79 > > I0614 13:44:44.3042042040 bytestream.cc:22] QEMU: ^ > > I0614 13:44:44.6411732040 bytestream.cc:22] QEMU: #0 > 0xcbe34bd8 > > in tcp_input third_party/libslirp/src/tcp_input.c:310:56 > > I0614 13:44:44.6412392040 bytestream.cc:22] QEMU: #1 > 0xcbe22a94 > > in ip6_input third_party/libslirp/src/ip6_input.c:74:9 > > I0614 13:44:44.6412622040 bytestream.cc:22] QEMU: #2 > 0xcbe0bbbc > > in slirp_input third_party/libslirp/src/slirp.c:1169:13 > > I0614 13:44:44.6412802040 bytestream.cc:22] QEMU: #3 > 0xcbd55f6c > > in net_slirp_receive third_party/qemu/net/slirp.c:136:5 > > I0614 13:44:44.6412962040 bytestream.cc:22] QEMU: #4 > 0xcbd4e77c > > in nc_sendv_compat third_party/qemu/net/net.c > > I0614 13:44:44.6413232040 bytestream.cc:22] QEMU: #5 > 0xcbd4e77c > > in qemu_deliver_packet_iov third_party/qemu/net/net.c:850:15 > > I0614 13:44:44.6413422040 bytestream.cc:22] QEMU: #6 > 0xcbd50bfc > > in qemu_net_queue_deliver_iov third_party/qemu/net/queue.c:179:11 > > I0614 13:44:44.6413592040 bytestream.cc:22] QEMU: #7 > 0xcbd50bfc > > in qemu_net_queue_send_iov third_party/qemu/net/queue.c:246:11 > > I0614 13:44:44.6413822040 bytestream.cc:22] QEMU: #8 > 0xcbd4a88c > > in qemu_sendv_packet_async third_party/qemu/net/net.c:891:12 > > I0614 13:44:44.6413962040 bytestream.cc:22] QEMU: #9 > 0xcacb1de0 > > in virtio_net_flush_tx third_party/qemu/hw/net/virtio-net.c:2586:15 > > I0614 13:44:44.6414162040 bytestream.cc:22] QEMU: #10 > > 0xcacb1580 in virtio_net_tx_bh > > third_party/qemu/hw/net/virtio-net.c:2703:11 > > I0614 13:44:44.6414382040 bytestream.cc:22] QEMU: #11 > > 0xcc2bcf64 in aio_bh_call third_party/qemu/util/async.c:142:5 > > I0614 13:44:44.6414632040 bytestream.cc:22] QEMU: #12 > > 0xcc2bcf64 in aio_bh_poll third_party/qemu/util/async.c:170:13 > > I0614 13:44:44.6414772040 bytestream.cc:22] QEMU: #13 > > 0xcc2b8f70 in aio_dispatch third_party/qemu/util/aio-posix.c:420:5 > > I0614 13:44:44.6414952040 bytestream.cc:22] QEMU: #14 > > 0xcc2bf120 in aio_ctx_dispatch third_party/qemu/util/async.c:312:5 > > I0614 13:44:44.6415102040 bytestream.cc:22] QEMU: #15 > > 0xcc3a7690 in g_main_dispatch third_party/glib/glib/gmain.c:3417:27 > > I0614 13:44:44.6415252040 bytestream.cc:22] QEMU: #16 > > 0xcc3a7690 in g_main_context_dispatch > > third_party/glib/glib/gmain.c:4135:7 > > I0614 13:44:44.6415462040 bytestream.cc:22] QEMU: #17 > > 0xcc2de3ec in glib_pollfds_poll > third_party/qemu/util/main-loop.c:232:9 > > I0614 13:44:44.6415622040 bytestream.cc:22] QEMU: #18 > > 0xcc2de3ec in os_host_main_loop_wait > > third_party/qemu/util/main-loop.c:255:5 > > I0614 13:44:44.6415802040 bytestream.cc:22] QEMU: #19 > > 0xcc2de3ec in main_loop_wait third_party/qemu/util/main-loop.c:531:11 > > I0614 13:44:44.6415982040 bytestream.cc:22] QEMU: #20 > > 0xcbd82798 in qemu_main_loop > third_party/qemu/softmmu/runstate.c:727:9 > > I0614 13:44:44.6416122040 bytestream.cc:22] QEMU: #21 > > 0xcadacb5c in main > > > > Patrick >
misaligned-pointer-use libslirp/src/tcp_input.c
Hey - I wanted to ask if someone else has seen this or has suggestions on how to fix it in libslirp / qemu. libslirp version: 3ad1710a96678fe79066b1469cead4058713a1d9 The blow is line: https://gitlab.freedesktop.org/slirp/libslirp/-/blob/master/src/tcp_input.c#L310 I0614 13:44:44.3040872040 bytestream.cc:22] QEMU: third_party/libslirp/src/tcp_input.c:310:56: runtime error: member access within misaligned address 0x9a4000f4 for type 'struct qlink', which requires 8 byte alignment I0614 13:44:44.3041562040 bytestream.cc:22] QEMU: 0x9a4000f4: note: pointer points here I0614 13:44:44.3041842040 bytestream.cc:22] QEMU: 00 00 00 00 00 00 00 02 20 02 0a 00 00 01 42 01 0a 00 02 02 42 01 0a 00 00 01 86 dd 60 02 dd 79 I0614 13:44:44.3042042040 bytestream.cc:22] QEMU: ^ I0614 13:44:44.6411732040 bytestream.cc:22] QEMU: #0 0xcbe34bd8 in tcp_input third_party/libslirp/src/tcp_input.c:310:56 I0614 13:44:44.6412392040 bytestream.cc:22] QEMU: #1 0xcbe22a94 in ip6_input third_party/libslirp/src/ip6_input.c:74:9 I0614 13:44:44.6412622040 bytestream.cc:22] QEMU: #2 0xcbe0bbbc in slirp_input third_party/libslirp/src/slirp.c:1169:13 I0614 13:44:44.6412802040 bytestream.cc:22] QEMU: #3 0xcbd55f6c in net_slirp_receive third_party/qemu/net/slirp.c:136:5 I0614 13:44:44.6412962040 bytestream.cc:22] QEMU: #4 0xcbd4e77c in nc_sendv_compat third_party/qemu/net/net.c I0614 13:44:44.6413232040 bytestream.cc:22] QEMU: #5 0xcbd4e77c in qemu_deliver_packet_iov third_party/qemu/net/net.c:850:15 I0614 13:44:44.6413422040 bytestream.cc:22] QEMU: #6 0xcbd50bfc in qemu_net_queue_deliver_iov third_party/qemu/net/queue.c:179:11 I0614 13:44:44.6413592040 bytestream.cc:22] QEMU: #7 0xcbd50bfc in qemu_net_queue_send_iov third_party/qemu/net/queue.c:246:11 I0614 13:44:44.6413822040 bytestream.cc:22] QEMU: #8 0xcbd4a88c in qemu_sendv_packet_async third_party/qemu/net/net.c:891:12 I0614 13:44:44.6413962040 bytestream.cc:22] QEMU: #9 0xcacb1de0 in virtio_net_flush_tx third_party/qemu/hw/net/virtio-net.c:2586:15 I0614 13:44:44.6414162040 bytestream.cc:22] QEMU: #10 0xcacb1580 in virtio_net_tx_bh third_party/qemu/hw/net/virtio-net.c:2703:11 I0614 13:44:44.6414382040 bytestream.cc:22] QEMU: #11 0xcc2bcf64 in aio_bh_call third_party/qemu/util/async.c:142:5 I0614 13:44:44.6414632040 bytestream.cc:22] QEMU: #12 0xcc2bcf64 in aio_bh_poll third_party/qemu/util/async.c:170:13 I0614 13:44:44.6414772040 bytestream.cc:22] QEMU: #13 0xcc2b8f70 in aio_dispatch third_party/qemu/util/aio-posix.c:420:5 I0614 13:44:44.6414952040 bytestream.cc:22] QEMU: #14 0xcc2bf120 in aio_ctx_dispatch third_party/qemu/util/async.c:312:5 I0614 13:44:44.6415102040 bytestream.cc:22] QEMU: #15 0xcc3a7690 in g_main_dispatch third_party/glib/glib/gmain.c:3417:27 I0614 13:44:44.6415252040 bytestream.cc:22] QEMU: #16 0xcc3a7690 in g_main_context_dispatch third_party/glib/glib/gmain.c:4135:7 I0614 13:44:44.6415462040 bytestream.cc:22] QEMU: #17 0xcc2de3ec in glib_pollfds_poll third_party/qemu/util/main-loop.c:232:9 I0614 13:44:44.6415622040 bytestream.cc:22] QEMU: #18 0xcc2de3ec in os_host_main_loop_wait third_party/qemu/util/main-loop.c:255:5 I0614 13:44:44.6415802040 bytestream.cc:22] QEMU: #19 0xcc2de3ec in main_loop_wait third_party/qemu/util/main-loop.c:531:11 I0614 13:44:44.6415982040 bytestream.cc:22] QEMU: #20 0xcbd82798 in qemu_main_loop third_party/qemu/softmmu/runstate.c:727:9 I0614 13:44:44.6416122040 bytestream.cc:22] QEMU: #21 0xcadacb5c in main Patrick
Re: [PATCH v5] tests/qtest: add qtests for npcm7xx sdhci
On Thu, May 26, 2022 at 8:54 AM Peter Maydell wrote: > On Fri, 25 Feb 2022 at 17:45, Hao Wu wrote: > > > > From: Shengtan Mao > > > > Reviewed-by: Hao Wu > > Reviewed-by: Chris Rauer > > Signed-off-by: Shengtan Mao > > Signed-off-by: Patrick Venture > > Hi; John Snow tells me that this test fails in the tests/vm/netbsd > VM (you can test this with 'make vm-build-netbsd') because the > assert() on the ftruncate() call fails: > > > +ret = ftruncate(fd, NPCM7XX_TEST_IMAGE_SIZE); > > +g_assert_cmpint(ret, ==, 0); > > > +#define NPCM7XX_TEST_IMAGE_SIZE (1 << 30) > > I haven't investigated the exact cause, but this is a > gigabyte, right? That's a pretty massive file for a test case to > create -- can we make the test use a more sensible size of > sd card image ? > It looks like the nuvoton part had an issue with a smaller image size, but we can resurrect that thread and poke at it a bit and see what shakes out. > > thanks > -- PMM >
Npcm7xx emac tap networking hitting 75MiB limit
Hey; I wanted to ask if anyone has seen this before. When we try to download a file larger than 75MB from within an Ncpm7xx board using TAP networking (versus user) it just fails there. using wget it reports null 75.0M - stalled -\rnull 75.0M - stalled -\rnull 75.0M - stalled -\rnull 75.0M - stalled -\rnull 75.0M - stalled -\rnull 75.0M - I haven't yet dug into what could be going wrong here, but I wanted to ask if any of y'all have seen some weird stuff here? Thanks, Patrick
Re: [PATCH 1/2] hw/misc: Add Nuvoton's PCI Mailbox Module
On Thu, Jan 27, 2022 at 1:27 PM Patrick Venture wrote:
>
>
> On Thu, Jan 27, 2022 at 10:37 AM Peter Maydell
> wrote:
>
>> On Mon, 10 Jan 2022 at 17:56, Patrick Venture wrote:
>> >
>> > From: Hao Wu
>> >
>> > The PCI Mailbox Module is a high-bandwidth communcation module
>> > between a Nuvoton BMC and CPU. It features 16KB RAM that are both
>> > accessible by the BMC and core CPU. and supports interrupt for
>> > both sides.
>> >
>> > This patch implements the BMC side of the PCI mailbox module.
>> > Communication with the core CPU is emulated via a chardev and
>> > will be in a follow-up patch.
>> >
>> > Reviewed-by: Patrick Venture
>> > Reviewed-by: Joe Komlodi
>>
>> Hi; this mostly looks good, but I have a question about s->content.
>>
>> > +static void npcm7xx_pci_mbox_init(Object *obj)
>> > +{
>> > +NPCM7xxPCIMBoxState *s = NPCM7XX_PCI_MBOX(obj);
>> > +SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> > +
>> > +memory_region_init_ram_device_ptr(>ram, obj, "pci-mbox-ram",
>> > + NPCM7XX_PCI_MBOX_RAM_SIZE,
>> s->content);
>>
>> What's s->content for? Nothing in the rest of the device emulation
>> touches it, which seems odd.
>>
>
> Hao informed me that we can drop the content bit here, since it's not used
> until a later patch that we'll be sending up with a bit more detail when we
> get a chance. I sent this series up to land some of the groundwork.
>
> I can send out a v2 with that bit removed.
>
>
>>
>> memory_region_init_ram_device_ptr() is intended primarily
>> for "create a MemoryRegion corresponding to something like
>> a bit of a host device (eg a host PCI MMIO BAR). That doesn't
>> seem like what you're doing here. In particular, using it
>> means that you take on responsibility for ensuring that the
>> underlying memory gets migrated, which you're not doing.
>>
>> If you just want a MemoryRegion that's backed by a bit of
>> host memory, use memory_region_init_ram().
>>
>
> I think this is what we use it for in the future patches, when we add it
> back, it'll come with the context.
>
>
>>
>> > +#define TYPE_NPCM7XX_PCI_MBOX "npcm7xx-pci-mbox"
>> > +#define NPCM7XX_PCI_MBOX(obj) \
>> > +OBJECT_CHECK(NPCM7xxPCIMBoxState, (obj), TYPE_NPCM7XX_PCI_MBOX)
>>
>> We prefer the OBJECT_DECLARE_SIMPLE_TYPE() macro rather than
>> hand-defining a cast macro these days.
>>
>
> Ack.
>
>
>>
>> thanks
>> -- PMM
>>
>
Peter, just an FYI, this fell off my radar, but I will be circling back
this week to revisit any patches I've sent that are in limbo or waiting on
me, etc. Thanks for your patience.
>
> Thanks!
>
Re: [PATCH v1 8/9] aspeed: Add an AST1030 eval board
On Mon, Mar 21, 2022 at 7:59 PM Jamin Lin wrote:
> From: Steven Lee
>
> The image should be supplied with ELF binary.
> $ qemu-system-arm -M ast1030-evb -kernel zephyr.elf -nographic
>
> Signed-off-by: Troy Lee
> Signed-off-by: Jamin Lin
> Signed-off-by: Steven Lee
>
Reviewed-by: Patrick Venture
> ---
> hw/arm/aspeed.c | 2 +-
> hw/arm/aspeed_minibmc.c | 129
> hw/arm/meson.build | 3 +-
> include/hw/arm/aspeed.h | 25
> 4 files changed, 157 insertions(+), 2 deletions(-)
> create mode 100644 hw/arm/aspeed_minibmc.c
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index d205384d98..e5a2e59aef 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -276,7 +276,7 @@ static void write_boot_rom(DriveInfo *dinfo, hwaddr
> addr, size_t rom_size,
> rom_add_blob_fixed("aspeed.boot_rom", storage, rom_size, addr);
> }
>
> -static void aspeed_board_init_flashes(AspeedSMCState *s, const char
> *flashtype,
> +void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
>unsigned int count, int unit0)
> {
> int i;
> diff --git a/hw/arm/aspeed_minibmc.c b/hw/arm/aspeed_minibmc.c
> new file mode 100644
> index 00..6a29475919
> --- /dev/null
> +++ b/hw/arm/aspeed_minibmc.c
> @@ -0,0 +1,129 @@
> +/*
> + * ASPEED AST1030 SoC
> + *
> + * Copyright (C) 2022 ASPEED Technology Inc.
> + *
> + * This code is licensed under the GPL version 2 or later. See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "hw/boards.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/qdev-clock.h"
> +#include "qemu/error-report.h"
> +#include "hw/arm/aspeed.h"
> +#include "hw/arm/aspeed_soc.h"
> +#include "hw/arm/boot.h"
> +#include "hw/i2c/smbus_eeprom.h"
> +#include "hw/sensor/tmp105.h"
> +
> +#define AST1030_INTERNAL_FLASH_SIZE (1024 * 1024)
> +
> +struct AspeedMiniBmcMachineState {
> +/* Private */
> +MachineState parent_obj;
> +/* Public */
> +
> +AspeedSoCState soc;
> +MemoryRegion ram_container;
> +MemoryRegion max_ram;
> +bool mmio_exec;
> +char *fmc_model;
> +char *spi_model;
> +};
> +
> +/* Main SYSCLK frequency in Hz (200MHz) */
> +#define SYSCLK_FRQ 2ULL
> +
> +static void aspeed_minibmc_machine_ast1030_evb_class_init(ObjectClass *oc,
> + void *data)
> +{
> +MachineClass *mc = MACHINE_CLASS(oc);
> +AspeedMiniBmcMachineClass *amc = ASPEED_MINIBMC_MACHINE_CLASS(oc);
> +
> +mc->desc = "Aspeed AST1030 MiniBMC (Cortex-M4)";
> +amc->soc_name = "ast1030-a1";
> +amc->hw_strap1 = 0;
> +amc->hw_strap2 = 0;
> +mc->default_ram_size = 0;
> +mc->default_cpus = mc->min_cpus = mc->max_cpus = 1;
> +amc->fmc_model = "sst25vf032b";
> +amc->spi_model = "sst25vf032b";
> +amc->num_cs = 2;
> +}
> +
> +static void ast1030_machine_instance_init(Object *obj)
> +{
> +ASPEED_MINIBMC_MACHINE(obj)->mmio_exec = false;
> +}
> +
> +static void aspeed_minibmc_machine_init(MachineState *machine)
> +{
> +AspeedMiniBmcMachineState *bmc = ASPEED_MINIBMC_MACHINE(machine);
> +AspeedMiniBmcMachineClass *amc =
> ASPEED_MINIBMC_MACHINE_GET_CLASS(machine);
> +Clock *sysclk;
> +
> +sysclk = clock_new(OBJECT(machine), "SYSCLK");
> +clock_set_hz(sysclk, SYSCLK_FRQ);
> +
> +object_initialize_child(OBJECT(machine), "soc", >soc,
> amc->soc_name);
> +qdev_connect_clock_in(DEVICE(>soc), "sysclk", sysclk);
> +
> +qdev_prop_set_uint32(DEVICE(>soc), "uart-default",
> + amc->uart_default);
> +qdev_realize(DEVICE(>soc), NULL, _abort);
> +
> +aspeed_board_init_flashes(>soc.fmc,
> + bmc->fmc_model ? bmc->fmc_model :
> amc->fmc_model,
> + amc->num_cs,
> + 0);
> +
> +aspeed_board_init_flashes(>soc.spi[0],
> + bmc->spi_model ? bmc->spi_model :
> amc->spi_model,
> + amc->num_cs, amc->num_cs);
> +
> +aspeed_board_init_flashes(>soc.spi[1],
> + bmc->spi_model ? bmc->spi_model :
> amc->spi_model,
>
[PATCH] hw/i2c: Enable an id for the pca954x devices
This allows the devices to be more readily found and specified.
Without setting the id field, they can only be found by device type
name, which doesn't let you specify the second of the same device type
behind a bus.
Tested: Verified that by default the device was findable with the id
'pca954x[77]', for an instance attached at that address.
Signed-off-by: Patrick Venture
Reviewed-by: Hao Wu
---
hw/i2c/i2c_mux_pca954x.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
index a9517b612a..4f8c2d6ae1 100644
--- a/hw/i2c/i2c_mux_pca954x.c
+++ b/hw/i2c/i2c_mux_pca954x.c
@@ -20,6 +20,7 @@
#include "hw/i2c/i2c_mux_pca954x.h"
#include "hw/i2c/smbus_slave.h"
#include "hw/qdev-core.h"
+#include "hw/qdev-properties.h"
#include "hw/sysbus.h"
#include "qemu/log.h"
#include "qemu/module.h"
@@ -43,6 +44,8 @@ typedef struct Pca954xState {
bool enabled[PCA9548_CHANNEL_COUNT];
I2CBus *bus[PCA9548_CHANNEL_COUNT];
+
+char *id;
} Pca954xState;
/*
@@ -181,6 +184,17 @@ static void pca9548_class_init(ObjectClass *klass, void
*data)
s->nchans = PCA9548_CHANNEL_COUNT;
}
+static void pca954x_realize(DeviceState *dev, Error **errp)
+{
+Pca954xState *s = PCA954X(dev);
+DeviceState *d = DEVICE(s);
+if (s->id) {
+d->id = g_strdup(s->id);
+} else {
+d->id = g_strdup_printf("pca954x[%x]", s->parent.i2c.address);
+}
+}
+
static void pca954x_init(Object *obj)
{
Pca954xState *s = PCA954X(obj);
@@ -197,6 +211,11 @@ static void pca954x_init(Object *obj)
}
}
+static Property pca954x_props[] = {
+DEFINE_PROP_STRING("id", Pca954xState, id),
+DEFINE_PROP_END_OF_LIST()
+};
+
static void pca954x_class_init(ObjectClass *klass, void *data)
{
I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
@@ -209,9 +228,12 @@ static void pca954x_class_init(ObjectClass *klass, void
*data)
rc->phases.enter = pca954x_enter_reset;
dc->desc = "Pca954x i2c-mux";
+dc->realize = pca954x_realize;
k->write_data = pca954x_write_data;
k->receive_byte = pca954x_read_byte;
+
+device_class_set_props(dc, pca954x_props);
}
static const TypeInfo pca954x_info[] = {
--
2.35.1.894.gb6a874cedc-goog
Lost patch, hw/sensor: enable adm1272 temp with qmp
I'm going through my team's spreadsheet for our upstreaming efforts to try to make sure things weren't lost, as sometimes a reply will go into my spam folder, and I saw this patch also fell under the radar: https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg01310.html If you want, I can also do a patch resend. Thanks, Patrick
[PATCH] hw/nvram: Add at24c-eeprom support for small eeproms
Tested: Verified at24c02 driver happy and contents matched
expectations.
Signed-off-by: Patrick Venture
Reviewed-by: Hao Wu
---
hw/nvram/eeprom_at24c.c | 52 +++--
1 file changed, 35 insertions(+), 17 deletions(-)
diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 01a3093600..abf9898f94 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -45,6 +45,8 @@ struct EEPROMState {
bool changed;
/* during WRITE, # of address bytes transfered */
uint8_t haveaddr;
+/* whether it's 8-bit addressed or 16-bit */
+bool eightbit;
uint8_t *mem;
@@ -85,7 +87,7 @@ uint8_t at24c_eeprom_recv(I2CSlave *s)
EEPROMState *ee = AT24C_EE(s);
uint8_t ret;
-if (ee->haveaddr == 1) {
+if (ee->haveaddr == 1 && !ee->eightbit) {
return 0xff;
}
@@ -102,26 +104,35 @@ int at24c_eeprom_send(I2CSlave *s, uint8_t data)
{
EEPROMState *ee = AT24C_EE(s);
-if (ee->haveaddr < 2) {
-ee->cur <<= 8;
-ee->cur |= data;
+if (ee->haveaddr < 1) {
+ee->cur = data;
ee->haveaddr++;
-if (ee->haveaddr == 2) {
-ee->cur %= ee->rsize;
+if (ee->eightbit) {
DPRINTK("Set pointer %04x\n", ee->cur);
}
+return 0;
+} else if (ee->haveaddr < 2) {
+if (!ee->eightbit) {
+ee->cur <<= 8;
+ee->cur |= data;
+ee->haveaddr++;
+if (ee->haveaddr == 2) {
+ee->cur %= ee->rsize;
+DPRINTK("Set pointer %04x\n", ee->cur);
+}
-} else {
-if (ee->writable) {
-DPRINTK("Send %02x\n", data);
-ee->mem[ee->cur] = data;
-ee->changed = true;
-} else {
-DPRINTK("Send error %02x read-only\n", data);
+return 0;
}
-ee->cur = (ee->cur + 1u) % ee->rsize;
+}
+if (ee->writable) {
+DPRINTK("Send %02x\n", data);
+ee->mem[ee->cur] = data;
+ee->changed = true;
+} else {
+DPRINTK("Send error %02x read-only\n", data);
}
+ee->cur = (ee->cur + 1u) % ee->rsize;
return 0;
}
@@ -131,12 +142,16 @@ static void at24c_eeprom_realize(DeviceState *dev, Error
**errp)
EEPROMState *ee = AT24C_EE(dev);
if (ee->blk) {
+/* blk length is a minimum of 1 sector. */
int64_t len = blk_getlength(ee->blk);
if (len != ee->rsize) {
-error_setg(errp, "%s: Backing file size %" PRId64 " != %u",
- TYPE_AT24C_EE, len, ee->rsize);
-return;
+/* for the at24c01 and at24c02, they are smaller than a sector. */
+if (ee->rsize >= BDRV_SECTOR_SIZE) {
+error_setg(errp, "%s: Backing file size %" PRId64 " != %u",
+ TYPE_AT24C_EE, len, ee->rsize);
+return;
+}
}
if (blk_set_perm(ee->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
@@ -148,6 +163,9 @@ static void at24c_eeprom_realize(DeviceState *dev, Error
**errp)
}
}
+if (ee->rsize < 512) {
+ee->eightbit = true;
+}
ee->mem = g_malloc0(ee->rsize);
}
--
2.35.1.723.g4982287a31-goog
Re: [PATCH] hw/nvram: Add at24c-eeprom support for small eeproms
On Mon, Mar 14, 2022 at 2:12 PM Patrick Venture wrote:
> Tested: Verified at24c02 driver happy and contents matched
> expectations.
>
> Signed-off-by: Patrick Venture
> Reviewed-by: Hao Wu
> ---
> hw/nvram/eeprom_at24c.c | 52 +++--
> 1 file changed, 35 insertions(+), 17 deletions(-)
>
> diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
> index 01a3093600..abf9898f94 100644
> --- a/hw/nvram/eeprom_at24c.c
> +++ b/hw/nvram/eeprom_at24c.c
> @@ -45,6 +45,8 @@ struct EEPROMState {
> bool changed;
> /* during WRITE, # of address bytes transfered */
> uint8_t haveaddr;
> +/* whether it's 8-bit addressed or 16-bit */
> +bool eightbit;
>
> uint8_t *mem;
>
> @@ -85,7 +87,7 @@ uint8_t at24c_eeprom_recv(I2CSlave *s)
> EEPROMState *ee = AT24C_EE(s);
> uint8_t ret;
>
> -if (ee->haveaddr == 1) {
> +if (ee->haveaddr == 1 && !ee->eightbit) {
> return 0xff;
> }
>
> @@ -102,26 +104,35 @@ int at24c_eeprom_send(I2CSlave *s, uint8_t data)
> {
> EEPROMState *ee = AT24C_EE(s);
>
> -if (ee->haveaddr < 2) {
> -ee->cur <<= 8;
> -ee->cur |= data;
> +if (ee->haveaddr < 1) {
> +ee->cur = data;
> ee->haveaddr++;
> -if (ee->haveaddr == 2) {
> -ee->cur %= ee->rsize;
> +if (ee->eightbit) {
> DPRINTK("Set pointer %04x\n", ee->cur);
> }
> +return 0;
> +} else if (ee->haveaddr < 2) {
> +if (!ee->eightbit) {
> +ee->cur <<= 8;
> +ee->cur |= data;
> +ee->haveaddr++;
> +if (ee->haveaddr == 2) {
> +ee->cur %= ee->rsize;
> +DPRINTK("Set pointer %04x\n", ee->cur);
> +}
>
> -} else {
> -if (ee->writable) {
> -DPRINTK("Send %02x\n", data);
> -ee->mem[ee->cur] = data;
> -ee->changed = true;
> -} else {
> -DPRINTK("Send error %02x read-only\n", data);
> +return 0;
> }
> -ee->cur = (ee->cur + 1u) % ee->rsize;
> +}
>
> +if (ee->writable) {
> +DPRINTK("Send %02x\n", data);
> +ee->mem[ee->cur] = data;
> +ee->changed = true;
> +} else {
> +DPRINTK("Send error %02x read-only\n", data);
> }
> +ee->cur = (ee->cur + 1u) % ee->rsize;
>
> return 0;
> }
> @@ -131,12 +142,16 @@ static void at24c_eeprom_realize(DeviceState *dev,
> Error **errp)
> EEPROMState *ee = AT24C_EE(dev);
>
> if (ee->blk) {
> +/* blk length is a minimum of 1 sector. */
> int64_t len = blk_getlength(ee->blk);
>
> if (len != ee->rsize) {
> -error_setg(errp, "%s: Backing file size %" PRId64 " != %u",
> - TYPE_AT24C_EE, len, ee->rsize);
> -return;
> +/* for the at24c01 and at24c02, they are smaller than a
> sector. */
> +if (ee->rsize >= BDRV_SECTOR_SIZE) {
> +error_setg(errp, "%s: Backing file size %" PRId64 " !=
> %u",
> + TYPE_AT24C_EE, len, ee->rsize);
> +return;
> +}
> }
>
> if (blk_set_perm(ee->blk, BLK_PERM_CONSISTENT_READ |
> BLK_PERM_WRITE,
> @@ -148,6 +163,9 @@ static void at24c_eeprom_realize(DeviceState *dev,
> Error **errp)
> }
> }
>
> +if (ee->rsize < 512) {
> +ee->eightbit = true;
> +}
>
Why 512? The eeprom sizes for the parts seem to grow exponentially, so c01
is 128, then c02 is 256, then 512. So I check for the two smaller
effectively. Might warrant a comment.
> ee->mem = g_malloc0(ee->rsize);
> }
>
> --
> 2.35.1.723.g4982287a31-goog
>
>
[PATCH] linux-user: Fix missing space in error message
From: Fergus Henderson
Signed-off-by: Fergus Henderson
Signed-off-by: Patrick Venture
---
linux-user/elfload.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 9628a38361..c45da4d633 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2504,7 +2504,7 @@ static void pgb_reserved_va(const char *image_name,
abi_ulong guest_loaddr,
addr = mmap(test, reserved_va, PROT_NONE, flags, -1, 0);
if (addr == MAP_FAILED || addr != test) {
error_report("Unable to reserve 0x%lx bytes of virtual address "
- "space at %p (%s) for use as guest address space (check
your"
+ "space at %p (%s) for use as guest address space (check
your "
"virtual memory ulimit setting, min_mmap_addr or reserve
less "
"using -R option)", reserved_va, test, strerror(errno));
exit(EXIT_FAILURE);
--
2.35.1.723.g4982287a31-goog
Re: Lost patch
On Wed, Mar 9, 2022 at 4:16 PM Philippe Mathieu-Daudé < philippe.mathieu.da...@gmail.com> wrote: > On 10/3/22 00:12, Patrick Venture wrote: > > Corey and Peter; > > > > I was about to submit a fix to the at24c-eeprom device and noticed that > > my v2 patch appears to have been lost to time. Is there any way we can > > get this pulled into 7.0? > > > > https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg03485.html > > Thanks for noticing. > > It is a bugfix so it is still good to go. I'm queuing it (except if > Peter beats me via qemu-arm) and will send a pullreq for it on next > Monday. > > Regards, > Thanks! > > Phil. >
Lost patch
Corey and Peter; I was about to submit a fix to the at24c-eeprom device and noticed that my v2 patch appears to have been lost to time. Is there any way we can get this pulled into 7.0? https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg03485.html Thanks, Patrick
Re: [PATCH v4] tests/qtest: add qtests for npcm7xx sdhci
On Fri, Feb 25, 2022 at 9:45 AM Hao Wu wrote:
> I have sent an updated version that uses memcmp()
>
> On Fri, Feb 25, 2022 at 3:44 AM Peter Maydell
> wrote:
>
>> On Thu, 24 Feb 2022 at 19:03, Hao Wu wrote:
>> >
>> > From: Shengtan Mao
>> >
>> > Reviewed-by: Hao Wu
>> > Reviewed-by: Chris Rauer
>> > Signed-off-by: Shengtan Mao
>> > Signed-off-by: Patrick Venture
>> > Signed-off-by: Hao Wu
>> > ---
>> > v4:
>> > * use strncmp to compare fixed length strings
>> > v3:
>> > * fixup compilation from missing macro value
>> > v2:
>> > * update copyright year
>> > * check result of open
>> > * use g_free instead of free
>> > * move declarations to the top
>> > * use g_file_open_tmp
>> > ---
>>
>> > +static void write_sdread(QTestState *qts, const char *msg)
>> > +{
>> > +int fd, ret;
>> > +size_t len = strlen(msg);
>> > +char *rmsg = g_malloc(len);
>> > +
>> > +/* write message to sd */
>> > +fd = open(sd_path, O_WRONLY);
>> > +g_assert(fd >= 0);
>> > +ret = write(fd, msg, len);
>> > +close(fd);
>> > +g_assert(ret == len);
>> > +
>> > +/* read message using sdhci */
>> > +ret = sdhci_read_cmd(qts, NPCM7XX_MMC_BA, rmsg, len);
>> > +g_assert(ret == len);
>> > +g_assert(!strncmp(rmsg, msg, len));
>>
>> We always know we want to compare exactly 'len' bytes here, and we know
>> the buffers in each case are at least that large. The right function
>> for that is memcmp(), I think.
>>
>
Thanks Hao for picking this up :)
>
>> thanks
>> -- PMM
>>
>
Re: [PATCH 0/5] Fixups for PMBus and new sensors
On Thu, Jan 6, 2022 at 3:09 PM Titus Rwantare wrote: > This patch series contains updates to PMBus in QEMU along with some PMBus > device models for Renesas regulators. > I have also added myself to MAINTAINERS as this code is in use daily, > where I am responsible for it. > > Shengtan Mao (1): > hw/i2c: Added linear mode translation for pmbus devices > > Titus Rwantare (4): > hw/i2c: pmbus updates > hw/sensor: add Intersil ISL69260 device model > hw/sensor: add Renesas raa229004 PMBus device > hw/misc: add Renesas raa228000 device > > MAINTAINERS | 15 +- > hw/arm/Kconfig| 1 + > hw/i2c/pmbus_device.c | 106 +++- > hw/sensor/Kconfig | 5 + > hw/sensor/isl_pmbus.c | 278 > hw/sensor/meson.build | 1 + > include/hw/i2c/pmbus_device.h | 23 +- > include/hw/sensor/isl_pmbus.h | 52 > tests/qtest/isl_pmbus-test.c | 460 ++ > tests/qtest/meson.build | 1 + > 10 files changed, 930 insertions(+), 12 deletions(-) > create mode 100644 hw/sensor/isl_pmbus.c > create mode 100644 include/hw/sensor/isl_pmbus.h > create mode 100644 tests/qtest/isl_pmbus-test.c > > Friendly ping - I believe I saw some of these have picked up Reviewer tags, but ideally this will get into 7.0 before next month's soft-freeze. > -- > 2.34.1.448.ga2b2bfdf31-goog > >
Re: [PATCH v2] hw/i2c: flatten pca954x mux device
On Thu, Feb 24, 2022 at 2:56 AM Peter Maydell wrote: > On Wed, 2 Feb 2022 at 17:57, Patrick Venture wrote: > > > > Previously this device created N subdevices which each owned an i2c bus. > > Now this device simply owns the N i2c busses directly. > > > > Tested: Verified devices behind mux are still accessible via qmp and i2c > > from within an arm32 SoC. > > > > Reviewed-by: Hao Wu > > Signed-off-by: Patrick Venture > > Reviewed-by: Philippe Mathieu-Daudé > > Tested-by: Philippe Mathieu-Daudé > > --- > > v2: explicitly create an incrementing name for the i2c busses (channels). > > --- > > Applied to target-arm.next, thanks. > > Apologies for not picking this up earlier, the v2 got lost in the > side-conversation about spam filtering. (Blame gmail for not > doing email threading properly if you like :-)) > Thanks, and no problem. This v2 is what we have downstream for this. I'm working on a further improvement to it (separate feature change) that'll allow setting an id on the device so that all its channels have that the id embedded in them. This'll handle some of the situations we're observing where the qdev paths aren't great for command line added i2c devices. There's a side conversation going on about how best to accomplish this. > > -- PMM >
Re: [PATCH v3] tests/qtest: add qtests for npcm7xx sdhci
On Mon, Feb 21, 2022 at 5:30 AM Peter Maydell wrote: > On Wed, 16 Feb 2022 at 17:30, Peter Maydell > wrote: > > > > On Tue, 8 Feb 2022 at 18:18, Patrick Venture wrote: > > > > > > From: Shengtan Mao > > > > > > Reviewed-by: Hao Wu > > > Reviewed-by: Chris Rauer > > > Signed-off-by: Shengtan Mao > > > Signed-off-by: Patrick Venture > > > --- > > > > > > > > Applied to target-arm.next, thanks. > > This hits assertions in some of the CI jobs, eg: > https://gitlab.com/qemu-project/qemu/-/jobs/2116932769 > > 258/717 qemu:qtest+qtest-arm / qtest-arm/npcm7xx_sdhci-test INTERRUPT > 643.16s killed by signal 6 SIGABRT > ― ✀ > ― > stderr: > ** Message: 06:06:50.205: /tmp/sdhci_F7ETH1 > ** > ERROR:../tests/qtest/npcm7xx_sdhci-test.c:101:sdwrite_read: assertion > failed: (!strcmp(rmsg, msg)) > > ―― > ...terminated. > > so I've dropped it again. > I'm sorry to hear that, I'll have to pick up some cycles in a week or so and see if I can reproduce the issue. > > thanks > -- PMM >
at24c* device
Philippe; Looking at the at24c device, it doesn't support the c02 and other 1-byte addressable eeproms that are supported by the at24c driver in linux, and other compatible eeproms. If I recall correctly, the smbus-eeprom device correctly handles the 1-byte addressable variants, and so maybe it's not required. Before I add block device support to the smbus-eeprom device, I wanted to ask if you or anyone thought it made more sense to fix the at24c to have a address-length field that let it then operate for both versions. The other tweak is that a block is 512 bytes, and the smaller eeprom is 256, so, there will be some other minor logic changes to allow it to basically have a file backing larger than its addressable contents. Thanks, Patrick
[PATCH] hw/arm: add initial mori-bmc board
This is the BMC attached to the OpenBMC Mori board.
Signed-off-by: Patrick Venture
Reviewed-by: Chris Rauer
Reviewed-by: Ilkyun Choi
---
docs/system/arm/nuvoton.rst | 1 +
hw/arm/npcm7xx_boards.c | 32
2 files changed, 33 insertions(+)
diff --git a/docs/system/arm/nuvoton.rst b/docs/system/arm/nuvoton.rst
index adf497e679..ef2792076a 100644
--- a/docs/system/arm/nuvoton.rst
+++ b/docs/system/arm/nuvoton.rst
@@ -21,6 +21,7 @@ Hyperscale applications. The following machines are based on
this chip :
- ``quanta-gbs-bmc``Quanta GBS server BMC
- ``quanta-gsj``Quanta GSJ server BMC
- ``kudo-bmc`` Fii USA Kudo server BMC
+- ``mori-bmc`` Fii USA Mori server BMC
There are also two more SoCs, NPCM710 and NPCM705, which are single-core
variants of NPCM750 and NPCM730, respectively. These are currently not
diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
index d701e5cc55..0678a56156 100644
--- a/hw/arm/npcm7xx_boards.c
+++ b/hw/arm/npcm7xx_boards.c
@@ -34,6 +34,7 @@
#define QUANTA_GSJ_POWER_ON_STRAPS 0x1fff
#define QUANTA_GBS_POWER_ON_STRAPS 0x17ff
#define KUDO_BMC_POWER_ON_STRAPS 0x1fff
+#define MORI_BMC_POWER_ON_STRAPS 0x1fff
static const char npcm7xx_default_bootrom[] = "npcm7xx_bootrom.bin";
@@ -429,6 +430,21 @@ static void kudo_bmc_init(MachineState *machine)
npcm7xx_load_kernel(machine, soc);
}
+static void mori_bmc_init(MachineState *machine)
+{
+NPCM7xxState *soc;
+
+soc = npcm7xx_create_soc(machine, MORI_BMC_POWER_ON_STRAPS);
+npcm7xx_connect_dram(soc, machine->ram);
+qdev_realize(DEVICE(soc), NULL, _fatal);
+
+npcm7xx_load_bootrom(machine, soc);
+npcm7xx_connect_flash(>fiu[1], 0, "mx66u51235f",
+ drive_get(IF_MTD, 3, 0));
+
+npcm7xx_load_kernel(machine, soc);
+}
+
static void npcm7xx_set_soc_type(NPCM7xxMachineClass *nmc, const char *type)
{
NPCM7xxClass *sc = NPCM7XX_CLASS(object_class_by_name(type));
@@ -501,6 +517,18 @@ static void kudo_bmc_machine_class_init(ObjectClass *oc,
void *data)
mc->default_ram_size = 1 * GiB;
};
+static void mori_bmc_machine_class_init(ObjectClass *oc, void *data)
+{
+NPCM7xxMachineClass *nmc = NPCM7XX_MACHINE_CLASS(oc);
+MachineClass *mc = MACHINE_CLASS(oc);
+
+npcm7xx_set_soc_type(nmc, TYPE_NPCM730);
+
+mc->desc = "Mori BMC (Cortex-A9)";
+mc->init = mori_bmc_init;
+mc->default_ram_size = 1 * GiB;
+}
+
static const TypeInfo npcm7xx_machine_types[] = {
{
.name = TYPE_NPCM7XX_MACHINE,
@@ -525,6 +553,10 @@ static const TypeInfo npcm7xx_machine_types[] = {
.name = MACHINE_TYPE_NAME("kudo-bmc"),
.parent = TYPE_NPCM7XX_MACHINE,
.class_init = kudo_bmc_machine_class_init,
+}, {
+.name = MACHINE_TYPE_NAME("mori-bmc"),
+.parent = TYPE_NPCM7XX_MACHINE,
+.class_init = mori_bmc_machine_class_init,
},
};
--
2.35.0.263.gb82422642f-goog
[PATCH v3] tests/qtest: add qtests for npcm7xx sdhci
From: Shengtan Mao
Reviewed-by: Hao Wu
Reviewed-by: Chris Rauer
Signed-off-by: Shengtan Mao
Signed-off-by: Patrick Venture
---
v3:
* fixup compilation from missing macro value
v2:
* update copyright year
* check result of open
* use g_free instead of free
* move declarations to the top
* use g_file_open_tmp
---
tests/qtest/meson.build | 1 +
tests/qtest/npcm7xx_sdhci-test.c | 215 +++
2 files changed, 216 insertions(+)
create mode 100644 tests/qtest/npcm7xx_sdhci-test.c
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 842b1df420..2b566999cd 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -189,6 +189,7 @@ qtests_npcm7xx = \
'npcm7xx_gpio-test',
'npcm7xx_pwm-test',
'npcm7xx_rng-test',
+ 'npcm7xx_sdhci-test',
'npcm7xx_smbus-test',
'npcm7xx_timer-test',
'npcm7xx_watchdog_timer-test'] + \
diff --git a/tests/qtest/npcm7xx_sdhci-test.c b/tests/qtest/npcm7xx_sdhci-test.c
new file mode 100644
index 00..7de28f900b
--- /dev/null
+++ b/tests/qtest/npcm7xx_sdhci-test.c
@@ -0,0 +1,215 @@
+/*
+ * QTests for NPCM7xx SD-3.0 / MMC-4.51 Host Controller
+ *
+ * Copyright (c) 2022 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/sd/npcm7xx_sdhci.h"
+
+#include "libqos/libqtest.h"
+#include "libqtest-single.h"
+#include "libqos/sdhci-cmd.h"
+
+#define NPCM7XX_REG_SIZE 0x100
+#define NPCM7XX_MMC_BA 0xF0842000
+#define NPCM7XX_BLK_SIZE 512
+#define NPCM7XX_TEST_IMAGE_SIZE (1 << 30)
+
+char *sd_path;
+
+static QTestState *setup_sd_card(void)
+{
+QTestState *qts = qtest_initf(
+"-machine kudo-bmc "
+"-device sd-card,drive=drive0 "
+"-drive id=drive0,if=none,file=%s,format=raw,auto-read-only=off",
+sd_path);
+
+qtest_writew(qts, NPCM7XX_MMC_BA + SDHC_SWRST, SDHC_RESET_ALL);
+qtest_writew(qts, NPCM7XX_MMC_BA + SDHC_CLKCON,
+ SDHC_CLOCK_SDCLK_EN | SDHC_CLOCK_INT_STABLE |
+ SDHC_CLOCK_INT_EN);
+sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_APP_CMD);
+sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x4120, 0, (41 << 8));
+sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_ALL_SEND_CID);
+sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_SEND_RELATIVE_ADDR);
+sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x4567, 0,
+ SDHC_SELECT_DESELECT_CARD);
+
+return qts;
+}
+
+static void write_sdread(QTestState *qts, const char *msg)
+{
+int fd, ret;
+size_t len = strlen(msg);
+char *rmsg = g_malloc(len);
+
+/* write message to sd */
+fd = open(sd_path, O_WRONLY);
+g_assert(fd >= 0);
+ret = write(fd, msg, len);
+close(fd);
+g_assert(ret == len);
+
+/* read message using sdhci */
+ret = sdhci_read_cmd(qts, NPCM7XX_MMC_BA, rmsg, len);
+g_assert(ret == len);
+g_assert(!strcmp(rmsg, msg));
+
+g_free(rmsg);
+}
+
+/* Check MMC can read values from sd */
+static void test_read_sd(void)
+{
+QTestState *qts = setup_sd_card();
+
+write_sdread(qts, "hello world");
+write_sdread(qts, "goodbye");
+
+qtest_quit(qts);
+}
+
+static void sdwrite_read(QTestState *qts, const char *msg)
+{
+int fd, ret;
+size_t len = strlen(msg);
+char *rmsg = g_malloc(len);
+
+/* write message using sdhci */
+sdhci_write_cmd(qts, NPCM7XX_MMC_BA, msg, len, NPCM7XX_BLK_SIZE);
+
+/* read message from sd */
+fd = open(sd_path, O_RDONLY);
+g_assert(fd >= 0);
+ret = read(fd, rmsg, len);
+close(fd);
+g_assert(ret == len);
+
+g_assert(!strcmp(rmsg, msg));
+
+g_free(rmsg);
+}
+
+/* Check MMC can write values to sd */
+static void test_write_sd(void)
+{
+QTestState *qts = setup_sd_card();
+
+sdwrite_read(qts, "hello world");
+sdwrite_read(qts, "goodbye");
+
+qtest_quit(qts);
+}
+
+/* Check SDHCI has correct default values. */
+static void test_reset(void)
+{
+QTestState *qts = qtest_init("-machine kudo-bmc");
+uint64_t addr = NPCM7XX_MMC_BA;
+uint64_t end_addr = addr + NPCM7XX_REG_SIZE;
+uint16_t prstvals_resets[] = {NPCM7XX_PRSTVALS_0_RESET,
+ NPCM7XX_PRSTVALS_1_RESET,
+ 0,
+ NPCM7XX_PRSTVALS_3_RESET,
+
Re: [PATCH v2] tests/qtest: add qtests for npcm7xx sdhci
On Mon, Feb 7, 2022 at 3:07 PM Patrick Venture wrote: > > > On Mon, Feb 7, 2022 at 9:34 AM Peter Maydell > wrote: > >> On Sun, 6 Feb 2022 at 01:41, Patrick Venture wrote: >> > >> > From: Shengtan Mao >> > >> > Reviewed-by: Hao Wu >> > Reviewed-by: Chris Rauer >> > Signed-off-by: Shengtan Mao >> > Signed-off-by: Patrick Venture >> > --- >> > v2: >> > * update copyright year >> > * check result of open >> > * use g_free instead of free >> > * move declarations to the top >> > * use g_file_open_tmp >> >> Fails to compile: >> >> ../../tests/qtest/npcm7xx_sdhci-test.c:121:32: error: use of >> undeclared identifier 'NPCM7XX_REG_SIZE' >> uint64_t end_addr = addr + NPCM7XX_REG_SIZE; >>^ >> > > Thanks. I must have only compiled at a part-way point while tweaking it. > I'll see if it compiles for me, and then figure out why it does when it > doesn't, or if it doesn't, then obviously fix it. Either way, will fix in > v3, thanks. > Validated that when I build it, it's not getting built :) So that's why this slipped through. I'll dig into the history to see what this value should be now. > > >> >> >> -- PMM >> >
Re: [PATCH v2] tests/qtest: add qtests for npcm7xx sdhci
On Mon, Feb 7, 2022 at 9:34 AM Peter Maydell wrote: > On Sun, 6 Feb 2022 at 01:41, Patrick Venture wrote: > > > > From: Shengtan Mao > > > > Reviewed-by: Hao Wu > > Reviewed-by: Chris Rauer > > Signed-off-by: Shengtan Mao > > Signed-off-by: Patrick Venture > > --- > > v2: > > * update copyright year > > * check result of open > > * use g_free instead of free > > * move declarations to the top > > * use g_file_open_tmp > > Fails to compile: > > ../../tests/qtest/npcm7xx_sdhci-test.c:121:32: error: use of > undeclared identifier 'NPCM7XX_REG_SIZE' > uint64_t end_addr = addr + NPCM7XX_REG_SIZE; >^ > Thanks. I must have only compiled at a part-way point while tweaking it. I'll see if it compiles for me, and then figure out why it does when it doesn't, or if it doesn't, then obviously fix it. Either way, will fix in v3, thanks. > > > -- PMM >
[PATCH v2] tests/qtest: add qtests for npcm7xx sdhci
From: Shengtan Mao
Reviewed-by: Hao Wu
Reviewed-by: Chris Rauer
Signed-off-by: Shengtan Mao
Signed-off-by: Patrick Venture
---
v2:
* update copyright year
* check result of open
* use g_free instead of free
* move declarations to the top
* use g_file_open_tmp
---
tests/qtest/meson.build | 1 +
tests/qtest/npcm7xx_sdhci-test.c | 214 +++
2 files changed, 215 insertions(+)
create mode 100644 tests/qtest/npcm7xx_sdhci-test.c
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 842b1df420..2b566999cd 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -189,6 +189,7 @@ qtests_npcm7xx = \
'npcm7xx_gpio-test',
'npcm7xx_pwm-test',
'npcm7xx_rng-test',
+ 'npcm7xx_sdhci-test',
'npcm7xx_smbus-test',
'npcm7xx_timer-test',
'npcm7xx_watchdog_timer-test'] + \
diff --git a/tests/qtest/npcm7xx_sdhci-test.c b/tests/qtest/npcm7xx_sdhci-test.c
new file mode 100644
index 00..e6f8d9c38c
--- /dev/null
+++ b/tests/qtest/npcm7xx_sdhci-test.c
@@ -0,0 +1,214 @@
+/*
+ * QTests for NPCM7xx SD-3.0 / MMC-4.51 Host Controller
+ *
+ * Copyright (c) 2022 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/sd/npcm7xx_sdhci.h"
+
+#include "libqos/libqtest.h"
+#include "libqtest-single.h"
+#include "libqos/sdhci-cmd.h"
+
+#define NPCM7XX_MMC_BA 0xF0842000
+#define NPCM7XX_BLK_SIZE 512
+#define NPCM7XX_TEST_IMAGE_SIZE (1 << 30)
+
+char *sd_path;
+
+static QTestState *setup_sd_card(void)
+{
+QTestState *qts = qtest_initf(
+"-machine kudo-bmc "
+"-device sd-card,drive=drive0 "
+"-drive id=drive0,if=none,file=%s,format=raw,auto-read-only=off",
+sd_path);
+
+qtest_writew(qts, NPCM7XX_MMC_BA + SDHC_SWRST, SDHC_RESET_ALL);
+qtest_writew(qts, NPCM7XX_MMC_BA + SDHC_CLKCON,
+ SDHC_CLOCK_SDCLK_EN | SDHC_CLOCK_INT_STABLE |
+ SDHC_CLOCK_INT_EN);
+sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_APP_CMD);
+sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x4120, 0, (41 << 8));
+sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_ALL_SEND_CID);
+sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0, 0, SDHC_SEND_RELATIVE_ADDR);
+sdhci_cmd_regs(qts, NPCM7XX_MMC_BA, 0, 0, 0x4567, 0,
+ SDHC_SELECT_DESELECT_CARD);
+
+return qts;
+}
+
+static void write_sdread(QTestState *qts, const char *msg)
+{
+int fd, ret;
+size_t len = strlen(msg);
+char *rmsg = g_malloc(len);
+
+/* write message to sd */
+fd = open(sd_path, O_WRONLY);
+g_assert(fd >= 0);
+ret = write(fd, msg, len);
+close(fd);
+g_assert(ret == len);
+
+/* read message using sdhci */
+ret = sdhci_read_cmd(qts, NPCM7XX_MMC_BA, rmsg, len);
+g_assert(ret == len);
+g_assert(!strcmp(rmsg, msg));
+
+g_free(rmsg);
+}
+
+/* Check MMC can read values from sd */
+static void test_read_sd(void)
+{
+QTestState *qts = setup_sd_card();
+
+write_sdread(qts, "hello world");
+write_sdread(qts, "goodbye");
+
+qtest_quit(qts);
+}
+
+static void sdwrite_read(QTestState *qts, const char *msg)
+{
+int fd, ret;
+size_t len = strlen(msg);
+char *rmsg = g_malloc(len);
+
+/* write message using sdhci */
+sdhci_write_cmd(qts, NPCM7XX_MMC_BA, msg, len, NPCM7XX_BLK_SIZE);
+
+/* read message from sd */
+fd = open(sd_path, O_RDONLY);
+g_assert(fd >= 0);
+ret = read(fd, rmsg, len);
+close(fd);
+g_assert(ret == len);
+
+g_assert(!strcmp(rmsg, msg));
+
+g_free(rmsg);
+}
+
+/* Check MMC can write values to sd */
+static void test_write_sd(void)
+{
+QTestState *qts = setup_sd_card();
+
+sdwrite_read(qts, "hello world");
+sdwrite_read(qts, "goodbye");
+
+qtest_quit(qts);
+}
+
+/* Check SDHCI has correct default values. */
+static void test_reset(void)
+{
+QTestState *qts = qtest_init("-machine kudo-bmc");
+uint64_t addr = NPCM7XX_MMC_BA;
+uint64_t end_addr = addr + NPCM7XX_REG_SIZE;
+uint16_t prstvals_resets[] = {NPCM7XX_PRSTVALS_0_RESET,
+ NPCM7XX_PRSTVALS_1_RESET,
+ 0,
+ NPCM7XX_PRSTVALS_3_RESET,
+ 0,
+ 0};
+int i;
+uint32_t mask
Re: [PATCH] hw/i2c: flatten pca954x mux device
On Wed, Feb 2, 2022 at 9:31 AM Philippe Mathieu-Daudé
wrote:
> On 2/2/22 17:40, Patrick Venture wrote:
>
> > Philippe,
> >
> > I0202 08:29:45.380384 6641 stream.go:31] qemu: child buses at
> > "pca9546": "channel[*]", "channel[*]", "channel[*]", "channel[*]"
> >
> > Ok, so that's interesting. In one system (using qom-list) it's
> > correct, but then when using it to do path assignment
> > (qdev-monitor), it fails...
> >
> > I'm not as fond of the name i2c-bus.%d, since they're referred to as
> > channels in the datasheet. If I do the manual name creation, can I
> > keep the name channel or should I pivot over?
> >
> > Thanks
> >
> >
> > -- >8 --
> > diff --git a/hw/i2c/i2c_mux_pca954x.c
> b/hw/i2c/i2c_mux_pca954x.c
> > index f9ce633b3a..a9517b612a 100644
> > --- a/hw/i2c/i2c_mux_pca954x.c
> > +++ b/hw/i2c/i2c_mux_pca954x.c
> > @@ -189,9 +189,11 @@ static void pca954x_init(Object *obj)
> >
> >/* SMBus modules. Cannot fail. */
> >for (i = 0; i < c->nchans; i++) {
> > +g_autofree gchar *bus_name =
> > g_strdup_printf("i2c.%d", i);
> > +
> >/* start all channels as disabled. */
> >s->enabled[i] = false;
> > -s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
> > +s->bus[i] = i2c_init_bus(DEVICE(s), bus_name);
> >}
> >}
> >
> > ---
> >
> > (look at HMP 'info qtree' output).
> >
> > > }
> > > }
> >
> > With the change:
> > Reviewed-by: Philippe Mathieu-Daudé > <mailto:f4...@amsat.org>>
> > Tested-by: Philippe Mathieu-Daudé > <mailto:f4...@amsat.org>>
> >
> >
> > Just saw your reply, and found a bunch of other non-spam in my spam
> > folder. I sent the message to the anti-spam team, hopefully that'll
> > resolve this for myself and presumably others.
>
> Thanks. I suppose the problem is the amsat.org domain.
>
Yours aren't the only ones I've missed, but who knows.
>
> > I definitely see the same result with the qdev-monitor, but was really
> > surprised that the qom-list worked. I'll explicitly set the name, and
> > i2c.%d is fine. The detail that they're channels is not really
> > important to the end user presumably.
>
> I agree it is better to follow datasheets, thus I am fine if you
> change and use channel. How would it look like? "channel.0"?
> FYI qdev busses are described in docs/qdev-device-use.txt.
>
Thanks. I went with i2c.%d in v2, since I figured it wasn't super
important.
>
> We should be able to plug a device using some command line
> such "-device i2c_test_dev,bus=channel.0,addr=0x55".
> I wonder how to select the base PCA9548 ...
>
So I have been working on that, and I have been running into a different
issue, but related.
/smbus[1]/i2c-bus/pca9546/i2c.0 works to add a device via command line.
However, if there are two pca9546s on that main bus. So if i do:
/smbus[1]/i2c-bus/child[0]/i2c.0 it'll respond that there is no child[0],
but rather includes "pca9546, pca9546"
>
> Maybe we need to pass the PCA ID to pca954x_init(), so we can
> name "channel.2.0" for the 1st channel on the 2nd PCA?
>
It sounds like you're thinking about the same problem overall.
>
> Regards,
>
> Phil.
>
Re: [PATCH] hw/i2c: flatten pca954x mux device
On Wed, Feb 2, 2022 at 11:01 AM Peter Maydell wrote: > On Wed, 2 Feb 2022 at 17:36, Patrick Venture wrote: > > Just saw your reply, and found a bunch of other non-spam in my > > spam folder. I sent the message to the anti-spam team, hopefully > > that'll resolve this for myself and presumably others. > > I dunno if you folk get a specially tuned version or just > the standard gmail spam filter, but IME it's not very good > with mailing list traffic. In particular "this is a patch" > should be a really really easy thing to detect as not-spam > but it doesn't always manage it. I have my filters set to > "Do not send to spam" for mailing list traffic... > I'm sure we have some dogfood version. I have a rule set to all mailing list from qemu-devel to go into a label and everything... but it gets the label and then is sometimes sent right to spam, even in messages where it's an active thread (like this one). And I just saw some other messages I missed. > > -- PMM >
Re: [PATCH] hw/i2c: flatten pca954x mux device
On Tue, Feb 1, 2022 at 12:54 PM Patrick Venture wrote:
>
>
> On Tue, Feb 1, 2022 at 11:02 AM Philippe Mathieu-Daudé
> wrote:
>
>> On 1/2/22 17:30, Patrick Venture wrote:
>> > Previously this device created N subdevices which each owned an i2c bus.
>> > Now this device simply owns the N i2c busses directly.
>> >
>> > Tested: Verified devices behind mux are still accessible via qmp and i2c
>> > from within an arm32 SoC.
>> >
>> > Reviewed-by: Hao Wu
>> > Signed-off-by: Patrick Venture
>> > ---
>> > hw/i2c/i2c_mux_pca954x.c | 75 ++--
>> > 1 file changed, 11 insertions(+), 64 deletions(-)
>>
>> > static void pca954x_init(Object *obj)
>> > {
>> > Pca954xState *s = PCA954X(obj);
>> > Pca954xClass *c = PCA954X_GET_CLASS(obj);
>> > int i;
>> >
>> > -/* Only initialize the children we expect. */
>> > +/* SMBus modules. Cannot fail. */
>> > for (i = 0; i < c->nchans; i++) {
>> > -object_initialize_child(obj, "channel[*]", >channel[i],
>> > -TYPE_PCA954X_CHANNEL);
>> > +/* start all channels as disabled. */
>> > +s->enabled[i] = false;
>> > +s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
>>
>> This is not a QOM property, so you need to initialize manually:
>>
>
> that was my suspicion but this is the output I'm seeing:
>
> {'execute': 'qom-list', 'arguments': { 'path':
> '/machine/soc/smbus[0]/i2c-bus/child[0]' }}
>
> {"return": [
> {"name": "type", "type": "string"},
> {"name": "parent_bus", "type": "link"},
> {"name": "realized", "type": "bool"},
> {"name": "hotplugged", "type": "bool"},
> {"name": "hotpluggable", "type": "bool"},
> {"name": "address", "type": "uint8"},
> {"name": "channel[3]", "type": "child"},
> {"name": "channel[0]", "type": "child"},
> {"name": "channel[1]", "type": "child"},
> {"name": "channel[2]", "type": "child"}
> ]}
>
> It seems to be naming them via the order they're created.
>
> Is this not behaving how you expect?
>
Philippe,
I0202 08:29:45.380384 6641 stream.go:31] qemu: child buses at "pca9546":
"channel[*]", "channel[*]", "channel[*]", "channel[*]"
Ok, so that's interesting. In one system (using qom-list) it's correct,
but then when using it to do path assignment (qdev-monitor), it fails...
I'm not as fond of the name i2c-bus.%d, since they're referred to as
channels in the datasheet. If I do the manual name creation, can I keep
the name channel or should I pivot over?
Thanks
>
>>
>> -- >8 --
>> diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
>> index f9ce633b3a..a9517b612a 100644
>> --- a/hw/i2c/i2c_mux_pca954x.c
>> +++ b/hw/i2c/i2c_mux_pca954x.c
>> @@ -189,9 +189,11 @@ static void pca954x_init(Object *obj)
>>
>> /* SMBus modules. Cannot fail. */
>> for (i = 0; i < c->nchans; i++) {
>> +g_autofree gchar *bus_name = g_strdup_printf("i2c.%d", i);
>> +
>> /* start all channels as disabled. */
>> s->enabled[i] = false;
>> -s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
>> +s->bus[i] = i2c_init_bus(DEVICE(s), bus_name);
>> }
>> }
>>
>> ---
>>
>> (look at HMP 'info qtree' output).
>>
>> > }
>> > }
>>
>> With the change:
>> Reviewed-by: Philippe Mathieu-Daudé
>> Tested-by: Philippe Mathieu-Daudé
>>
>
[PATCH v2] hw/i2c: flatten pca954x mux device
Previously this device created N subdevices which each owned an i2c bus.
Now this device simply owns the N i2c busses directly.
Tested: Verified devices behind mux are still accessible via qmp and i2c
from within an arm32 SoC.
Reviewed-by: Hao Wu
Signed-off-by: Patrick Venture
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Philippe Mathieu-Daudé
---
v2: explicitly create an incrementing name for the i2c busses (channels).
---
hw/i2c/i2c_mux_pca954x.c | 77 +++-
1 file changed, 13 insertions(+), 64 deletions(-)
diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
index 847c59921c..a9517b612a 100644
--- a/hw/i2c/i2c_mux_pca954x.c
+++ b/hw/i2c/i2c_mux_pca954x.c
@@ -30,24 +30,6 @@
#define PCA9548_CHANNEL_COUNT 8
#define PCA9546_CHANNEL_COUNT 4
-/*
- * struct Pca954xChannel - The i2c mux device will have N of these states
- * that own the i2c channel bus.
- * @bus: The owned channel bus.
- * @enabled: Is this channel active?
- */
-typedef struct Pca954xChannel {
-SysBusDevice parent;
-
-I2CBus *bus;
-
-bool enabled;
-} Pca954xChannel;
-
-#define TYPE_PCA954X_CHANNEL "pca954x-channel"
-#define PCA954X_CHANNEL(obj) \
-OBJECT_CHECK(Pca954xChannel, (obj), TYPE_PCA954X_CHANNEL)
-
/*
* struct Pca954xState - The pca954x state object.
* @control: The value written to the mux control.
@@ -59,8 +41,8 @@ typedef struct Pca954xState {
uint8_t control;
-/* The channel i2c buses. */
-Pca954xChannel channel[PCA9548_CHANNEL_COUNT];
+bool enabled[PCA9548_CHANNEL_COUNT];
+I2CBus *bus[PCA9548_CHANNEL_COUNT];
} Pca954xState;
/*
@@ -98,11 +80,11 @@ static bool pca954x_match(I2CSlave *candidate, uint8_t
address,
}
for (i = 0; i < mc->nchans; i++) {
-if (!mux->channel[i].enabled) {
+if (!mux->enabled[i]) {
continue;
}
-if (i2c_scan_bus(mux->channel[i].bus, address, broadcast,
+if (i2c_scan_bus(mux->bus[i], address, broadcast,
current_devs)) {
if (!broadcast) {
return true;
@@ -125,9 +107,9 @@ static void pca954x_enable_channel(Pca954xState *s, uint8_t
enable_mask)
*/
for (i = 0; i < mc->nchans; i++) {
if (enable_mask & (1 << i)) {
-s->channel[i].enabled = true;
+s->enabled[i] = true;
} else {
-s->channel[i].enabled = false;
+s->enabled[i] = false;
}
}
}
@@ -184,23 +166,7 @@ I2CBus *pca954x_i2c_get_bus(I2CSlave *mux, uint8_t channel)
Pca954xState *pca954x = PCA954X(mux);
g_assert(channel < pc->nchans);
-return I2C_BUS(qdev_get_child_bus(DEVICE(>channel[channel]),
- "i2c-bus"));
-}
-
-static void pca954x_channel_init(Object *obj)
-{
-Pca954xChannel *s = PCA954X_CHANNEL(obj);
-s->bus = i2c_init_bus(DEVICE(s), "i2c-bus");
-
-/* Start all channels as disabled. */
-s->enabled = false;
-}
-
-static void pca954x_channel_class_init(ObjectClass *klass, void *data)
-{
-DeviceClass *dc = DEVICE_CLASS(klass);
-dc->desc = "Pca954x Channel";
+return pca954x->bus[channel];
}
static void pca9546_class_init(ObjectClass *klass, void *data)
@@ -215,28 +181,19 @@ static void pca9548_class_init(ObjectClass *klass, void
*data)
s->nchans = PCA9548_CHANNEL_COUNT;
}
-static void pca954x_realize(DeviceState *dev, Error **errp)
-{
-Pca954xState *s = PCA954X(dev);
-Pca954xClass *c = PCA954X_GET_CLASS(s);
-int i;
-
-/* SMBus modules. Cannot fail. */
-for (i = 0; i < c->nchans; i++) {
-sysbus_realize(SYS_BUS_DEVICE(>channel[i]), _abort);
-}
-}
-
static void pca954x_init(Object *obj)
{
Pca954xState *s = PCA954X(obj);
Pca954xClass *c = PCA954X_GET_CLASS(obj);
int i;
-/* Only initialize the children we expect. */
+/* SMBus modules. Cannot fail. */
for (i = 0; i < c->nchans; i++) {
-object_initialize_child(obj, "channel[*]", >channel[i],
-TYPE_PCA954X_CHANNEL);
+g_autofree gchar *bus_name = g_strdup_printf("i2c.%d", i);
+
+/* start all channels as disabled. */
+s->enabled[i] = false;
+s->bus[i] = i2c_init_bus(DEVICE(s), bus_name);
}
}
@@ -252,7 +209,6 @@ static void pca954x_class_init(ObjectClass *klass, void
*data)
rc->phases.enter = pca954x_enter_reset;
dc->desc = "Pca954x i2c-mux";
-dc->realize = pca954x_realize;
k->write_data = pca954x_write_data;
k->receive_byte = pca954x_read_byte;
@@ -278,13 +234,6 @@ static const TypeInfo pca954x_info[] = {
.parent= TYPE_PCA954X,
.class_init= pca9548_class_init,
},
-{
-.name = TYPE_PCA954X_CHANNEL,
-
Re: [PATCH] hw/i2c: flatten pca954x mux device
On Wed, Feb 2, 2022 at 8:34 AM Patrick Venture wrote:
>
>
> On Tue, Feb 1, 2022 at 12:54 PM Patrick Venture
> wrote:
>
>>
>>
>> On Tue, Feb 1, 2022 at 11:02 AM Philippe Mathieu-Daudé
>> wrote:
>>
>>> On 1/2/22 17:30, Patrick Venture wrote:
>>> > Previously this device created N subdevices which each owned an i2c
>>> bus.
>>> > Now this device simply owns the N i2c busses directly.
>>> >
>>> > Tested: Verified devices behind mux are still accessible via qmp and
>>> i2c
>>> > from within an arm32 SoC.
>>> >
>>> > Reviewed-by: Hao Wu
>>> > Signed-off-by: Patrick Venture
>>> > ---
>>> > hw/i2c/i2c_mux_pca954x.c | 75
>>> ++--
>>> > 1 file changed, 11 insertions(+), 64 deletions(-)
>>>
>>> > static void pca954x_init(Object *obj)
>>> > {
>>> > Pca954xState *s = PCA954X(obj);
>>> > Pca954xClass *c = PCA954X_GET_CLASS(obj);
>>> > int i;
>>> >
>>> > -/* Only initialize the children we expect. */
>>> > +/* SMBus modules. Cannot fail. */
>>> > for (i = 0; i < c->nchans; i++) {
>>> > -object_initialize_child(obj, "channel[*]", >channel[i],
>>> > -TYPE_PCA954X_CHANNEL);
>>> > +/* start all channels as disabled. */
>>> > +s->enabled[i] = false;
>>> > +s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
>>>
>>> This is not a QOM property, so you need to initialize manually:
>>>
>>
>> that was my suspicion but this is the output I'm seeing:
>>
>> {'execute': 'qom-list', 'arguments': { 'path':
>> '/machine/soc/smbus[0]/i2c-bus/child[0]' }}
>>
>> {"return": [
>> {"name": "type", "type": "string"},
>> {"name": "parent_bus", "type": "link"},
>> {"name": "realized", "type": "bool"},
>> {"name": "hotplugged", "type": "bool"},
>> {"name": "hotpluggable", "type": "bool"},
>> {"name": "address", "type": "uint8"},
>> {"name": "channel[3]", "type": "child"},
>> {"name": "channel[0]", "type": "child"},
>> {"name": "channel[1]", "type": "child"},
>> {"name": "channel[2]", "type": "child"}
>> ]}
>>
>> It seems to be naming them via the order they're created.
>>
>> Is this not behaving how you expect?
>>
>
> Philippe,
>
> I0202 08:29:45.380384 6641 stream.go:31] qemu: child buses at "pca9546":
> "channel[*]", "channel[*]", "channel[*]", "channel[*]"
>
> Ok, so that's interesting. In one system (using qom-list) it's correct,
> but then when using it to do path assignment (qdev-monitor), it fails...
>
> I'm not as fond of the name i2c-bus.%d, since they're referred to as
> channels in the datasheet. If I do the manual name creation, can I keep
> the name channel or should I pivot over?
>
> Thanks
>
>
>>
>>>
>>> -- >8 --
>>> diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
>>> index f9ce633b3a..a9517b612a 100644
>>> --- a/hw/i2c/i2c_mux_pca954x.c
>>> +++ b/hw/i2c/i2c_mux_pca954x.c
>>> @@ -189,9 +189,11 @@ static void pca954x_init(Object *obj)
>>>
>>> /* SMBus modules. Cannot fail. */
>>> for (i = 0; i < c->nchans; i++) {
>>> +g_autofree gchar *bus_name = g_strdup_printf("i2c.%d", i);
>>> +
>>> /* start all channels as disabled. */
>>> s->enabled[i] = false;
>>> -s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
>>> +s->bus[i] = i2c_init_bus(DEVICE(s), bus_name);
>>> }
>>> }
>>>
>>> ---
>>>
>>> (look at HMP 'info qtree' output).
>>>
>>> > }
>>> > }
>>>
>>> With the change:
>>> Reviewed-by: Philippe Mathieu-Daudé
>>> Tested-by: Philippe Mathieu-Daudé
>>>
>>
Just saw your reply, and found a bunch of other non-spam in my spam
folder. I sent the message to the anti-spam team, hopefully that'll
resolve this for myself and presumably others.
I definitely see the same result with the qdev-monitor, but was really
surprised that the qom-list worked. I'll explicitly set the name, and
i2c.%d is fine. The detail that they're channels is not really important
to the end user presumably.
I'll have v2 out shortly.
thanks,
Patrick
Re: [PATCH] hw/i2c: flatten pca954x mux device
On Tue, Feb 1, 2022 at 11:02 AM Philippe Mathieu-Daudé
wrote:
> On 1/2/22 17:30, Patrick Venture wrote:
> > Previously this device created N subdevices which each owned an i2c bus.
> > Now this device simply owns the N i2c busses directly.
> >
> > Tested: Verified devices behind mux are still accessible via qmp and i2c
> > from within an arm32 SoC.
> >
> > Reviewed-by: Hao Wu
> > Signed-off-by: Patrick Venture
> > ---
> > hw/i2c/i2c_mux_pca954x.c | 75 ++--
> > 1 file changed, 11 insertions(+), 64 deletions(-)
>
> > static void pca954x_init(Object *obj)
> > {
> > Pca954xState *s = PCA954X(obj);
> > Pca954xClass *c = PCA954X_GET_CLASS(obj);
> > int i;
> >
> > -/* Only initialize the children we expect. */
> > +/* SMBus modules. Cannot fail. */
> > for (i = 0; i < c->nchans; i++) {
> > -object_initialize_child(obj, "channel[*]", >channel[i],
> > -TYPE_PCA954X_CHANNEL);
> > +/* start all channels as disabled. */
> > +s->enabled[i] = false;
> > +s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
>
> This is not a QOM property, so you need to initialize manually:
>
that was my suspicion but this is the output I'm seeing:
{'execute': 'qom-list', 'arguments': { 'path':
'/machine/soc/smbus[0]/i2c-bus/child[0]' }}
{"return": [
{"name": "type", "type": "string"},
{"name": "parent_bus", "type": "link"},
{"name": "realized", "type": "bool"},
{"name": "hotplugged", "type": "bool"},
{"name": "hotpluggable", "type": "bool"},
{"name": "address", "type": "uint8"},
{"name": "channel[3]", "type": "child"},
{"name": "channel[0]", "type": "child"},
{"name": "channel[1]", "type": "child"},
{"name": "channel[2]", "type": "child"}
]}
It seems to be naming them via the order they're created.
Is this not behaving how you expect?
>
> -- >8 --
> diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
> index f9ce633b3a..a9517b612a 100644
> --- a/hw/i2c/i2c_mux_pca954x.c
> +++ b/hw/i2c/i2c_mux_pca954x.c
> @@ -189,9 +189,11 @@ static void pca954x_init(Object *obj)
>
> /* SMBus modules. Cannot fail. */
> for (i = 0; i < c->nchans; i++) {
> +g_autofree gchar *bus_name = g_strdup_printf("i2c.%d", i);
> +
> /* start all channels as disabled. */
> s->enabled[i] = false;
> -s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
> +s->bus[i] = i2c_init_bus(DEVICE(s), bus_name);
> }
> }
>
> ---
>
> (look at HMP 'info qtree' output).
>
> > }
> > }
>
> With the change:
> Reviewed-by: Philippe Mathieu-Daudé
> Tested-by: Philippe Mathieu-Daudé
>
[PATCH] hw/i2c: flatten pca954x mux device
Previously this device created N subdevices which each owned an i2c bus.
Now this device simply owns the N i2c busses directly.
Tested: Verified devices behind mux are still accessible via qmp and i2c
from within an arm32 SoC.
Reviewed-by: Hao Wu
Signed-off-by: Patrick Venture
---
hw/i2c/i2c_mux_pca954x.c | 75 ++--
1 file changed, 11 insertions(+), 64 deletions(-)
diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
index 847c59921c..f9ce633b3a 100644
--- a/hw/i2c/i2c_mux_pca954x.c
+++ b/hw/i2c/i2c_mux_pca954x.c
@@ -30,24 +30,6 @@
#define PCA9548_CHANNEL_COUNT 8
#define PCA9546_CHANNEL_COUNT 4
-/*
- * struct Pca954xChannel - The i2c mux device will have N of these states
- * that own the i2c channel bus.
- * @bus: The owned channel bus.
- * @enabled: Is this channel active?
- */
-typedef struct Pca954xChannel {
-SysBusDevice parent;
-
-I2CBus *bus;
-
-bool enabled;
-} Pca954xChannel;
-
-#define TYPE_PCA954X_CHANNEL "pca954x-channel"
-#define PCA954X_CHANNEL(obj) \
-OBJECT_CHECK(Pca954xChannel, (obj), TYPE_PCA954X_CHANNEL)
-
/*
* struct Pca954xState - The pca954x state object.
* @control: The value written to the mux control.
@@ -59,8 +41,8 @@ typedef struct Pca954xState {
uint8_t control;
-/* The channel i2c buses. */
-Pca954xChannel channel[PCA9548_CHANNEL_COUNT];
+bool enabled[PCA9548_CHANNEL_COUNT];
+I2CBus *bus[PCA9548_CHANNEL_COUNT];
} Pca954xState;
/*
@@ -98,11 +80,11 @@ static bool pca954x_match(I2CSlave *candidate, uint8_t
address,
}
for (i = 0; i < mc->nchans; i++) {
-if (!mux->channel[i].enabled) {
+if (!mux->enabled[i]) {
continue;
}
-if (i2c_scan_bus(mux->channel[i].bus, address, broadcast,
+if (i2c_scan_bus(mux->bus[i], address, broadcast,
current_devs)) {
if (!broadcast) {
return true;
@@ -125,9 +107,9 @@ static void pca954x_enable_channel(Pca954xState *s, uint8_t
enable_mask)
*/
for (i = 0; i < mc->nchans; i++) {
if (enable_mask & (1 << i)) {
-s->channel[i].enabled = true;
+s->enabled[i] = true;
} else {
-s->channel[i].enabled = false;
+s->enabled[i] = false;
}
}
}
@@ -184,23 +166,7 @@ I2CBus *pca954x_i2c_get_bus(I2CSlave *mux, uint8_t channel)
Pca954xState *pca954x = PCA954X(mux);
g_assert(channel < pc->nchans);
-return I2C_BUS(qdev_get_child_bus(DEVICE(>channel[channel]),
- "i2c-bus"));
-}
-
-static void pca954x_channel_init(Object *obj)
-{
-Pca954xChannel *s = PCA954X_CHANNEL(obj);
-s->bus = i2c_init_bus(DEVICE(s), "i2c-bus");
-
-/* Start all channels as disabled. */
-s->enabled = false;
-}
-
-static void pca954x_channel_class_init(ObjectClass *klass, void *data)
-{
-DeviceClass *dc = DEVICE_CLASS(klass);
-dc->desc = "Pca954x Channel";
+return pca954x->bus[channel];
}
static void pca9546_class_init(ObjectClass *klass, void *data)
@@ -215,28 +181,17 @@ static void pca9548_class_init(ObjectClass *klass, void
*data)
s->nchans = PCA9548_CHANNEL_COUNT;
}
-static void pca954x_realize(DeviceState *dev, Error **errp)
-{
-Pca954xState *s = PCA954X(dev);
-Pca954xClass *c = PCA954X_GET_CLASS(s);
-int i;
-
-/* SMBus modules. Cannot fail. */
-for (i = 0; i < c->nchans; i++) {
-sysbus_realize(SYS_BUS_DEVICE(>channel[i]), _abort);
-}
-}
-
static void pca954x_init(Object *obj)
{
Pca954xState *s = PCA954X(obj);
Pca954xClass *c = PCA954X_GET_CLASS(obj);
int i;
-/* Only initialize the children we expect. */
+/* SMBus modules. Cannot fail. */
for (i = 0; i < c->nchans; i++) {
-object_initialize_child(obj, "channel[*]", >channel[i],
-TYPE_PCA954X_CHANNEL);
+/* start all channels as disabled. */
+s->enabled[i] = false;
+s->bus[i] = i2c_init_bus(DEVICE(s), "channel[*]");
}
}
@@ -252,7 +207,6 @@ static void pca954x_class_init(ObjectClass *klass, void
*data)
rc->phases.enter = pca954x_enter_reset;
dc->desc = "Pca954x i2c-mux";
-dc->realize = pca954x_realize;
k->write_data = pca954x_write_data;
k->receive_byte = pca954x_read_byte;
@@ -278,13 +232,6 @@ static const TypeInfo pca954x_info[] = {
.parent= TYPE_PCA954X,
.class_init= pca9548_class_init,
},
-{
-.name = TYPE_PCA954X_CHANNEL,
-.parent = TYPE_SYS_BUS_DEVICE,
-.class_init = pca954x_channel_class_init,
-.instance_size = sizeof(Pca954xChannel),
-.instance_init = pca954x_channel_init,
-}
};
DEFINE_TYPES(pca954x_info)
--
2.35.0.rc2.247.g8bbb082509-goog
[PATCH v4 1/2] hw/sensor: Add SB-TSI Temperature Sensor Interface
From: Hao Wu SB Temperature Sensor Interface (SB-TSI) is an SMBus compatible interface that reports AMD SoC's Ttcl (normalized temperature), and resembles a typical 8-pin remote temperature sensor's I2C interface to BMC. This patch implements a basic AMD SB-TSI sensor that is compatible with the open-source data sheet from AMD and Linux kernel driver. Reference: Linux kernel driver: https://lkml.org/lkml/2020/12/11/968 Register Map: https://developer.amd.com/wp-content/resources/56255_3_03.PDF (Chapter 6) Signed-off-by: Hao Wu Signed-off-by: Patrick Venture Reviewed-by: Doug Evans Reviewed-by: Philippe Mathieu-Daudé Acked-by: Corey Minyard --- meson.build | 1 + hw/sensor/trace.h | 1 + include/hw/sensor/sbtsi.h | 45 + hw/sensor/tmp_sbtsi.c | 369 ++ hw/sensor/Kconfig | 4 + hw/sensor/meson.build | 1 + hw/sensor/trace-events| 5 + 7 files changed, 426 insertions(+) create mode 100644 hw/sensor/trace.h create mode 100644 include/hw/sensor/sbtsi.h create mode 100644 hw/sensor/tmp_sbtsi.c create mode 100644 hw/sensor/trace-events diff --git a/meson.build b/meson.build index c1b1db1e28..3634214546 100644 --- a/meson.build +++ b/meson.build @@ -2494,6 +2494,7 @@ if have_system 'hw/rtc', 'hw/s390x', 'hw/scsi', +'hw/sensor', 'hw/sd', 'hw/sh4', 'hw/sparc', diff --git a/hw/sensor/trace.h b/hw/sensor/trace.h new file mode 100644 index 00..e4721560b0 --- /dev/null +++ b/hw/sensor/trace.h @@ -0,0 +1 @@ +#include "trace/trace-hw_sensor.h" diff --git a/include/hw/sensor/sbtsi.h b/include/hw/sensor/sbtsi.h new file mode 100644 index 00..9073f76ebb --- /dev/null +++ b/include/hw/sensor/sbtsi.h @@ -0,0 +1,45 @@ +/* + * AMD SBI Temperature Sensor Interface (SB-TSI) + * + * Copyright 2021 Google LLC + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ +#ifndef QEMU_TMP_SBTSI_H +#define QEMU_TMP_SBTSI_H + +/* + * SB-TSI registers only support SMBus byte data access. "_INT" registers are + * the integer part of a temperature value or limit, and "_DEC" registers are + * corresponding decimal parts. + */ +#define SBTSI_REG_TEMP_INT 0x01 /* RO */ +#define SBTSI_REG_STATUS0x02 /* RO */ +#define SBTSI_REG_CONFIG0x03 /* RO */ +#define SBTSI_REG_TEMP_HIGH_INT 0x07 /* RW */ +#define SBTSI_REG_TEMP_LOW_INT 0x08 /* RW */ +#define SBTSI_REG_CONFIG_WR 0x09 /* RW */ +#define SBTSI_REG_TEMP_DEC 0x10 /* RO */ +#define SBTSI_REG_TEMP_HIGH_DEC 0x13 /* RW */ +#define SBTSI_REG_TEMP_LOW_DEC 0x14 /* RW */ +#define SBTSI_REG_ALERT_CONFIG 0xBF /* RW */ +#define SBTSI_REG_MAN 0xFE /* RO */ +#define SBTSI_REG_REV 0xFF /* RO */ + +#define SBTSI_STATUS_HIGH_ALERT BIT(4) +#define SBTSI_STATUS_LOW_ALERT BIT(3) +#define SBTSI_CONFIG_ALERT_MASK BIT(7) +#define SBTSI_ALARM_EN BIT(0) + +/* The temperature we stored are in units of 0.125 degrees. */ +#define SBTSI_TEMP_UNIT_IN_MILLIDEGREE 125 + +#endif diff --git a/hw/sensor/tmp_sbtsi.c b/hw/sensor/tmp_sbtsi.c new file mode 100644 index 00..d5406844ef --- /dev/null +++ b/hw/sensor/tmp_sbtsi.c @@ -0,0 +1,369 @@ +/* + * AMD SBI Temperature Sensor Interface (SB-TSI) + * + * Copyright 2021 Google LLC + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ + +#include "qemu/osdep.h" +#include "hw/i2c/smbus_slave.h" +#include "hw/sensor/sbtsi.h" +#include "hw/irq.h" +#include "migration/vmstate.h" +#include "qapi/error.h" +#include "qapi/visitor.h" +#include "qemu/log.h" +#include "qemu/module.h" +#include "qom/object.h" +#include "trace.h" + +#define TYPE_SBTSI "sbtsi" +#define SBTSI(obj) OBJECT_CHECK(SBTSIState, (obj), TYPE_SBTSI) + +/** + * SBTSIState: + * temperatures are in units of 0.125 degrees + * @temperature: Temperature + * @limit_low: Lowest temperature + * @limit_high: Highest temperature + * @status: The st
[PATCH v4 0/2] hw/sensor: Add SB-TSI Temperature Sensor Interface
v4: * Added missing signature block from submitter. v3: * typofix where I accidentally embedded 'wq' into a string * moved the type sbtsi definition back into the source file * renamed the qtest file to use hyphens only v2: * Split the commit into a separate patch for the qtest * Moved the common registers into the new header * Introduced a new header Hao Wu (2): hw/sensor: Add SB-TSI Temperature Sensor Interface tests: add qtest for hw/sensor/sbtsi meson.build | 1 + hw/sensor/trace.h| 1 + include/hw/sensor/sbtsi.h| 45 + hw/sensor/tmp_sbtsi.c| 369 +++ tests/qtest/tmp-sbtsi-test.c | 161 +++ hw/sensor/Kconfig| 4 + hw/sensor/meson.build| 1 + hw/sensor/trace-events | 5 + tests/qtest/meson.build | 1 + 9 files changed, 588 insertions(+) create mode 100644 hw/sensor/trace.h create mode 100644 include/hw/sensor/sbtsi.h create mode 100644 hw/sensor/tmp_sbtsi.c create mode 100644 tests/qtest/tmp-sbtsi-test.c create mode 100644 hw/sensor/trace-events -- 2.34.1.575.g55b058a8bb-goog
[PATCH v4 2/2] tests: add qtest for hw/sensor/sbtsi
From: Hao Wu
Reviewed-by: Doug Evans
Signed-off-by: Hao Wu
Signed-off-by: Patrick Venture
Acked-by: Thomas Huth
---
tests/qtest/tmp-sbtsi-test.c | 161 +++
tests/qtest/meson.build | 1 +
2 files changed, 162 insertions(+)
create mode 100644 tests/qtest/tmp-sbtsi-test.c
diff --git a/tests/qtest/tmp-sbtsi-test.c b/tests/qtest/tmp-sbtsi-test.c
new file mode 100644
index 00..ff1e193739
--- /dev/null
+++ b/tests/qtest/tmp-sbtsi-test.c
@@ -0,0 +1,161 @@
+/*
+ * QTests for the SBTSI temperature sensor
+ *
+ * Copyright 2020 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "qemu/osdep.h"
+
+#include "libqtest-single.h"
+#include "libqos/qgraph.h"
+#include "libqos/i2c.h"
+#include "qapi/qmp/qdict.h"
+#include "qemu/bitops.h"
+#include "hw/sensor/sbtsi.h"
+
+#define TEST_ID "sbtsi-test"
+#define TEST_ADDR (0x4c)
+
+/* The temperature stored are in units of 0.125 degrees. */
+#define LIMIT_LOW_IN_MILLIDEGREE (10500)
+#define LIMIT_HIGH_IN_MILLIDEGREE (55125)
+
+static uint32_t qmp_sbtsi_get_temperature(const char *id)
+{
+QDict *response;
+int ret;
+
+response = qmp("{ 'execute': 'qom-get', 'arguments': { 'path': %s, "
+ "'property': 'temperature' } }", id);
+g_assert(qdict_haskey(response, "return"));
+ret = (uint32_t)qdict_get_int(response, "return");
+qobject_unref(response);
+return ret;
+}
+
+static void qmp_sbtsi_set_temperature(const char *id, uint32_t value)
+{
+QDict *response;
+
+response = qmp("{ 'execute': 'qom-set', 'arguments': { 'path': %s, "
+ "'property': 'temperature', 'value': %d } }", id, value);
+g_assert(qdict_haskey(response, "return"));
+qobject_unref(response);
+}
+
+/*
+ * Compute the temperature using the integer and decimal part and return
+ * millidegrees. The decimal part are only the top 3 bits so we shift it by
+ * 5 here.
+ */
+static uint32_t regs_to_temp(uint8_t integer, uint8_t decimal)
+{
+return ((integer << 3) + (decimal >> 5)) * SBTSI_TEMP_UNIT_IN_MILLIDEGREE;
+}
+
+/*
+ * Compute the integer and decimal parts of the temperature in millidegrees.
+ * H/W store the decimal in the top 3 bits so we shift it by 5.
+ */
+static void temp_to_regs(uint32_t temp, uint8_t *integer, uint8_t *decimal)
+{
+temp /= SBTSI_TEMP_UNIT_IN_MILLIDEGREE;
+*integer = temp >> 3;
+*decimal = (temp & 0x7) << 5;
+}
+
+static void tx_rx(void *obj, void *data, QGuestAllocator *alloc)
+{
+uint16_t value;
+uint8_t integer, decimal;
+QI2CDevice *i2cdev = (QI2CDevice *)obj;
+
+/* Test default values */
+value = qmp_sbtsi_get_temperature(TEST_ID);
+g_assert_cmpuint(value, ==, 0);
+
+integer = i2c_get8(i2cdev, SBTSI_REG_TEMP_INT);
+decimal = i2c_get8(i2cdev, SBTSI_REG_TEMP_DEC);
+g_assert_cmpuint(regs_to_temp(integer, decimal), ==, 0);
+
+/* Test setting temperature */
+qmp_sbtsi_set_temperature(TEST_ID, 2);
+value = qmp_sbtsi_get_temperature(TEST_ID);
+g_assert_cmpuint(value, ==, 2);
+
+integer = i2c_get8(i2cdev, SBTSI_REG_TEMP_INT);
+decimal = i2c_get8(i2cdev, SBTSI_REG_TEMP_DEC);
+g_assert_cmpuint(regs_to_temp(integer, decimal), ==, 2);
+
+/* Set alert mask in config */
+i2c_set8(i2cdev, SBTSI_REG_CONFIG_WR, SBTSI_CONFIG_ALERT_MASK);
+value = i2c_get8(i2cdev, SBTSI_REG_CONFIG);
+g_assert_cmphex(value, ==, SBTSI_CONFIG_ALERT_MASK);
+/* Enable alarm_en */
+i2c_set8(i2cdev, SBTSI_REG_ALERT_CONFIG, SBTSI_ALARM_EN);
+value = i2c_get8(i2cdev, SBTSI_REG_ALERT_CONFIG);
+g_assert_cmphex(value, ==, SBTSI_ALARM_EN);
+
+/* Test setting limits */
+/* Limit low = 10.500 */
+temp_to_regs(LIMIT_LOW_IN_MILLIDEGREE, , );
+i2c_set8(i2cdev, SBTSI_REG_TEMP_LOW_INT, integer);
+i2c_set8(i2cdev, SBTSI_REG_TEMP_LOW_DEC, decimal);
+integer = i2c_get8(i2cdev, SBTSI_REG_TEMP_LOW_INT);
+decimal = i2c_get8(i2cdev, SBTSI_REG_TEMP_LOW_DEC);
+g_assert_cmpuint(
+regs_to_temp(integer, decimal), ==, LIMIT_LOW_IN_MILLIDEGREE);
+/* Limit high = 55.125 */
+temp_to_regs(LIMIT_HIGH_IN_MILLIDEGREE, , );
+i2c_set8(i2cdev, SBTSI_REG_TEMP_HIGH_INT, integer);
+i2c_set8(i2cdev, SBTSI_REG_TEMP_HIGH_DEC, decimal);
+integer = i2c_get8(i2cdev, SBTSI_REG_TEMP_HIGH_INT);
+
Re: [PATCH] hw/sensor: Add SB-TSI Temperature Sensor Interface
On Thu, Jan 27, 2022 at 10:20 AM Hao Wu wrote: > > On Thu, Jan 27, 2022 at 6:55 AM Corey Minyard wrote: > >> On Wed, Jan 26, 2022 at 04:09:03PM -0800, Hao Wu wrote: >> > Hi, >> > >> > Sorry for the late reply. I'm not sure what "auto-increment" means here. >> >> The question is: When a value is read, does the address automatically >> increment. I2C devices very often do this. If you do a multi-byte >> read, they often will read the value from address 0, then from address >> 1, etc. Same for writes. >> > The datasheet does not suggest such behavior could happen. > >> >> > The kernel driver at >> > https://lkml.org/lkml/2020/12/11/968 >> > only has byte-size read/write operations, so I don't see a problem here. >> > The values are extracted >> > from the datasheet. >> >> The *Linux* kernel driver my only do byte-size operations, but other >> kernels may work different, and the Linux kernel may change. You have >> to implement from the datasheet, not the device driver. >> > The implementation is already according to the datasheet. > >> Thanks, >> >> -corey >> >> I have to send out a new patchset to include my signature block since I'm sending a patch Hao wrote. Laurent pointed this out to me in a different patch series. >From this exchange, I don't think there are other changes for v2. Please let me know if that's false. Thanks, Patrick > > >> > On Tue, Jan 18, 2022 at 9:10 AM Patrick Venture >> wrote: >> > >> > > >> > > >> > > On Mon, Jan 17, 2022 at 6:05 AM Corey Minyard >> wrote: >> > > >> > >> On Sun, Jan 09, 2022 at 06:17:34PM -0800, Patrick Venture wrote: >> > >> > On Fri, Jan 7, 2022 at 7:04 PM Patrick Venture > > >> > >> wrote: >> > >> > >> > >> > > From: Hao Wu >> > >> > > >> > >> > > SB Temperature Sensor Interface (SB-TSI) is an SMBus compatible >> > >> > > interface that reports AMD SoC's Ttcl (normalized temperature), >> > >> > > and resembles a typical 8-pin remote temperature sensor's I2C >> > >> interface >> > >> > > to BMC. >> > >> > > >> > >> > > This patch implements a basic AMD SB-TSI sensor that is >> > >> > > compatible with the open-source data sheet from AMD and Linux >> > >> > > kernel driver. >> > >> > > >> > >> > > Reference: >> > >> > > Linux kernel driver: >> > >> > > https://lkml.org/lkml/2020/12/11/968 >> > >> > > Register Map: >> > >> > > https://developer.amd.com/wp-content/resources/56255_3_03.PDF >> > >> > > (Chapter 6) >> > >> > > >> > >> > > Signed-off-by: Hao Wu >> > >> > > Reviewed-by: Doug Evans >> > >> > > --- >> > >> > > hw/sensor/Kconfig| 4 + >> > >> > > hw/sensor/meson.build| 1 + >> > >> > > hw/sensor/tmp_sbtsi.c| 393 >> > >> +++ >> > >> > > hw/sensor/trace-events | 5 + >> > >> > > hw/sensor/trace.h| 1 + >> > >> > > meson.build | 1 + >> > >> > > tests/qtest/meson.build | 1 + >> > >> > > tests/qtest/tmp_sbtsi-test.c | 180 >> > >> > > 8 files changed, 586 insertions(+) >> > >> > > create mode 100644 hw/sensor/tmp_sbtsi.c >> > >> > > create mode 100644 hw/sensor/trace-events >> > >> > > create mode 100644 hw/sensor/trace.h >> > >> > > create mode 100644 tests/qtest/tmp_sbtsi-test.c >> > >> > > >> > >> > > diff --git a/hw/sensor/Kconfig b/hw/sensor/Kconfig >> > >> > > index 9c8a049b06..27f6f79c84 100644 >> > >> > > --- a/hw/sensor/Kconfig >> > >> > > +++ b/hw/sensor/Kconfig >> > >> > > @@ -21,3 +21,7 @@ config ADM1272 >> > >> > > config MAX34451 >> > >> > > bool >> > >> > > depends on I2C >> > >> > > + >> > >> > > +config AMDSBTSI >> > >> > > +bool >&
Re: [PATCH 1/2] hw/misc: Add Nuvoton's PCI Mailbox Module
On Thu, Jan 27, 2022 at 10:37 AM Peter Maydell
wrote:
> On Mon, 10 Jan 2022 at 17:56, Patrick Venture wrote:
> >
> > From: Hao Wu
> >
> > The PCI Mailbox Module is a high-bandwidth communcation module
> > between a Nuvoton BMC and CPU. It features 16KB RAM that are both
> > accessible by the BMC and core CPU. and supports interrupt for
> > both sides.
> >
> > This patch implements the BMC side of the PCI mailbox module.
> > Communication with the core CPU is emulated via a chardev and
> > will be in a follow-up patch.
> >
> > Reviewed-by: Patrick Venture
> > Reviewed-by: Joe Komlodi
>
> Hi; this mostly looks good, but I have a question about s->content.
>
> > +static void npcm7xx_pci_mbox_init(Object *obj)
> > +{
> > +NPCM7xxPCIMBoxState *s = NPCM7XX_PCI_MBOX(obj);
> > +SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> > +
> > +memory_region_init_ram_device_ptr(>ram, obj, "pci-mbox-ram",
> > + NPCM7XX_PCI_MBOX_RAM_SIZE,
> s->content);
>
> What's s->content for? Nothing in the rest of the device emulation
> touches it, which seems odd.
>
Hao informed me that we can drop the content bit here, since it's not used
until a later patch that we'll be sending up with a bit more detail when we
get a chance. I sent this series up to land some of the groundwork.
I can send out a v2 with that bit removed.
>
> memory_region_init_ram_device_ptr() is intended primarily
> for "create a MemoryRegion corresponding to something like
> a bit of a host device (eg a host PCI MMIO BAR). That doesn't
> seem like what you're doing here. In particular, using it
> means that you take on responsibility for ensuring that the
> underlying memory gets migrated, which you're not doing.
>
> If you just want a MemoryRegion that's backed by a bit of
> host memory, use memory_region_init_ram().
>
I think this is what we use it for in the future patches, when we add it
back, it'll come with the context.
>
> > +#define TYPE_NPCM7XX_PCI_MBOX "npcm7xx-pci-mbox"
> > +#define NPCM7XX_PCI_MBOX(obj) \
> > +OBJECT_CHECK(NPCM7xxPCIMBoxState, (obj), TYPE_NPCM7XX_PCI_MBOX)
>
> We prefer the OBJECT_DECLARE_SIMPLE_TYPE() macro rather than
> hand-defining a cast macro these days.
>
Ack.
>
> thanks
> -- PMM
>
Thanks!
[PATCH v3 0/2] linux-user: check read permissions before how
In sigprocmask check the read permissions first before checking the `how`. This is done for both: TARGET_NR_sigprocmask and TARGET_NR_rt_sigprocmask v3: * Added missing signature from venture v2: * Update code style during code change * Also update check order for TARGET_NR_sigprocmask Patrick Venture (1): linux-user: sigprocmask check read perms first Shu-Chun Weng (1): linux-user: rt_sigprocmask, check read perms first linux-user/syscall.c | 24 ++-- 1 file changed, 14 insertions(+), 10 deletions(-) -- 2.35.0.rc0.227.g00780c9af4-goog
[PATCH v3 1/2] linux-user: rt_sigprocmask, check read perms first
From: Shu-Chun Weng
Linux kernel does it this way (checks read permission before validating `how`)
and the latest version of ABSL's `AddressIsReadable()` depends on this
behavior.
c.f.
https://github.com/torvalds/linux/blob/9539ba4308ad5bdca6cb41c7b73cbb9f796dcdd7/kernel/signal.c#L3147
Reviewed-by: Patrick Venture
Signed-off-by: Shu-Chun Weng
Reviewed-by: Laurent Vivier
Signed-off-by: Patrick Venture
---
linux-user/syscall.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 5950222a77..34bd819e38 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9508,6 +9508,13 @@ static abi_long do_syscall1(void *cpu_env, int num,
abi_long arg1,
}
if (arg2) {
+p = lock_user(VERIFY_READ, arg2, sizeof(target_sigset_t), 1);
+if (!p) {
+return -TARGET_EFAULT;
+}
+target_to_host_sigset(, p);
+unlock_user(p, arg2, 0);
+set_ptr =
switch(how) {
case TARGET_SIG_BLOCK:
how = SIG_BLOCK;
@@ -9521,11 +9528,6 @@ static abi_long do_syscall1(void *cpu_env, int num,
abi_long arg1,
default:
return -TARGET_EINVAL;
}
-if (!(p = lock_user(VERIFY_READ, arg2,
sizeof(target_sigset_t), 1)))
-return -TARGET_EFAULT;
-target_to_host_sigset(, p);
-unlock_user(p, arg2, 0);
-set_ptr =
} else {
how = 0;
set_ptr = NULL;
--
2.35.0.rc0.227.g00780c9af4-goog
[PATCH v3 2/2] linux-user: sigprocmask check read perms first
Linux kernel now checks the read permissions before validating `how`
Suggested-by: Laurent Vivier
Signed-off-by: Patrick Venture
Reviewed-by: Laurent Vivier
---
linux-user/syscall.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 34bd819e38..210483d4e4 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9465,6 +9465,13 @@ static abi_long do_syscall1(void *cpu_env, int num,
abi_long arg1,
int how;
if (arg2) {
+p = lock_user(VERIFY_READ, arg2, sizeof(target_sigset_t), 1));
+if (!p) {
+return -TARGET_EFAULT;
+}
+target_to_host_old_sigset(, p);
+unlock_user(p, arg2, 0);
+set_ptr =
switch (arg1) {
case TARGET_SIG_BLOCK:
how = SIG_BLOCK;
@@ -9478,11 +9485,6 @@ static abi_long do_syscall1(void *cpu_env, int num,
abi_long arg1,
default:
return -TARGET_EINVAL;
}
-if (!(p = lock_user(VERIFY_READ, arg2,
sizeof(target_sigset_t), 1)))
-return -TARGET_EFAULT;
-target_to_host_old_sigset(, p);
-unlock_user(p, arg2, 0);
-set_ptr =
} else {
how = 0;
set_ptr = NULL;
--
2.35.0.rc0.227.g00780c9af4-goog
Re: [PATCH v2 1/2] linux-user: rt_sigprocmask, check read perms first
On Wed, Jan 26, 2022 at 11:26 AM Laurent Vivier wrote:
> Le 26/01/2022 à 18:58, Patrick Venture a écrit :
> > From: Shu-Chun Weng
> >
> > Linux kernel does it this way (checks read permission before validating
> `how`)
> > and the latest version of ABSL's `AddressIsReadable()` depends on this
> > behavior.
> >
> > c.f.
> https://github.com/torvalds/linux/blob/9539ba4308ad5bdca6cb41c7b73cbb9f796dcdd7/kernel/signal.c#L3147
> > Reviewed-by: Patrick Venture
> > Signed-off-by: Shu-Chun Weng
>
> Reviewed-by: Laurent Vivier
>
> but you must resend the patch: you are not the author, but you have to add
> your Signed-off-by.
> (and now you can add my reviewed-by)
>
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n296
Thanks! I definitely forgot to sign the patches I wasn't the author -- but
you're right and thanks for pointing me to the guide.
>
>
> Thanks,
> Laurent
>
> > ---
> > linux-user/syscall.c | 12 +++-
> > 1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 5950222a77..34bd819e38 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -9508,6 +9508,13 @@ static abi_long do_syscall1(void *cpu_env, int
> num, abi_long arg1,
> > }
> >
> > if (arg2) {
> > +p = lock_user(VERIFY_READ, arg2,
> sizeof(target_sigset_t), 1);
> > +if (!p) {
> > +return -TARGET_EFAULT;
> > +}
> > +target_to_host_sigset(, p);
> > +unlock_user(p, arg2, 0);
> > +set_ptr =
> > switch(how) {
> > case TARGET_SIG_BLOCK:
> > how = SIG_BLOCK;
> > @@ -9521,11 +9528,6 @@ static abi_long do_syscall1(void *cpu_env, int
> num, abi_long arg1,
> > default:
> > return -TARGET_EINVAL;
> > }
> > -if (!(p = lock_user(VERIFY_READ, arg2,
> sizeof(target_sigset_t), 1)))
> > -return -TARGET_EFAULT;
> > -target_to_host_sigset(, p);
> > -unlock_user(p, arg2, 0);
> > -set_ptr =
> > } else {
> > how = 0;
> > set_ptr = NULL;
>
>
[PATCH v2 2/2] linux-user: sigprocmask check read perms first
Linux kernel now checks the read permissions before validating `how`
Suggested-by: Laurent Vivier
Signed-off-by: Patrick Venture
---
linux-user/syscall.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 34bd819e38..210483d4e4 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9465,6 +9465,13 @@ static abi_long do_syscall1(void *cpu_env, int num,
abi_long arg1,
int how;
if (arg2) {
+p = lock_user(VERIFY_READ, arg2, sizeof(target_sigset_t), 1));
+if (!p) {
+return -TARGET_EFAULT;
+}
+target_to_host_old_sigset(, p);
+unlock_user(p, arg2, 0);
+set_ptr =
switch (arg1) {
case TARGET_SIG_BLOCK:
how = SIG_BLOCK;
@@ -9478,11 +9485,6 @@ static abi_long do_syscall1(void *cpu_env, int num,
abi_long arg1,
default:
return -TARGET_EINVAL;
}
-if (!(p = lock_user(VERIFY_READ, arg2,
sizeof(target_sigset_t), 1)))
-return -TARGET_EFAULT;
-target_to_host_old_sigset(, p);
-unlock_user(p, arg2, 0);
-set_ptr =
} else {
how = 0;
set_ptr = NULL;
--
2.35.0.rc0.227.g00780c9af4-goog
[PATCH v2 1/2] linux-user: rt_sigprocmask, check read perms first
From: Shu-Chun Weng
Linux kernel does it this way (checks read permission before validating `how`)
and the latest version of ABSL's `AddressIsReadable()` depends on this
behavior.
c.f.
https://github.com/torvalds/linux/blob/9539ba4308ad5bdca6cb41c7b73cbb9f796dcdd7/kernel/signal.c#L3147
Reviewed-by: Patrick Venture
Signed-off-by: Shu-Chun Weng
---
linux-user/syscall.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 5950222a77..34bd819e38 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9508,6 +9508,13 @@ static abi_long do_syscall1(void *cpu_env, int num,
abi_long arg1,
}
if (arg2) {
+p = lock_user(VERIFY_READ, arg2, sizeof(target_sigset_t), 1);
+if (!p) {
+return -TARGET_EFAULT;
+}
+target_to_host_sigset(, p);
+unlock_user(p, arg2, 0);
+set_ptr =
switch(how) {
case TARGET_SIG_BLOCK:
how = SIG_BLOCK;
@@ -9521,11 +9528,6 @@ static abi_long do_syscall1(void *cpu_env, int num,
abi_long arg1,
default:
return -TARGET_EINVAL;
}
-if (!(p = lock_user(VERIFY_READ, arg2,
sizeof(target_sigset_t), 1)))
-return -TARGET_EFAULT;
-target_to_host_sigset(, p);
-unlock_user(p, arg2, 0);
-set_ptr =
} else {
how = 0;
set_ptr = NULL;
--
2.35.0.rc0.227.g00780c9af4-goog
[PATCH v2 0/2] linux-user: check read permissions before how
In sigprocmask check the read permissions first before checking the `how`. This is done for both: TARGET_NR_sigprocmask and TARGET_NR_rt_sigprocmask v2: * Update code style during code change * Also update check order for TARGET_NR_sigprocmask Patrick Venture (1): linux-user: sigprocmask check read perms first Shu-Chun Weng (1): linux-user: rt_sigprocmask, check read perms first linux-user/syscall.c | 24 ++-- 1 file changed, 14 insertions(+), 10 deletions(-) -- 2.35.0.rc0.227.g00780c9af4-goog
Re: [PATCH] linux-user: rt_sigprocmask, check read perms first
On Tue, Jan 11, 2022 at 3:06 PM Patrick Venture wrote:
>
>
> On Tue, Jan 11, 2022 at 12:50 PM Laurent Vivier wrote:
>
>> Hi Patrick,
>>
>> Le 11/01/2022 à 21:14, Patrick Venture a écrit :
>> >
>> >
>> > On Sat, Jan 8, 2022 at 10:16 AM Laurent Vivier > <mailto:laur...@vivier.eu>> wrote:
>> >
>> > Le 06/01/2022 à 23:00, Patrick Venture a écrit :
>> > > From: Shu-Chun Weng mailto:s...@google.com>>
>> > >
>> > > Linux kernel does it this way (checks read permission before
>> validating `how`)
>> > > and the latest version of ABSL's `AddressIsReadable()` depends
>> on this
>> > > behavior.
>> > >
>> > > c.f.
>> >
>> https://github.com/torvalds/linux/blob/9539ba4308ad5bdca6cb41c7b73cbb9f796dcdd7/kernel/signal.c#L3147
>> > <
>> https://github.com/torvalds/linux/blob/9539ba4308ad5bdca6cb41c7b73cbb9f796dcdd7/kernel/signal.c#L3147
>> >
>> > > Reviewed-by: Patrick Venture > vent...@google.com>>
>> > > Signed-off-by: Shu-Chun Weng > s...@google.com>>
>> > > ---
>> > > linux-user/syscall.c | 10 +-
>> > > 1 file changed, 5 insertions(+), 5 deletions(-)
>> > >
>> > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> > > index ce9d64896c..3070d31f34 100644
>> > > --- a/linux-user/syscall.c
>> > > +++ b/linux-user/syscall.c
>> > > @@ -9491,6 +9491,11 @@ static abi_long do_syscall1(void
>> *cpu_env, int num, abi_long arg1,
>> > > }
>> > >
>> > > if (arg2) {
>> > > +if (!(p = lock_user(VERIFY_READ, arg2,
>> sizeof(target_sigset_t), 1)))
>> > > +return -TARGET_EFAULT;
>> > > +target_to_host_sigset(, p);
>> > > +unlock_user(p, arg2, 0);
>> > > +set_ptr =
>> > > switch(how) {
>> > > case TARGET_SIG_BLOCK:
>> > > how = SIG_BLOCK;
>> > > @@ -9504,11 +9509,6 @@ static abi_long do_syscall1(void
>> *cpu_env, int num, abi_long arg1,
>> > > default:
>> > > return -TARGET_EINVAL;
>> > > }
>> > > -if (!(p = lock_user(VERIFY_READ, arg2,
>> sizeof(target_sigset_t), 1)))
>> > > -return -TARGET_EFAULT;
>> > > -target_to_host_sigset(, p);
>> > > -unlock_user(p, arg2, 0);
>> > > -set_ptr =
>> > > } else {
>> > > how = 0;
>> > > set_ptr = NULL;
>> >
>> > I know it's only code move but generally we also update the style
>> to pass scripts/checkpatch.pl
>> > <http://checkpatch.pl>
>> > successfully.
>> >
>> >
>> > That is a reasonable request, however, can I just send a follow-on
>> patch? I didn't write this one
>> > and I honestly don't know much about it, but I don't mind doing the
>> cleanup
>> >
>> >
>> > Could you also update TARGET_NR_sigprocmask in the same way as it
>> seems the kernel behaves like
>> > this
>> > too in this case?
>> >
>> >
>> > I can take a look. I would prefer then to also prefetch the style
>> fixup in a preceding patch. I
>> > don't recall seeing whether qemu supports clang-format.
>> >
>>
>> There is no problem. You can keep this patch unmodified, and add patches
>> to fix the problems.
>>
>> I only ask to have all the patches in one series.
>>
>
> Will take a swing at this for v2.
>
Laurent,
I spent some time today going over the patches and digging into what
they're actually doing and I think I can make this two patches, both with
the style changes squashed and will send in a v2 today.
Thanks
>
>
>>
>> Thanks,
>> Laurent
>>
>>
Re: [PATCH v2] hw/smbios: Add table 4 parameter, "processor-id"
On Tue, Jan 25, 2022 at 5:35 AM Igor Mammedov wrote:
> On Mon, 24 Jan 2022 12:11:51 -0800
> Patrick Venture wrote:
>
> > This parameter is to be used in the processor_id lower 32-bit entry in
> > the type 4 table. The upper 32-bits represent the features for the CPU.
> > This patch leaves those as 0 when the lower 32-bits are set.
>
> Did you forget to update commit message ?
>
I did. Oops :) I will send a quick v3 with that fix.
>
> Other than that patch looks good to me,
> so with commit message fixed:
>
> Reviewed-by: Igor Mammedov
>
> > This parameter is set as optional and if left will use the values from
> > the CPU model.
> >
> > This enables hiding the host information from the guest and allowing AMD
> > VMs to run pretending to be Intel for some userspace software concerns.
> >
> > Reviewed-by: Peter Foley
> > Reviewed-by: Titus Rwantare
> > Signed-off-by: Patrick Venture
> > ---
> > v2: Added to SRST options list, upgraded to full 64-bit value.
> > ---
> > hw/smbios/smbios.c | 19 ---
> > qemu-options.hx| 3 ++-
> > 2 files changed, 18 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> > index 7397e56737..385b69b0c9 100644
> > --- a/hw/smbios/smbios.c
> > +++ b/hw/smbios/smbios.c
> > @@ -104,9 +104,11 @@ static struct {
> > const char *sock_pfx, *manufacturer, *version, *serial, *asset,
> *part;
> > uint64_t max_speed;
> > uint64_t current_speed;
> > +uint64_t processor_id;
> > } type4 = {
> > .max_speed = DEFAULT_CPU_SPEED,
> > -.current_speed = DEFAULT_CPU_SPEED
> > +.current_speed = DEFAULT_CPU_SPEED,
> > +.processor_id = 0,
> > };
> >
> > static struct {
> > @@ -327,6 +329,10 @@ static const QemuOptDesc qemu_smbios_type4_opts[] =
> {
> > .name = "part",
> > .type = QEMU_OPT_STRING,
> > .help = "part number",
> > +}, {
> > +.name = "processor-id",
> > +.type = QEMU_OPT_NUMBER,
> > +.help = "processor id",
> > },
> > { /* end of list */ }
> > };
> > @@ -669,8 +675,13 @@ static void smbios_build_type_4_table(MachineState
> *ms, unsigned instance)
> > t->processor_type = 0x03; /* CPU */
> > t->processor_family = 0x01; /* Other */
> > SMBIOS_TABLE_SET_STR(4, processor_manufacturer_str,
> type4.manufacturer);
> > -t->processor_id[0] = cpu_to_le32(smbios_cpuid_version);
> > -t->processor_id[1] = cpu_to_le32(smbios_cpuid_features);
> > +if (type4.processor_id == 0) {
> > +t->processor_id[0] = cpu_to_le32(smbios_cpuid_version);
> > +t->processor_id[1] = cpu_to_le32(smbios_cpuid_features);
> > +} else {
> > +t->processor_id[0] = cpu_to_le32((uint32_t)type4.processor_id);
> > +t->processor_id[1] = cpu_to_le32(type4.processor_id >> 32);
> > +}
> > SMBIOS_TABLE_SET_STR(4, processor_version_str, type4.version);
> > t->voltage = 0;
> > t->external_clock = cpu_to_le16(0); /* Unknown */
> > @@ -1292,6 +1303,8 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> > save_opt(, opts, "serial");
> > save_opt(, opts, "asset");
> > save_opt(, opts, "part");
> > +/* If the value is 0, it will take the value from the CPU
> model. */
> > +type4.processor_id = qemu_opt_get_number(opts,
> "processor-id", 0);
> > type4.max_speed = qemu_opt_get_number(opts, "max-speed",
> >DEFAULT_CPU_SPEED);
> > type4.current_speed = qemu_opt_get_number(opts,
> "current-speed",
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index ec90505d84..6256417935 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -2527,6 +2527,7 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
> > "specify SMBIOS type 3 fields\n"
> > "-smbios
> type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str]\n"
> > "
> [,asset=str][,part=str][,max-speed=%d][,current-speed=%d]\n"
> > +" [,processor-id=%d]\n"
> > "specify SMBIOS type 4 fields\n"
> > "-smbios type=11[,value=str][,path=filename]\n"
> > "specify SMBIOS type 11 fields\n"
> > @@ -2552,7 +2553,7 @@ SRST
> > ``-smbios
> type=3[,manufacturer=str][,version=str][,serial=str][,asset=str][,sku=str]``
> > Specify SMBIOS type 3 fields
> >
> > -``-smbios
> type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str][,asset=str][,part=str]``
> > +``-smbios
> type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str][,asset=str][,part=str][,processor-id=%d]``
> > Specify SMBIOS type 4 fields
> >
> > ``-smbios type=11[,value=str][,path=filename]``
>
>
[PATCH v3] hw/smbios: Add table 4 parameter, "processor-id"
This parameter is to be used in the processor_id entry in the type 4
table.
This parameter is set as optional and if left will use the values from
the CPU model.
This enables hiding the host information from the guest and allowing AMD
VMs to run pretending to be Intel for some userspace software concerns.
Reviewed-by: Peter Foley
Reviewed-by: Titus Rwantare
Signed-off-by: Patrick Venture
Reviewed-by: Igor Mammedov
---
v3: updated commit message to match v2
v2: Added to SRST options list, upgraded to full 64-bit value.
---
hw/smbios/smbios.c | 19 ---
qemu-options.hx| 3 ++-
2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 7397e56737..385b69b0c9 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -104,9 +104,11 @@ static struct {
const char *sock_pfx, *manufacturer, *version, *serial, *asset, *part;
uint64_t max_speed;
uint64_t current_speed;
+uint64_t processor_id;
} type4 = {
.max_speed = DEFAULT_CPU_SPEED,
-.current_speed = DEFAULT_CPU_SPEED
+.current_speed = DEFAULT_CPU_SPEED,
+.processor_id = 0,
};
static struct {
@@ -327,6 +329,10 @@ static const QemuOptDesc qemu_smbios_type4_opts[] = {
.name = "part",
.type = QEMU_OPT_STRING,
.help = "part number",
+}, {
+.name = "processor-id",
+.type = QEMU_OPT_NUMBER,
+.help = "processor id",
},
{ /* end of list */ }
};
@@ -669,8 +675,13 @@ static void smbios_build_type_4_table(MachineState *ms,
unsigned instance)
t->processor_type = 0x03; /* CPU */
t->processor_family = 0x01; /* Other */
SMBIOS_TABLE_SET_STR(4, processor_manufacturer_str, type4.manufacturer);
-t->processor_id[0] = cpu_to_le32(smbios_cpuid_version);
-t->processor_id[1] = cpu_to_le32(smbios_cpuid_features);
+if (type4.processor_id == 0) {
+t->processor_id[0] = cpu_to_le32(smbios_cpuid_version);
+t->processor_id[1] = cpu_to_le32(smbios_cpuid_features);
+} else {
+t->processor_id[0] = cpu_to_le32((uint32_t)type4.processor_id);
+t->processor_id[1] = cpu_to_le32(type4.processor_id >> 32);
+}
SMBIOS_TABLE_SET_STR(4, processor_version_str, type4.version);
t->voltage = 0;
t->external_clock = cpu_to_le16(0); /* Unknown */
@@ -1292,6 +1303,8 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
save_opt(, opts, "serial");
save_opt(, opts, "asset");
save_opt(, opts, "part");
+/* If the value is 0, it will take the value from the CPU model. */
+type4.processor_id = qemu_opt_get_number(opts, "processor-id", 0);
type4.max_speed = qemu_opt_get_number(opts, "max-speed",
DEFAULT_CPU_SPEED);
type4.current_speed = qemu_opt_get_number(opts, "current-speed",
diff --git a/qemu-options.hx b/qemu-options.hx
index ec90505d84..6256417935 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2527,6 +2527,7 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
"specify SMBIOS type 3 fields\n"
"-smbios
type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str]\n"
" [,asset=str][,part=str][,max-speed=%d][,current-speed=%d]\n"
+" [,processor-id=%d]\n"
"specify SMBIOS type 4 fields\n"
"-smbios type=11[,value=str][,path=filename]\n"
"specify SMBIOS type 11 fields\n"
@@ -2552,7 +2553,7 @@ SRST
``-smbios
type=3[,manufacturer=str][,version=str][,serial=str][,asset=str][,sku=str]``
Specify SMBIOS type 3 fields
-``-smbios
type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str][,asset=str][,part=str]``
+``-smbios
type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str][,asset=str][,part=str][,processor-id=%d]``
Specify SMBIOS type 4 fields
``-smbios type=11[,value=str][,path=filename]``
--
2.35.0.rc0.227.g00780c9af4-goog
[PATCH v2] hw/smbios: Add table 4 parameter, "processor-id"
This parameter is to be used in the processor_id lower 32-bit entry in
the type 4 table. The upper 32-bits represent the features for the CPU.
This patch leaves those as 0 when the lower 32-bits are set.
This parameter is set as optional and if left will use the values from
the CPU model.
This enables hiding the host information from the guest and allowing AMD
VMs to run pretending to be Intel for some userspace software concerns.
Reviewed-by: Peter Foley
Reviewed-by: Titus Rwantare
Signed-off-by: Patrick Venture
---
v2: Added to SRST options list, upgraded to full 64-bit value.
---
hw/smbios/smbios.c | 19 ---
qemu-options.hx| 3 ++-
2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 7397e56737..385b69b0c9 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -104,9 +104,11 @@ static struct {
const char *sock_pfx, *manufacturer, *version, *serial, *asset, *part;
uint64_t max_speed;
uint64_t current_speed;
+uint64_t processor_id;
} type4 = {
.max_speed = DEFAULT_CPU_SPEED,
-.current_speed = DEFAULT_CPU_SPEED
+.current_speed = DEFAULT_CPU_SPEED,
+.processor_id = 0,
};
static struct {
@@ -327,6 +329,10 @@ static const QemuOptDesc qemu_smbios_type4_opts[] = {
.name = "part",
.type = QEMU_OPT_STRING,
.help = "part number",
+}, {
+.name = "processor-id",
+.type = QEMU_OPT_NUMBER,
+.help = "processor id",
},
{ /* end of list */ }
};
@@ -669,8 +675,13 @@ static void smbios_build_type_4_table(MachineState *ms,
unsigned instance)
t->processor_type = 0x03; /* CPU */
t->processor_family = 0x01; /* Other */
SMBIOS_TABLE_SET_STR(4, processor_manufacturer_str, type4.manufacturer);
-t->processor_id[0] = cpu_to_le32(smbios_cpuid_version);
-t->processor_id[1] = cpu_to_le32(smbios_cpuid_features);
+if (type4.processor_id == 0) {
+t->processor_id[0] = cpu_to_le32(smbios_cpuid_version);
+t->processor_id[1] = cpu_to_le32(smbios_cpuid_features);
+} else {
+t->processor_id[0] = cpu_to_le32((uint32_t)type4.processor_id);
+t->processor_id[1] = cpu_to_le32(type4.processor_id >> 32);
+}
SMBIOS_TABLE_SET_STR(4, processor_version_str, type4.version);
t->voltage = 0;
t->external_clock = cpu_to_le16(0); /* Unknown */
@@ -1292,6 +1303,8 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
save_opt(, opts, "serial");
save_opt(, opts, "asset");
save_opt(, opts, "part");
+/* If the value is 0, it will take the value from the CPU model. */
+type4.processor_id = qemu_opt_get_number(opts, "processor-id", 0);
type4.max_speed = qemu_opt_get_number(opts, "max-speed",
DEFAULT_CPU_SPEED);
type4.current_speed = qemu_opt_get_number(opts, "current-speed",
diff --git a/qemu-options.hx b/qemu-options.hx
index ec90505d84..6256417935 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2527,6 +2527,7 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
"specify SMBIOS type 3 fields\n"
"-smbios
type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str]\n"
" [,asset=str][,part=str][,max-speed=%d][,current-speed=%d]\n"
+" [,processor-id=%d]\n"
"specify SMBIOS type 4 fields\n"
"-smbios type=11[,value=str][,path=filename]\n"
"specify SMBIOS type 11 fields\n"
@@ -2552,7 +2553,7 @@ SRST
``-smbios
type=3[,manufacturer=str][,version=str][,serial=str][,asset=str][,sku=str]``
Specify SMBIOS type 3 fields
-``-smbios
type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str][,asset=str][,part=str]``
+``-smbios
type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str][,asset=str][,part=str][,processor-id=%d]``
Specify SMBIOS type 4 fields
``-smbios type=11[,value=str][,path=filename]``
--
2.35.0.rc0.227.g00780c9af4-goog
Re: [PATCH] hw/smbios: Add table 4 parameter, "processor-id"
On Tue, Jan 18, 2022 at 11:37 PM Igor Mammedov wrote:
> On Tue, 18 Jan 2022 09:15:42 -0800
> Patrick Venture wrote:
>
> > On Tue, Jan 11, 2022 at 5:13 AM Igor Mammedov
> wrote:
> >
> > > On Thu, 6 Jan 2022 14:33:16 -0800
> > > Patrick Venture wrote:
> > >
> [...]
> > > > diff --git a/qemu-options.hx b/qemu-options.hx
> > > > index ec90505d84..3c51b6cf8f 100644
> > > > --- a/qemu-options.hx
> > > > +++ b/qemu-options.hx
> > > > @@ -2527,6 +2527,7 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
> > > > "specify SMBIOS type 3 fields\n"
> > > > "-smbios
> > >
> type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str]\n"
> > > > "
> > > [,asset=str][,part=str][,max-speed=%d][,current-speed=%d]\n"
> > > > +" [,processor-id=%d]\n"
> > > > "specify SMBIOS type 4 fields\n"
> > > > "-smbios type=11[,value=str][,path=filename]\n"
> > > > "specify SMBIOS type 11 fields\n"
> > >
> > > missing update of SRST part
> > >
> >
> > I grepped for SRST, where is this that I need to update also?
>
> option definition has 2 parts DEF() and SRST that follows right
> after it, the later is used as help text for the option
> SRST
Ahh, yeah, I grepped in the wrong place :)
Working on validating v2 presently then sending out.
>
> ``-smbios file=binary``
> ...
>
> >
> > Thanks!
>
Thanks!
Re: [PATCH] hw/nvram: use at24 macro
On Wed, Jan 19, 2022 at 1:43 PM Patrick Venture wrote:
> Use the macro for going from I2CSlave to EEPROMState.
>
> Signed-off-by: Patrick Venture
> ---
> hw/nvram/eeprom_at24c.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
> index af6f5dbb99..da435500ba 100644
> --- a/hw/nvram/eeprom_at24c.c
> +++ b/hw/nvram/eeprom_at24c.c
> @@ -54,7 +54,7 @@ struct EEPROMState {
> static
> int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
> {
> -EEPROMState *ee = container_of(s, EEPROMState, parent_obj);
> +EEPROMState *ee = AT24C_EE(s);
>
> switch (event) {
> case I2C_START_SEND:
> --
> 2.34.1.703.g22d0c6ccf7-goog
>
+Corey - thanks!
[PATCH] hw/nvram: use at24 macro
Use the macro for going from I2CSlave to EEPROMState.
Signed-off-by: Patrick Venture
---
hw/nvram/eeprom_at24c.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index af6f5dbb99..da435500ba 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -54,7 +54,7 @@ struct EEPROMState {
static
int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
{
-EEPROMState *ee = container_of(s, EEPROMState, parent_obj);
+EEPROMState *ee = AT24C_EE(s);
switch (event) {
case I2C_START_SEND:
--
2.34.1.703.g22d0c6ccf7-goog
Re: [PATCH] hw/smbios: Add table 4 parameter, "processor-id"
On Tue, Jan 11, 2022 at 5:13 AM Igor Mammedov wrote:
> On Thu, 6 Jan 2022 14:33:16 -0800
> Patrick Venture wrote:
>
> > This parameter is to be used in the processor_id lower 32-bit entry in
>
> I'd call it processor_id_lo throughout the patch
> or if you prefer to keep current name, then make it support full QWORD
> as spec says.
>
Ack
>
> > the type 4 table. The upper 32-bits represent the features for the CPU.
> > This patch leaves those as 0 when the lower 32-bits are set.
>
> why 0 it out instead of using smbios_cpuid_features value?
>
Because presumably the cpuid_feature value would not match whatever
override was chosen here. But I like your idea of just making it a
quadword.
>
>
> > This parameter is set as optional and if left will use the values from
> > the CPU model.
> >
> > This enables hiding the host information from the guest and allowing AMD
> > VMs to run pretending to be Intel for some userspace software concerns.
>
> > Reviewed-by: Peter Foley
> > Reviewed-by: Titus Rwantare
> > Signed-off-by: Patrick Venture
> > ---
> > hw/smbios/smbios.c | 19 ---
> > qemu-options.hx| 1 +
> > 2 files changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> > index 7397e56737..0553ee0b17 100644
> > --- a/hw/smbios/smbios.c
> > +++ b/hw/smbios/smbios.c
> > @@ -104,9 +104,11 @@ static struct {
> > const char *sock_pfx, *manufacturer, *version, *serial, *asset,
> *part;
> > uint64_t max_speed;
> > uint64_t current_speed;
> > +uint32_t processor_id;
>
>
>
> > } type4 = {
> > .max_speed = DEFAULT_CPU_SPEED,
> > -.current_speed = DEFAULT_CPU_SPEED
> > +.current_speed = DEFAULT_CPU_SPEED,
> > +.processor_id = 0,
> > };
> >
> > static struct {
> > @@ -327,6 +329,10 @@ static const QemuOptDesc qemu_smbios_type4_opts[] =
> {
> > .name = "part",
> > .type = QEMU_OPT_STRING,
> > .help = "part number",
> > +}, {
> > +.name = "processor-id",
> > +.type = QEMU_OPT_NUMBER,
> > +.help = "processor id",
> > },
> > { /* end of list */ }
> > };
> > @@ -669,8 +675,13 @@ static void smbios_build_type_4_table(MachineState
> *ms, unsigned instance)
> > t->processor_type = 0x03; /* CPU */
> > t->processor_family = 0x01; /* Other */
> > SMBIOS_TABLE_SET_STR(4, processor_manufacturer_str,
> type4.manufacturer);
> > -t->processor_id[0] = cpu_to_le32(smbios_cpuid_version);
> > -t->processor_id[1] = cpu_to_le32(smbios_cpuid_features);
> > +if (type4.processor_id == 0) {
> > +t->processor_id[0] = cpu_to_le32(smbios_cpuid_version);
> > +t->processor_id[1] = cpu_to_le32(smbios_cpuid_features);
> > +} else {
> > +t->processor_id[0] = cpu_to_le32(type4.processor_id);
> > +t->processor_id[1] = 0;
> > +}
> > SMBIOS_TABLE_SET_STR(4, processor_version_str, type4.version);
> > t->voltage = 0;
> > t->external_clock = cpu_to_le16(0); /* Unknown */
> > @@ -1292,6 +1303,8 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> > save_opt(, opts, "serial");
> > save_opt(, opts, "asset");
> > save_opt(, opts, "part");
> > +/* If the value is 0, it will take the value from the CPU
> model. */
> > +type4.processor_id = qemu_opt_get_number(opts,
> "processor-id", 0);
> > type4.max_speed = qemu_opt_get_number(opts, "max-speed",
> >DEFAULT_CPU_SPEED);
> > type4.current_speed = qemu_opt_get_number(opts,
> "current-speed",
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index ec90505d84..3c51b6cf8f 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -2527,6 +2527,7 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
> > "specify SMBIOS type 3 fields\n"
> > "-smbios
> type=4[,sock_pfx=str][,manufacturer=str][,version=str][,serial=str]\n"
> > "
> [,asset=str][,part=str][,max-speed=%d][,current-speed=%d]\n"
> > +" [,processor-id=%d]\n"
> > "specify SMBIOS type 4 fields\n"
> > "-smbios type=11[,value=str][,path=filename]\n"
> > "specify SMBIOS type 11 fields\n"
>
> missing update of SRST part
>
I grepped for SRST, where is this that I need to update also?
Thanks!
Re: [PATCH] hw/sensor: Add SB-TSI Temperature Sensor Interface
On Mon, Jan 17, 2022 at 6:05 AM Corey Minyard wrote:
> On Sun, Jan 09, 2022 at 06:17:34PM -0800, Patrick Venture wrote:
> > On Fri, Jan 7, 2022 at 7:04 PM Patrick Venture
> wrote:
> >
> > > From: Hao Wu
> > >
> > > SB Temperature Sensor Interface (SB-TSI) is an SMBus compatible
> > > interface that reports AMD SoC's Ttcl (normalized temperature),
> > > and resembles a typical 8-pin remote temperature sensor's I2C interface
> > > to BMC.
> > >
> > > This patch implements a basic AMD SB-TSI sensor that is
> > > compatible with the open-source data sheet from AMD and Linux
> > > kernel driver.
> > >
> > > Reference:
> > > Linux kernel driver:
> > > https://lkml.org/lkml/2020/12/11/968
> > > Register Map:
> > > https://developer.amd.com/wp-content/resources/56255_3_03.PDF
> > > (Chapter 6)
> > >
> > > Signed-off-by: Hao Wu
> > > Reviewed-by: Doug Evans
> > > ---
> > > hw/sensor/Kconfig| 4 +
> > > hw/sensor/meson.build| 1 +
> > > hw/sensor/tmp_sbtsi.c| 393 +++
> > > hw/sensor/trace-events | 5 +
> > > hw/sensor/trace.h| 1 +
> > > meson.build | 1 +
> > > tests/qtest/meson.build | 1 +
> > > tests/qtest/tmp_sbtsi-test.c | 180
> > > 8 files changed, 586 insertions(+)
> > > create mode 100644 hw/sensor/tmp_sbtsi.c
> > > create mode 100644 hw/sensor/trace-events
> > > create mode 100644 hw/sensor/trace.h
> > > create mode 100644 tests/qtest/tmp_sbtsi-test.c
> > >
> > > diff --git a/hw/sensor/Kconfig b/hw/sensor/Kconfig
> > > index 9c8a049b06..27f6f79c84 100644
> > > --- a/hw/sensor/Kconfig
> > > +++ b/hw/sensor/Kconfig
> > > @@ -21,3 +21,7 @@ config ADM1272
> > > config MAX34451
> > > bool
> > > depends on I2C
> > > +
> > > +config AMDSBTSI
> > > +bool
> > > +depends on I2C
> > > diff --git a/hw/sensor/meson.build b/hw/sensor/meson.build
> > > index 059c4ca935..f7b0e645eb 100644
> > > --- a/hw/sensor/meson.build
> > > +++ b/hw/sensor/meson.build
> > > @@ -4,3 +4,4 @@ softmmu_ss.add(when: 'CONFIG_DPS310', if_true:
> > > files('dps310.c'))
> > > softmmu_ss.add(when: 'CONFIG_EMC141X', if_true: files('emc141x.c'))
> > > softmmu_ss.add(when: 'CONFIG_ADM1272', if_true: files('adm1272.c'))
> > > softmmu_ss.add(when: 'CONFIG_MAX34451', if_true: files('max34451.c'))
> > > +softmmu_ss.add(when: 'CONFIG_AMDSBTSI', if_true: files('tmp_sbtsi.c'))
> > > diff --git a/hw/sensor/tmp_sbtsi.c b/hw/sensor/tmp_sbtsi.c
> > > new file mode 100644
> > > index 00..b68c7ebf61
> > > --- /dev/null
> > > +++ b/hw/sensor/tmp_sbtsi.c
> > > @@ -0,0 +1,393 @@
> > > +/*
> > > + * AMD SBI Temperature Sensor Interface (SB-TSI)
> > > + *
> > > + * Copyright 2021 Google LLC
> > > + *
> > > + * This program is free software; you can redistribute it and/or
> modify it
> > > + * under the terms of the GNU General Public License as published by
> the
> > > + * Free Software Foundation; either version 2 of the License, or
> > > + * (at your option) any later version.
> > > + *
> > > + * This program is distributed in the hope that it will be useful, but
> > > WITHOUT
> > > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
> or
> > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
> License
> > > + * for more details.
> > > + */
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "hw/i2c/smbus_slave.h"
> > > +#include "hw/irq.h"
> > > +#include "migration/vmstate.h"
> > > +#include "qapi/error.h"
> > > +#include "qapi/visitor.h"
> > > +#include "qemu/log.h"
> > > +#include "qemu/module.h"
> > > +#include "trace.h"
> > > +
> > > +#define TYPE_SBTSI "sbtsi"
> > > +#define SBTSI(obj) OBJECT_CHECK(SBTSIState, (obj), TYPE_SBTSI)
> > > +
> > > +/**
> > > + * SBTSIState:
> > > + * temperatures are in units of 0.125 degrees
> > > + * @temperature: Temperature
> > > + * @limit_low: Lowest temperature
> > > + * @limit_high: Highest te
[PATCH v3 2/2] tests: add qtest for hw/sensor/sbtsi
From: Hao Wu
Reviewed-by: Doug Evans
Signed-off-by: Hao Wu
Signed-off-by: Patrick Venture
Acked-by: Thomas Huth
---
tests/qtest/tmp-sbtsi-test.c | 161 +++
tests/qtest/meson.build | 1 +
2 files changed, 162 insertions(+)
create mode 100644 tests/qtest/tmp-sbtsi-test.c
diff --git a/tests/qtest/tmp-sbtsi-test.c b/tests/qtest/tmp-sbtsi-test.c
new file mode 100644
index 00..ff1e193739
--- /dev/null
+++ b/tests/qtest/tmp-sbtsi-test.c
@@ -0,0 +1,161 @@
+/*
+ * QTests for the SBTSI temperature sensor
+ *
+ * Copyright 2020 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "qemu/osdep.h"
+
+#include "libqtest-single.h"
+#include "libqos/qgraph.h"
+#include "libqos/i2c.h"
+#include "qapi/qmp/qdict.h"
+#include "qemu/bitops.h"
+#include "hw/sensor/sbtsi.h"
+
+#define TEST_ID "sbtsi-test"
+#define TEST_ADDR (0x4c)
+
+/* The temperature stored are in units of 0.125 degrees. */
+#define LIMIT_LOW_IN_MILLIDEGREE (10500)
+#define LIMIT_HIGH_IN_MILLIDEGREE (55125)
+
+static uint32_t qmp_sbtsi_get_temperature(const char *id)
+{
+QDict *response;
+int ret;
+
+response = qmp("{ 'execute': 'qom-get', 'arguments': { 'path': %s, "
+ "'property': 'temperature' } }", id);
+g_assert(qdict_haskey(response, "return"));
+ret = (uint32_t)qdict_get_int(response, "return");
+qobject_unref(response);
+return ret;
+}
+
+static void qmp_sbtsi_set_temperature(const char *id, uint32_t value)
+{
+QDict *response;
+
+response = qmp("{ 'execute': 'qom-set', 'arguments': { 'path': %s, "
+ "'property': 'temperature', 'value': %d } }", id, value);
+g_assert(qdict_haskey(response, "return"));
+qobject_unref(response);
+}
+
+/*
+ * Compute the temperature using the integer and decimal part and return
+ * millidegrees. The decimal part are only the top 3 bits so we shift it by
+ * 5 here.
+ */
+static uint32_t regs_to_temp(uint8_t integer, uint8_t decimal)
+{
+return ((integer << 3) + (decimal >> 5)) * SBTSI_TEMP_UNIT_IN_MILLIDEGREE;
+}
+
+/*
+ * Compute the integer and decimal parts of the temperature in millidegrees.
+ * H/W store the decimal in the top 3 bits so we shift it by 5.
+ */
+static void temp_to_regs(uint32_t temp, uint8_t *integer, uint8_t *decimal)
+{
+temp /= SBTSI_TEMP_UNIT_IN_MILLIDEGREE;
+*integer = temp >> 3;
+*decimal = (temp & 0x7) << 5;
+}
+
+static void tx_rx(void *obj, void *data, QGuestAllocator *alloc)
+{
+uint16_t value;
+uint8_t integer, decimal;
+QI2CDevice *i2cdev = (QI2CDevice *)obj;
+
+/* Test default values */
+value = qmp_sbtsi_get_temperature(TEST_ID);
+g_assert_cmpuint(value, ==, 0);
+
+integer = i2c_get8(i2cdev, SBTSI_REG_TEMP_INT);
+decimal = i2c_get8(i2cdev, SBTSI_REG_TEMP_DEC);
+g_assert_cmpuint(regs_to_temp(integer, decimal), ==, 0);
+
+/* Test setting temperature */
+qmp_sbtsi_set_temperature(TEST_ID, 2);
+value = qmp_sbtsi_get_temperature(TEST_ID);
+g_assert_cmpuint(value, ==, 2);
+
+integer = i2c_get8(i2cdev, SBTSI_REG_TEMP_INT);
+decimal = i2c_get8(i2cdev, SBTSI_REG_TEMP_DEC);
+g_assert_cmpuint(regs_to_temp(integer, decimal), ==, 2);
+
+/* Set alert mask in config */
+i2c_set8(i2cdev, SBTSI_REG_CONFIG_WR, SBTSI_CONFIG_ALERT_MASK);
+value = i2c_get8(i2cdev, SBTSI_REG_CONFIG);
+g_assert_cmphex(value, ==, SBTSI_CONFIG_ALERT_MASK);
+/* Enable alarm_en */
+i2c_set8(i2cdev, SBTSI_REG_ALERT_CONFIG, SBTSI_ALARM_EN);
+value = i2c_get8(i2cdev, SBTSI_REG_ALERT_CONFIG);
+g_assert_cmphex(value, ==, SBTSI_ALARM_EN);
+
+/* Test setting limits */
+/* Limit low = 10.500 */
+temp_to_regs(LIMIT_LOW_IN_MILLIDEGREE, , );
+i2c_set8(i2cdev, SBTSI_REG_TEMP_LOW_INT, integer);
+i2c_set8(i2cdev, SBTSI_REG_TEMP_LOW_DEC, decimal);
+integer = i2c_get8(i2cdev, SBTSI_REG_TEMP_LOW_INT);
+decimal = i2c_get8(i2cdev, SBTSI_REG_TEMP_LOW_DEC);
+g_assert_cmpuint(
+regs_to_temp(integer, decimal), ==, LIMIT_LOW_IN_MILLIDEGREE);
+/* Limit high = 55.125 */
+temp_to_regs(LIMIT_HIGH_IN_MILLIDEGREE, , );
+i2c_set8(i2cdev, SBTSI_REG_TEMP_HIGH_INT, integer);
+i2c_set8(i2cdev, SBTSI_REG_TEMP_HIGH_DEC, decimal);
+integer = i2c_get8(i2cdev, SBTSI_REG_TEMP_HIGH_INT);
+
[PATCH v3 1/2] hw/sensor: Add SB-TSI Temperature Sensor Interface
From: Hao Wu SB Temperature Sensor Interface (SB-TSI) is an SMBus compatible interface that reports AMD SoC's Ttcl (normalized temperature), and resembles a typical 8-pin remote temperature sensor's I2C interface to BMC. This patch implements a basic AMD SB-TSI sensor that is compatible with the open-source data sheet from AMD and Linux kernel driver. Reference: Linux kernel driver: https://lkml.org/lkml/2020/12/11/968 Register Map: https://developer.amd.com/wp-content/resources/56255_3_03.PDF (Chapter 6) Signed-off-by: Hao Wu Reviewed-by: Doug Evans Reviewed-by: Philippe Mathieu-Daudé --- meson.build | 1 + hw/sensor/trace.h | 1 + include/hw/sensor/sbtsi.h | 45 + hw/sensor/tmp_sbtsi.c | 369 ++ hw/sensor/Kconfig | 4 + hw/sensor/meson.build | 1 + hw/sensor/trace-events| 5 + 7 files changed, 426 insertions(+) create mode 100644 hw/sensor/trace.h create mode 100644 include/hw/sensor/sbtsi.h create mode 100644 hw/sensor/tmp_sbtsi.c create mode 100644 hw/sensor/trace-events diff --git a/meson.build b/meson.build index c1b1db1e28..3634214546 100644 --- a/meson.build +++ b/meson.build @@ -2494,6 +2494,7 @@ if have_system 'hw/rtc', 'hw/s390x', 'hw/scsi', +'hw/sensor', 'hw/sd', 'hw/sh4', 'hw/sparc', diff --git a/hw/sensor/trace.h b/hw/sensor/trace.h new file mode 100644 index 00..e4721560b0 --- /dev/null +++ b/hw/sensor/trace.h @@ -0,0 +1 @@ +#include "trace/trace-hw_sensor.h" diff --git a/include/hw/sensor/sbtsi.h b/include/hw/sensor/sbtsi.h new file mode 100644 index 00..9073f76ebb --- /dev/null +++ b/include/hw/sensor/sbtsi.h @@ -0,0 +1,45 @@ +/* + * AMD SBI Temperature Sensor Interface (SB-TSI) + * + * Copyright 2021 Google LLC + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ +#ifndef QEMU_TMP_SBTSI_H +#define QEMU_TMP_SBTSI_H + +/* + * SB-TSI registers only support SMBus byte data access. "_INT" registers are + * the integer part of a temperature value or limit, and "_DEC" registers are + * corresponding decimal parts. + */ +#define SBTSI_REG_TEMP_INT 0x01 /* RO */ +#define SBTSI_REG_STATUS0x02 /* RO */ +#define SBTSI_REG_CONFIG0x03 /* RO */ +#define SBTSI_REG_TEMP_HIGH_INT 0x07 /* RW */ +#define SBTSI_REG_TEMP_LOW_INT 0x08 /* RW */ +#define SBTSI_REG_CONFIG_WR 0x09 /* RW */ +#define SBTSI_REG_TEMP_DEC 0x10 /* RO */ +#define SBTSI_REG_TEMP_HIGH_DEC 0x13 /* RW */ +#define SBTSI_REG_TEMP_LOW_DEC 0x14 /* RW */ +#define SBTSI_REG_ALERT_CONFIG 0xBF /* RW */ +#define SBTSI_REG_MAN 0xFE /* RO */ +#define SBTSI_REG_REV 0xFF /* RO */ + +#define SBTSI_STATUS_HIGH_ALERT BIT(4) +#define SBTSI_STATUS_LOW_ALERT BIT(3) +#define SBTSI_CONFIG_ALERT_MASK BIT(7) +#define SBTSI_ALARM_EN BIT(0) + +/* The temperature we stored are in units of 0.125 degrees. */ +#define SBTSI_TEMP_UNIT_IN_MILLIDEGREE 125 + +#endif diff --git a/hw/sensor/tmp_sbtsi.c b/hw/sensor/tmp_sbtsi.c new file mode 100644 index 00..d5406844ef --- /dev/null +++ b/hw/sensor/tmp_sbtsi.c @@ -0,0 +1,369 @@ +/* + * AMD SBI Temperature Sensor Interface (SB-TSI) + * + * Copyright 2021 Google LLC + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ + +#include "qemu/osdep.h" +#include "hw/i2c/smbus_slave.h" +#include "hw/sensor/sbtsi.h" +#include "hw/irq.h" +#include "migration/vmstate.h" +#include "qapi/error.h" +#include "qapi/visitor.h" +#include "qemu/log.h" +#include "qemu/module.h" +#include "qom/object.h" +#include "trace.h" + +#define TYPE_SBTSI "sbtsi" +#define SBTSI(obj) OBJECT_CHECK(SBTSIState, (obj), TYPE_SBTSI) + +/** + * SBTSIState: + * temperatures are in units of 0.125 degrees + * @temperature: Temperature + * @limit_low: Lowest temperature + * @limit_high: Highest temperature + * @status: The status register + * @config: The config register + * @alert_config: The config for alarm_l output. + * @addr: The address to read/write for the next cmd. + * @alarm: The alarm_l output pin (GPIO) + */ +typedef struct
[PATCH v3 0/2] hw/sensor: Add SB-TSI Temperature Sensor Interface
v3: * typofix where I accidentally embedded 'wq' into a string * moved the type sbtsi definition back into the source file * renamed the qtest file to use hyphens only v2: * Split the commit into a separate patch for the qtest * Moved the common registers into the new header * Introduced a new header Hao Wu (2): hw/sensor: Add SB-TSI Temperature Sensor Interface tests: add qtest for hw/sensor/sbtsi meson.build | 1 + hw/sensor/trace.h| 1 + include/hw/sensor/sbtsi.h| 45 + hw/sensor/tmp_sbtsi.c| 369 +++ tests/qtest/tmp-sbtsi-test.c | 161 +++ hw/sensor/Kconfig| 4 + hw/sensor/meson.build| 1 + hw/sensor/trace-events | 5 + tests/qtest/meson.build | 1 + 9 files changed, 588 insertions(+) create mode 100644 hw/sensor/trace.h create mode 100644 include/hw/sensor/sbtsi.h create mode 100644 hw/sensor/tmp_sbtsi.c create mode 100644 tests/qtest/tmp-sbtsi-test.c create mode 100644 hw/sensor/trace-events -- 2.34.1.575.g55b058a8bb-goog
