Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[David Gibson]

On Fri, Jul 05, 2024 at 02:40:19PM +1000, Nicholas Piggin wrote:
> On Fri Jul 5, 2024 at 11:41 AM AEST, David Gibson wrote:
> > On Fri, Jul 05, 2024 at 11:18:47AM +1000, Nicholas Piggin wrote:
> > > On Thu Jul 4, 2024 at 10:15 PM AEST, Peter Maydell wrote:
> > > > On Sat, 29 Jun 2024 at 04:17, David Gibson 
> > > >  wrote:
> > > > >
> > > > > On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > > > > > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki 
> > > > > >  wrote:
> > > > > > >
> > > > > > > FDT properties are aligned by 4 bytes, not 8 bytes.
> > > > > > >
> > > > > > > Signed-off-by: Akihiko Odaki 
> > > > > > > ---
> > > > > > >  hw/ppc/vof.c | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> > > > > > > index e3b430a81f4f..b5b6514d79fc 100644
> > > > > > > --- a/hw/ppc/vof.c
> > > > > > > +++ b/hw/ppc/vof.c
> > > > > > > @@ -646,7 +646,7 @@ static void vof_dt_memory_available(void 
> > > > > > > *fdt, GArray *claimed, uint64_t base)
> > > > > > >  mem0_reg = fdt_getprop(fdt, offset, "reg", );
> > > > > > >  g_assert(mem0_reg && proplen == sizeof(uint32_t) * (ac + 
> > > > > > > sc));
> > > > > > >  if (sc == 2) {
> > > > > > > -mem0_end = be64_to_cpu(*(uint64_t *)(mem0_reg + 
> > > > > > > sizeof(uint32_t) * ac));
> > > > > > > +mem0_end = ldq_be_p(mem0_reg + sizeof(uint32_t) * ac);
> > > > > > >  } else {
> > > > > > >  mem0_end = be32_to_cpu(*(uint32_t *)(mem0_reg + 
> > > > > > > sizeof(uint32_t) * ac));
> > > > > > >  }
> > > > > >
> > > > > > I did wonder if there was a better way to do what this is doing,
> > > > > > but neither we (in system/device_tree.c) nor libfdt seem to
> > > > > > provide one.
> > > > >
> > > > > libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> > > > > not an automatic aligned-or-unaligned helper.   Maybe we should add 
> > > > > that?
> > > >
> > > > fdt32_ld() and friends only do the "load from this bit of memory"
> > > > part, which we already have QEMU utility functions for (and which
> > > > are this patch uses).
> > > >
> > > > This particular bit of code is dealing with an fdt property ("memory")
> > > > that is an array of (address, size) tuples where address and size
> > > > can independently be either 32 or 64 bits, and it wants the
> > > > size value of tuple 0. So the missing functionality is something at
> > > > a higher level than fdt32_ld() which would let you say "give me
> > > > tuple N field X" with some way to specify the tuple layout. (Which
> > > > is an awkward kind of API to write in C.)
> > > >
> > > > Slightly less general, but for this case we could perhaps have
> > > > something like the getprop equivalent of qemu_fdt_setprop_sized_cells():
> > > >
> > > >   uint64_t value_array[2];
> > > >   qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", _array,
> > > >ac, sc);
> > > >   /*
> > > >* fills in value_array[0] with address, value_array[1] with size,
> > > >* probably barfs if the varargs-list of cell-sizes doesn't
> > > >* cover the whole property, similar to the current assert on
> > > >* proplen.
> > > >*/
> > > >   mem0_end = value_array[0];
> > > 
> > > Since 4/8 byte cells are most common and size is probably
> > > normally known, what about something simpler to start with?
> >
> > Hrm, I don't think this helps much.  As Peter points out the actual
> > load isn't really the issue, it's locating the right spot for it.
> 
> I don't really see why that's a problem, it's just a pointer
> addition - base + fdt_address_cells * 4. The problem was in

This is harder if #address-cells and #size-cells are different, or if
you're parsing ranges and #address-cells is different between parent
and child node.

> the memory access (yes it's fixed with the patch but you could
> add a general libfdt way to do it).

Huh.. well I'm getting different impressions of what the problem
actually is from what I initially read versus Peter Maydell's
comments, so I don't really know what to think.

If it's just the load then fdt32_ld() etc. already exist.  Or is it
really such a hot path that unconditionally handling unaligned
accesses isn't tenable?

> Some fancy function like above could be used, But is it really
> worth implementing such a thing for this?
> 
> Thanks,
> Nick
> 

-- 
David Gibson (he or they)   | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you, not the other way
| around.
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[Nicholas Piggin]

On Fri Jul 5, 2024 at 11:41 AM AEST, David Gibson wrote:
> On Fri, Jul 05, 2024 at 11:18:47AM +1000, Nicholas Piggin wrote:
> > On Thu Jul 4, 2024 at 10:15 PM AEST, Peter Maydell wrote:
> > > On Sat, 29 Jun 2024 at 04:17, David Gibson  
> > > wrote:
> > > >
> > > > On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > > > > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki 
> > > > >  wrote:
> > > > > >
> > > > > > FDT properties are aligned by 4 bytes, not 8 bytes.
> > > > > >
> > > > > > Signed-off-by: Akihiko Odaki 
> > > > > > ---
> > > > > >  hw/ppc/vof.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> > > > > > index e3b430a81f4f..b5b6514d79fc 100644
> > > > > > --- a/hw/ppc/vof.c
> > > > > > +++ b/hw/ppc/vof.c
> > > > > > @@ -646,7 +646,7 @@ static void vof_dt_memory_available(void *fdt, 
> > > > > > GArray *claimed, uint64_t base)
> > > > > >  mem0_reg = fdt_getprop(fdt, offset, "reg", );
> > > > > >  g_assert(mem0_reg && proplen == sizeof(uint32_t) * (ac + sc));
> > > > > >  if (sc == 2) {
> > > > > > -mem0_end = be64_to_cpu(*(uint64_t *)(mem0_reg + 
> > > > > > sizeof(uint32_t) * ac));
> > > > > > +mem0_end = ldq_be_p(mem0_reg + sizeof(uint32_t) * ac);
> > > > > >  } else {
> > > > > >  mem0_end = be32_to_cpu(*(uint32_t *)(mem0_reg + 
> > > > > > sizeof(uint32_t) * ac));
> > > > > >  }
> > > > >
> > > > > I did wonder if there was a better way to do what this is doing,
> > > > > but neither we (in system/device_tree.c) nor libfdt seem to
> > > > > provide one.
> > > >
> > > > libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> > > > not an automatic aligned-or-unaligned helper.   Maybe we should add 
> > > > that?
> > >
> > > fdt32_ld() and friends only do the "load from this bit of memory"
> > > part, which we already have QEMU utility functions for (and which
> > > are this patch uses).
> > >
> > > This particular bit of code is dealing with an fdt property ("memory")
> > > that is an array of (address, size) tuples where address and size
> > > can independently be either 32 or 64 bits, and it wants the
> > > size value of tuple 0. So the missing functionality is something at
> > > a higher level than fdt32_ld() which would let you say "give me
> > > tuple N field X" with some way to specify the tuple layout. (Which
> > > is an awkward kind of API to write in C.)
> > >
> > > Slightly less general, but for this case we could perhaps have
> > > something like the getprop equivalent of qemu_fdt_setprop_sized_cells():
> > >
> > >   uint64_t value_array[2];
> > >   qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", _array,
> > >ac, sc);
> > >   /*
> > >* fills in value_array[0] with address, value_array[1] with size,
> > >* probably barfs if the varargs-list of cell-sizes doesn't
> > >* cover the whole property, similar to the current assert on
> > >* proplen.
> > >*/
> > >   mem0_end = value_array[0];
> > 
> > Since 4/8 byte cells are most common and size is probably
> > normally known, what about something simpler to start with?
>
> Hrm, I don't think this helps much.  As Peter points out the actual
> load isn't really the issue, it's locating the right spot for it.

I don't really see why that's a problem, it's just a pointer
addition - base + fdt_address_cells * 4. The problem was in
the memory access (yes it's fixed with the patch but you could
add a general libfdt way to do it).

Some fancy function like above could be used, But is it really
worth implementing such a thing for this?

Thanks,
Nick

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[David Gibson]

On Thu, Jul 04, 2024 at 01:15:57PM +0100, Peter Maydell wrote:
> On Sat, 29 Jun 2024 at 04:17, David Gibson  
> wrote:
> >
> > On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki  
> > > wrote:
> > > >
> > > > FDT properties are aligned by 4 bytes, not 8 bytes.
> > > >
> > > > Signed-off-by: Akihiko Odaki 
> > > > ---
> > > >  hw/ppc/vof.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> > > > index e3b430a81f4f..b5b6514d79fc 100644
> > > > --- a/hw/ppc/vof.c
> > > > +++ b/hw/ppc/vof.c
> > > > @@ -646,7 +646,7 @@ static void vof_dt_memory_available(void *fdt, 
> > > > GArray *claimed, uint64_t base)
> > > >  mem0_reg = fdt_getprop(fdt, offset, "reg", );
> > > >  g_assert(mem0_reg && proplen == sizeof(uint32_t) * (ac + sc));
> > > >  if (sc == 2) {
> > > > -mem0_end = be64_to_cpu(*(uint64_t *)(mem0_reg + 
> > > > sizeof(uint32_t) * ac));
> > > > +mem0_end = ldq_be_p(mem0_reg + sizeof(uint32_t) * ac);
> > > >  } else {
> > > >  mem0_end = be32_to_cpu(*(uint32_t *)(mem0_reg + 
> > > > sizeof(uint32_t) * ac));
> > > >  }
> > >
> > > I did wonder if there was a better way to do what this is doing,
> > > but neither we (in system/device_tree.c) nor libfdt seem to
> > > provide one.
> >
> > libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> > not an automatic aligned-or-unaligned helper.   Maybe we should add that?
> 
> fdt32_ld() and friends only do the "load from this bit of memory"
> part, which we already have QEMU utility functions for (and which
> are this patch uses).
> 
> This particular bit of code is dealing with an fdt property ("memory")
> that is an array of (address, size) tuples where address and size
> can independently be either 32 or 64 bits, and it wants the
> size value of tuple 0. So the missing functionality is something at
> a higher level than fdt32_ld() which would let you say "give me
> tuple N field X" with some way to specify the tuple layout. (Which
> is an awkward kind of API to write in C.)

Ah, right.  Yeah.. that's a pretty awkward API in C.

> Slightly less general, but for this case we could perhaps have
> something like the getprop equivalent of qemu_fdt_setprop_sized_cells():
> 
>   uint64_t value_array[2];
>   qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", _array,
>ac, sc);
>   /*
>* fills in value_array[0] with address, value_array[1] with size,
>* probably barfs if the varargs-list of cell-sizes doesn't
>* cover the whole property, similar to the current assert on
>* proplen.
>*/
>   mem0_end = value_array[0];

Seems reasonable to me.  The only other thought I had was something
like Python's struct.unpack() [0].  But your suggestion is probably
more natural in C.

[0] https://docs.python.org/3/library/struct.html#struct.unpack

-- 
David Gibson (he or they)   | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you, not the other way
| around.
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[David Gibson]

On Fri, Jul 05, 2024 at 11:18:47AM +1000, Nicholas Piggin wrote:
> On Thu Jul 4, 2024 at 10:15 PM AEST, Peter Maydell wrote:
> > On Sat, 29 Jun 2024 at 04:17, David Gibson  
> > wrote:
> > >
> > > On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > > > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki  
> > > > wrote:
> > > > >
> > > > > FDT properties are aligned by 4 bytes, not 8 bytes.
> > > > >
> > > > > Signed-off-by: Akihiko Odaki 
> > > > > ---
> > > > >  hw/ppc/vof.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> > > > > index e3b430a81f4f..b5b6514d79fc 100644
> > > > > --- a/hw/ppc/vof.c
> > > > > +++ b/hw/ppc/vof.c
> > > > > @@ -646,7 +646,7 @@ static void vof_dt_memory_available(void *fdt, 
> > > > > GArray *claimed, uint64_t base)
> > > > >  mem0_reg = fdt_getprop(fdt, offset, "reg", );
> > > > >  g_assert(mem0_reg && proplen == sizeof(uint32_t) * (ac + sc));
> > > > >  if (sc == 2) {
> > > > > -mem0_end = be64_to_cpu(*(uint64_t *)(mem0_reg + 
> > > > > sizeof(uint32_t) * ac));
> > > > > +mem0_end = ldq_be_p(mem0_reg + sizeof(uint32_t) * ac);
> > > > >  } else {
> > > > >  mem0_end = be32_to_cpu(*(uint32_t *)(mem0_reg + 
> > > > > sizeof(uint32_t) * ac));
> > > > >  }
> > > >
> > > > I did wonder if there was a better way to do what this is doing,
> > > > but neither we (in system/device_tree.c) nor libfdt seem to
> > > > provide one.
> > >
> > > libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> > > not an automatic aligned-or-unaligned helper.   Maybe we should add that?
> >
> > fdt32_ld() and friends only do the "load from this bit of memory"
> > part, which we already have QEMU utility functions for (and which
> > are this patch uses).
> >
> > This particular bit of code is dealing with an fdt property ("memory")
> > that is an array of (address, size) tuples where address and size
> > can independently be either 32 or 64 bits, and it wants the
> > size value of tuple 0. So the missing functionality is something at
> > a higher level than fdt32_ld() which would let you say "give me
> > tuple N field X" with some way to specify the tuple layout. (Which
> > is an awkward kind of API to write in C.)
> >
> > Slightly less general, but for this case we could perhaps have
> > something like the getprop equivalent of qemu_fdt_setprop_sized_cells():
> >
> >   uint64_t value_array[2];
> >   qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", _array,
> >ac, sc);
> >   /*
> >* fills in value_array[0] with address, value_array[1] with size,
> >* probably barfs if the varargs-list of cell-sizes doesn't
> >* cover the whole property, similar to the current assert on
> >* proplen.
> >*/
> >   mem0_end = value_array[0];
> 
> Since 4/8 byte cells are most common and size is probably
> normally known, what about something simpler to start with?

Hrm, I don't think this helps much.  As Peter points out the actual
load isn't really the issue, it's locating the right spot for it.

> 
> Thanks,
> Nick
> 
> ---
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 0677fea..c4b6355 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -148,6 +148,15 @@ static inline uint32_t fdt32_ld(const fdt32_t *p)
>   | bp[3];
>  }
>  
> +/*
> + * Load the value from a 32-bit cell of a property. Cells are 32-bit aligned
> + * so can use a single load.
> + */
> +static inline uint32_t fdt32_ld_prop(const fdt32_t *p)
> +{
> + return fdt32_to_cpu(*p);
> +}
> +
>  static inline void fdt32_st(void *property, uint32_t value)
>  {
>   uint8_t *bp = (uint8_t *)property;
> @@ -172,6 +181,18 @@ static inline uint64_t fdt64_ld(const fdt64_t *p)
>   | bp[7];
>  }
>  
> +/*
> + * Load the value from a 64-bit cell of a property. Cells are 32-bit aligned
> + * so can use two loads.
> + */
> +static inline uint64_t fdt64_ld_prop(const fdt64_t *p)
> +{
> + const fdt64_t *_p = p;
> +
> + return ((uint64_t)fdt32_to_cpu(_p[0]) << 32)
> + | fdt32_to_cpu(_p[1]);
> +}
> +
>  static inline void fdt64_st(void *property, uint64_t value)
>  {
>   uint8_t *bp = (uint8_t *)property;
> 

-- 
David Gibson (he or they)   | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you, not the other way
| around.
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[Nicholas Piggin]

On Thu Jul 4, 2024 at 10:15 PM AEST, Peter Maydell wrote:
> On Sat, 29 Jun 2024 at 04:17, David Gibson  
> wrote:
> >
> > On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki  
> > > wrote:
> > > >
> > > > FDT properties are aligned by 4 bytes, not 8 bytes.
> > > >
> > > > Signed-off-by: Akihiko Odaki 
> > > > ---
> > > >  hw/ppc/vof.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> > > > index e3b430a81f4f..b5b6514d79fc 100644
> > > > --- a/hw/ppc/vof.c
> > > > +++ b/hw/ppc/vof.c
> > > > @@ -646,7 +646,7 @@ static void vof_dt_memory_available(void *fdt, 
> > > > GArray *claimed, uint64_t base)
> > > >  mem0_reg = fdt_getprop(fdt, offset, "reg", );
> > > >  g_assert(mem0_reg && proplen == sizeof(uint32_t) * (ac + sc));
> > > >  if (sc == 2) {
> > > > -mem0_end = be64_to_cpu(*(uint64_t *)(mem0_reg + 
> > > > sizeof(uint32_t) * ac));
> > > > +mem0_end = ldq_be_p(mem0_reg + sizeof(uint32_t) * ac);
> > > >  } else {
> > > >  mem0_end = be32_to_cpu(*(uint32_t *)(mem0_reg + 
> > > > sizeof(uint32_t) * ac));
> > > >  }
> > >
> > > I did wonder if there was a better way to do what this is doing,
> > > but neither we (in system/device_tree.c) nor libfdt seem to
> > > provide one.
> >
> > libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> > not an automatic aligned-or-unaligned helper.   Maybe we should add that?
>
> fdt32_ld() and friends only do the "load from this bit of memory"
> part, which we already have QEMU utility functions for (and which
> are this patch uses).
>
> This particular bit of code is dealing with an fdt property ("memory")
> that is an array of (address, size) tuples where address and size
> can independently be either 32 or 64 bits, and it wants the
> size value of tuple 0. So the missing functionality is something at
> a higher level than fdt32_ld() which would let you say "give me
> tuple N field X" with some way to specify the tuple layout. (Which
> is an awkward kind of API to write in C.)
>
> Slightly less general, but for this case we could perhaps have
> something like the getprop equivalent of qemu_fdt_setprop_sized_cells():
>
>   uint64_t value_array[2];
>   qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", _array,
>ac, sc);
>   /*
>* fills in value_array[0] with address, value_array[1] with size,
>* probably barfs if the varargs-list of cell-sizes doesn't
>* cover the whole property, similar to the current assert on
>* proplen.
>*/
>   mem0_end = value_array[0];

Since 4/8 byte cells are most common and size is probably
normally known, what about something simpler to start with?

Thanks,
Nick

---
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 0677fea..c4b6355 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -148,6 +148,15 @@ static inline uint32_t fdt32_ld(const fdt32_t *p)
| bp[3];
 }
 
+/*
+ * Load the value from a 32-bit cell of a property. Cells are 32-bit aligned
+ * so can use a single load.
+ */
+static inline uint32_t fdt32_ld_prop(const fdt32_t *p)
+{
+   return fdt32_to_cpu(*p);
+}
+
 static inline void fdt32_st(void *property, uint32_t value)
 {
uint8_t *bp = (uint8_t *)property;
@@ -172,6 +181,18 @@ static inline uint64_t fdt64_ld(const fdt64_t *p)
| bp[7];
 }
 
+/*
+ * Load the value from a 64-bit cell of a property. Cells are 32-bit aligned
+ * so can use two loads.
+ */
+static inline uint64_t fdt64_ld_prop(const fdt64_t *p)
+{
+   const fdt64_t *_p = p;
+
+   return ((uint64_t)fdt32_to_cpu(_p[0]) << 32)
+   | fdt32_to_cpu(_p[1]);
+}
+
 static inline void fdt64_st(void *property, uint64_t value)
 {
uint8_t *bp = (uint8_t *)property;

Re: [PATCH] hw/ufs: Fix mcq register range determination logic

[Minwoo Im]

On 24-07-03 17:54:10, Jeuk Kim wrote:
> The function ufs_is_mcq_reg() only evaluated the range of the
> mcq_op_reg offset, which is defined as a constant.
> Therefore, it was possible for ufs_is_mcq_reg() to return true
> despite ufs device is configured to not support the mcq.
> This could cause ufs_mmio_read()/ufs_mmio_write() to overflow the
> buffer. So fix it.
> 
> Fixes: 5c079578d2e4 ("hw/ufs: Add support MCQ of UFSHCI 4.0")
> Signed-off-by: Jeuk Kim 

Reviewed-by: Minwoo Im 

Re: [PATCH v6 09/10] hw/nvme: add reservation protocal command

[Stefan Hajnoczi]

I will skip this since Klaus Jensen's review is required for NVMe anyway.

Stefan


signature.asc
Description: PGP signature

Re: [PATCH v6 05/10] hw/scsi: add persistent reservation in/out api for scsi device

[Stefan Hajnoczi]

On Thu, Jun 13, 2024 at 03:13:22PM +0800, Changqi Lu wrote:
> Add persistent reservation in/out operations in the
> SCSI device layer. By introducing the persistent
> reservation in/out api, this enables the SCSI device
> to perform reservation-related tasks, including querying
> keys, querying reservation status, registering reservation
> keys, initiating and releasing reservations, as well as
> clearing and preempting reservations held by other keys.
> 
> These operations are crucial for management and control of
> shared storage resources in a persistent manner.
> 
> Signed-off-by: Changqi Lu 
> Signed-off-by: zhenwei pi 
> ---
>  hw/scsi/scsi-disk.c | 352 
>  1 file changed, 352 insertions(+)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 4bd7af9d0c..0e964dbd87 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -32,6 +32,7 @@
>  #include "migration/vmstate.h"
>  #include "hw/scsi/emulation.h"
>  #include "scsi/constants.h"
> +#include "scsi/utils.h"
>  #include "sysemu/block-backend.h"
>  #include "sysemu/blockdev.h"
>  #include "hw/block/block.h"
> @@ -42,6 +43,7 @@
>  #include "qemu/cutils.h"
>  #include "trace.h"
>  #include "qom/object.h"
> +#include "block/block_int.h"
>  
>  #ifdef __linux
>  #include 
> @@ -1474,6 +1476,346 @@ static void scsi_disk_emulate_read_data(SCSIRequest 
> *req)
>  scsi_req_complete(>req, GOOD);
>  }
>  
> +typedef struct SCSIPrReadKeys {
> +uint32_t generation;
> +uint32_t num_keys;
> +uint64_t *keys;
> +void *req;
> +} SCSIPrReadKeys;
> +
> +typedef struct SCSIPrReadReservation {
> +uint32_t generation;
> +uint64_t key;
> +BlockPrType type;
> +void *req;
> +} SCSIPrReadReservation;
> +
> +static void scsi_pr_read_keys_complete(void *opaque, int ret)
> +{
> +int num_keys;
> +uint8_t *buf;
> +SCSIPrReadKeys *blk_keys = (SCSIPrReadKeys *)opaque;
> +SCSIDiskReq *r = (SCSIDiskReq *)blk_keys->req;
> +SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> +
> +assert(blk_get_aio_context(s->qdev.conf.blk) ==
> +qemu_get_current_aio_context());
> +
> +assert(r->req.aiocb != NULL);
> +r->req.aiocb = NULL;
> +
> +if (scsi_disk_req_check_error(r, ret, true)) {
> +goto done;
> +}
> +
> +buf = scsi_req_get_buf(>req);
> +num_keys = MIN(blk_keys->num_keys, ret);
> +blk_keys->generation = cpu_to_be32(blk_keys->generation);
> +memcpy([0], _keys->generation, 4);
> +for (int i = 0; i < num_keys; i++) {
> +blk_keys->keys[i] = cpu_to_be64(blk_keys->keys[i]);
> +memcpy([8 + i * 8], _keys->keys[i], 8);
> +}
> +num_keys = cpu_to_be32(num_keys * 8);
> +memcpy([4], _keys, 4);
> +
> +scsi_req_data(>req, r->buflen);
> +done:
> +scsi_req_unref(>req);
> +g_free(blk_keys->keys);
> +g_free(blk_keys);
> +}
> +
> +static int scsi_disk_emulate_pr_read_keys(SCSIRequest *req)
> +{
> +SCSIPrReadKeys *blk_keys;
> +SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> +SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
> +int buflen = MIN(r->req.cmd.xfer, r->buflen);
> +int num_keys = (buflen - sizeof(uint32_t) * 2) / sizeof(uint64_t);
> +
> +blk_keys = g_new0(SCSIPrReadKeys, 1);
> +blk_keys->generation = 0;
> +/* num_keys is the maximum number of keys that can be transmitted */
> +blk_keys->num_keys = num_keys;
> +blk_keys->keys = g_malloc(sizeof(uint64_t) * num_keys);
> +blk_keys->req = r;
> +
> +/* The request is used as the AIO opaque value, so add a ref.  */
> +scsi_req_ref(>req);
> +r->req.aiocb = blk_aio_pr_read_keys(s->qdev.conf.blk, 
> _keys->generation,
> +blk_keys->num_keys, blk_keys->keys,
> +scsi_pr_read_keys_complete, 
> blk_keys);
> +return 0;
> +}
> +
> +static void scsi_pr_read_reservation_complete(void *opaque, int ret)
> +{
> +uint8_t *buf;
> +uint32_t additional_len = 0;
> +SCSIPrReadReservation *blk_rsv = (SCSIPrReadReservation *)opaque;
> +SCSIDiskReq *r = (SCSIDiskReq *)blk_rsv->req;
> +SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> +
> +assert(blk_get_aio_context(s->qdev.conf.blk) ==
> +qemu_get_current_aio_context());
> +
> +assert(r->req.aiocb != NULL);
> +r->req.aiocb = NULL;
> +
> +if (scsi_disk_req_check_error(r, ret, true)) {
> +goto done;
> +}
> +
> +buf = scsi_req_get_buf(>req);
> +blk_rsv->generation = cpu_to_be32(blk_rsv->generation);
> +memcpy([0], _rsv->generation, 4);
> +if (ret) {
> +additional_len = cpu_to_be32(16);
> +blk_rsv->key = cpu_to_be64(blk_rsv->key);
> +memcpy([8], _rsv->key, 8);
> +buf[21] = block_pr_type_to_scsi(blk_rsv->type) & 0xf;
> +} else {
> +additional_len = cpu_to_be32(0);
> +}
> +
> +memcpy([4], _len, 4);
> + 

Re: [PATCH v6 10/10] block/iscsi: add persistent reservation in/out driver

[Stefan Hajnoczi]

On Thu, Jun 13, 2024 at 03:13:27PM +0800, Changqi Lu wrote:
> Add persistent reservation in/out operations for iscsi driver.
> The following methods are implemented: bdrv_co_pr_read_keys,
> bdrv_co_pr_read_reservation, bdrv_co_pr_register, bdrv_co_pr_reserve,
> bdrv_co_pr_release, bdrv_co_pr_clear and bdrv_co_pr_preempt.
> 
> Signed-off-by: Changqi Lu 
> Signed-off-by: zhenwei pi 
> ---
>  block/iscsi.c | 443 ++
>  1 file changed, 443 insertions(+)
> 
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 2ff14b7472..d94ebe35bd 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -96,6 +96,7 @@ typedef struct IscsiLun {
>  unsigned long *allocmap_valid;
>  long allocmap_size;
>  int cluster_size;
> +uint8_t pr_cap;
>  bool use_16_for_rw;
>  bool write_protected;
>  bool lbpme;
> @@ -280,6 +281,8 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int 
> status,
>  iTask->err_code = -error;
>  iTask->err_str = g_strdup(iscsi_get_error(iscsi));
>  }
> +} else if (status == SCSI_STATUS_RESERVATION_CONFLICT) {
> +iTask->err_code = -EBADE;

Should err_str be set too? For example, iscsi_co_writev() seems to
assume err_str is set if the iSCSI task fails.

>  }
>  }
>  }
> @@ -1792,6 +1795,52 @@ static void iscsi_save_designator(IscsiLun *lun,
>  }
>  }
>  
> +static void iscsi_get_pr_cap_sync(IscsiLun *iscsilun, Error **errp)
> +{
> +struct scsi_task *task = NULL;
> +struct scsi_persistent_reserve_in_report_capabilities *rc = NULL;
> +int retries = ISCSI_CMD_RETRIES;
> +int xferlen = sizeof(struct 
> scsi_persistent_reserve_in_report_capabilities);
> +
> +do {
> +if (task != NULL) {
> +scsi_free_scsi_task(task);
> +task = NULL;
> +}
> +
> +task = iscsi_persistent_reserve_in_sync(iscsilun->iscsi,
> +   iscsilun->lun, SCSI_PR_IN_REPORT_CAPABILITIES, xferlen);
> +if (task != NULL && task->status == SCSI_STATUS_GOOD) {
> +rc = scsi_datain_unmarshall(task);
> +if (rc == NULL) {
> +error_setg(errp,
> +"iSCSI: Failed to unmarshall report capabilities data.");
> +} else {
> +iscsilun->pr_cap =
> +
> scsi_pr_cap_to_block(rc->persistent_reservation_type_mask);
> +iscsilun->pr_cap |= (rc->ptpl_a) ? BLK_PR_CAP_PTPL : 0;
> +}
> +break;
> +}
> +
> +if (task != NULL && task->status == SCSI_STATUS_CHECK_CONDITION
> +&& task->sense.key == SCSI_SENSE_UNIT_ATTENTION) {
> +break;
> +}
> +
> +} while (task != NULL && task->status == SCSI_STATUS_CHECK_CONDITION
> + && task->sense.key == SCSI_SENSE_UNIT_ATTENTION
> + && retries-- > 0);
> +
> +if (task == NULL || task->status != SCSI_STATUS_GOOD) {
> +error_setg(errp, "iSCSI: failed to send report capabilities 
> command");
> +}
> +
> +if (task) {
> +scsi_free_scsi_task(task);
> +}
> +}
> +
>  static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
>Error **errp)
>  {
> @@ -2024,6 +2073,11 @@ static int iscsi_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  bs->supported_zero_flags = BDRV_REQ_MAY_UNMAP;
>  }
>  
> +iscsi_get_pr_cap_sync(iscsilun, _err);
> +if (local_err != NULL) {
> +error_propagate(errp, local_err);
> +ret = -EINVAL;
> +}
>  out:
>  qemu_opts_del(opts);
>  g_free(initiator_name);
> @@ -2110,6 +2164,8 @@ static void iscsi_refresh_limits(BlockDriverState *bs, 
> Error **errp)
>  bs->bl.opt_transfer = pow2floor(iscsilun->bl.opt_xfer_len *
>  iscsilun->block_size);
>  }
> +
> +bs->bl.pr_cap = iscsilun->pr_cap;
>  }
>  
>  /* Note that this will not re-establish a connection with an iSCSI target - 
> it
> @@ -2408,6 +2464,385 @@ out_unlock:
>  return r;
>  }
>  
> +static int coroutine_fn
> +iscsi_co_pr_read_keys(BlockDriverState *bs, uint32_t *generation,
> +  uint32_t num_keys, uint64_t *keys)
> +{
> +IscsiLun *iscsilun = bs->opaque;
> +QEMUIOVector qiov;
> +struct IscsiTask iTask;
> +int xferlen = sizeof(struct scsi_persistent_reserve_in_read_keys) +
> +  sizeof(uint64_t) * num_keys;
> +uint8_t *buf = g_malloc0(xferlen);
> +int32_t num_collect_keys = 0;
> +int r = 0;
> +
> +qemu_iovec_init_buf(, buf, xferlen);
> +iscsi_co_init_iscsitask(iscsilun, );
> +qemu_mutex_lock(>mutex);
> +retry:
> +iTask.task = iscsi_persistent_reserve_in_task(iscsilun->iscsi,
> + iscsilun->lun, SCSI_PR_IN_READ_KEYS, xferlen,
> + iscsi_co_generic_cb, );
> +
> +if 

Re: [PATCH v6 06/10] block/nvme: add reservation command protocol constants

[Stefan Hajnoczi]

On Thu, Jun 13, 2024 at 03:13:23PM +0800, Changqi Lu wrote:
> Add constants for the NVMe persistent command protocol.
> The constants include the reservation command opcode and
> reservation type values defined in section 7 of the NVMe
> 2.0 specification.
> 
> Signed-off-by: Changqi Lu 
> Signed-off-by: zhenwei pi 
> ---
>  include/block/nvme.h | 61 
>  1 file changed, 61 insertions(+)
> 
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index bb231d0b9a..da6ccb0f3b 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -633,6 +633,10 @@ enum NvmeIoCommands {
>  NVME_CMD_WRITE_ZEROES   = 0x08,
>  NVME_CMD_DSM= 0x09,
>  NVME_CMD_VERIFY = 0x0c,
> +NVME_CMD_RESV_REGISTER  = 0x0d,
> +NVME_CMD_RESV_REPORT= 0x0e,
> +NVME_CMD_RESV_ACQUIRE   = 0x11,
> +NVME_CMD_RESV_RELEASE   = 0x15,
>  NVME_CMD_IO_MGMT_RECV   = 0x12,

Keep NVME_CMD_IO_MGMT_RECV (0x12) before NVME_CMD_RESV_RELEASE (0x15) in
sorted order?

>  NVME_CMD_COPY   = 0x19,
>  NVME_CMD_IO_MGMT_SEND   = 0x1d,
> @@ -641,6 +645,63 @@ enum NvmeIoCommands {
>  NVME_CMD_ZONE_APPEND= 0x7d,
>  };
>  
> +typedef enum {
> +NVME_RESV_REGISTER_ACTION_REGISTER  = 0x00,
> +NVME_RESV_REGISTER_ACTION_UNREGISTER= 0x01,
> +NVME_RESV_REGISTER_ACTION_REPLACE   = 0x02,
> +} NvmeReservationRegisterAction;
> +
> +typedef enum {
> +NVME_RESV_RELEASE_ACTION_RELEASE= 0x00,
> +NVME_RESV_RELEASE_ACTION_CLEAR  = 0x01,
> +} NvmeReservationReleaseAction;
> +
> +typedef enum {
> +NVME_RESV_ACQUIRE_ACTION_ACQUIRE= 0x00,
> +NVME_RESV_ACQUIRE_ACTION_PREEMPT= 0x01,
> +NVME_RESV_ACQUIRE_ACTION_PREEMPT_AND_ABORT  = 0x02,
> +} NvmeReservationAcquireAction;
> +
> +typedef enum {
> +NVME_RESV_WRITE_EXCLUSIVE   = 0x01,
> +NVME_RESV_EXCLUSIVE_ACCESS  = 0x02,
> +NVME_RESV_WRITE_EXCLUSIVE_REGS_ONLY = 0x03,
> +NVME_RESV_EXCLUSIVE_ACCESS_REGS_ONLY= 0x04,
> +NVME_RESV_WRITE_EXCLUSIVE_ALL_REGS  = 0x05,
> +NVME_RESV_EXCLUSIVE_ACCESS_ALL_REGS = 0x06,
> +} NvmeResvType;
> +
> +typedef enum {
> +NVME_RESV_PTPL_NO_CHANGE = 0x00,
> +NVME_RESV_PTPL_DISABLE   = 0x02,
> +NVME_RESV_PTPL_ENABLE= 0x03,
> +} NvmeResvPTPL;
> +
> +typedef enum NVMEPrCap {
> +/* Persist Through Power Loss */
> +NVME_PR_CAP_PTPL = 1 << 0,
> +/* Write Exclusive reservation type */
> +NVME_PR_CAP_WR_EX = 1 << 1,
> +/* Exclusive Access reservation type */
> +NVME_PR_CAP_EX_AC = 1 << 2,
> +/* Write Exclusive Registrants Only reservation type */
> +NVME_PR_CAP_WR_EX_RO = 1 << 3,
> +/* Exclusive Access Registrants Only reservation type */
> +NVME_PR_CAP_EX_AC_RO = 1 << 4,
> +/* Write Exclusive All Registrants reservation type */
> +NVME_PR_CAP_WR_EX_AR = 1 << 5,
> +/* Exclusive Access All Registrants reservation type */
> +NVME_PR_CAP_EX_AC_AR = 1 << 6,
> +
> +NVME_PR_CAP_ALL = (NVME_PR_CAP_PTPL |
> +  NVME_PR_CAP_WR_EX |
> +  NVME_PR_CAP_EX_AC |
> +  NVME_PR_CAP_WR_EX_RO |
> +  NVME_PR_CAP_EX_AC_RO |
> +  NVME_PR_CAP_WR_EX_AR |
> +  NVME_PR_CAP_EX_AC_AR),
> +} NvmePrCap;
> +
>  typedef struct QEMU_PACKED NvmeDeleteQ {
>  uint8_t opcode;
>  uint8_t flags;
> -- 
> 2.20.1
> 


signature.asc
Description: PGP signature

Re: [PATCH v6 08/10] hw/nvme: enable ONCS and rescap function

[Stefan Hajnoczi]

On Thu, Jun 13, 2024 at 03:13:25PM +0800, Changqi Lu wrote:
> This commit enables ONCS to support the reservation
> function at the controller level. Also enables rescap
> function in the namespace by detecting the supported reservation
> function in the backend driver.
> 
> Signed-off-by: Changqi Lu 
> Signed-off-by: zhenwei pi 
> ---
>  hw/nvme/ctrl.c | 3 ++-
>  hw/nvme/ns.c   | 5 +
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 127c3d2383..182307a48b 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -8248,7 +8248,8 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice 
> *pci_dev)
>  id->nn = cpu_to_le32(NVME_MAX_NAMESPACES);
>  id->oncs = cpu_to_le16(NVME_ONCS_WRITE_ZEROES | NVME_ONCS_TIMESTAMP |
> NVME_ONCS_FEATURES | NVME_ONCS_DSM |
> -   NVME_ONCS_COMPARE | NVME_ONCS_COPY);
> +   NVME_ONCS_COMPARE | NVME_ONCS_COPY |
> +   NVME_ONCS_RESRVATIONS);

RESRVATIONS -> RESERVATIONS typo?

>  
>  /*
>   * NOTE: If this device ever supports a command set that does NOT use 0x0
> diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c
> index ea8db175db..320c9bf658 100644
> --- a/hw/nvme/ns.c
> +++ b/hw/nvme/ns.c
> @@ -20,6 +20,7 @@
>  #include "qemu/bitops.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/block-backend.h"
> +#include "block/block_int.h"
>  
>  #include "nvme.h"
>  #include "trace.h"
> @@ -33,6 +34,7 @@ void nvme_ns_init_format(NvmeNamespace *ns)
>  BlockDriverInfo bdi;
>  int npdg, ret;
>  int64_t nlbas;
> +uint8_t blk_pr_cap;
>  
>  ns->lbaf = id_ns->lbaf[NVME_ID_NS_FLBAS_INDEX(id_ns->flbas)];
>  ns->lbasz = 1 << ns->lbaf.ds;
> @@ -55,6 +57,9 @@ void nvme_ns_init_format(NvmeNamespace *ns)
>  }
>  
>  id_ns->npda = id_ns->npdg = npdg - 1;
> +
> +blk_pr_cap = blk_bs(ns->blkconf.blk)->file->bs->bl.pr_cap;

Kevin: This unprotected block graph access and the assumption that
->file->bs exists could be problematic. What is the best practice for
making this code safe and defensive?

> +id_ns->rescap = block_pr_cap_to_nvme(blk_pr_cap);
>  }
>  
>  static int nvme_ns_init(NvmeNamespace *ns, Error **errp)
> -- 
> 2.20.1
> 


signature.asc
Description: PGP signature

Re: [PATCH v6 05/10] hw/scsi: add persistent reservation in/out api for scsi device

[Stefan Hajnoczi]

On Thu, Jun 13, 2024 at 03:13:22PM +0800, Changqi Lu wrote:
> Add persistent reservation in/out operations in the
> SCSI device layer. By introducing the persistent
> reservation in/out api, this enables the SCSI device
> to perform reservation-related tasks, including querying
> keys, querying reservation status, registering reservation
> keys, initiating and releasing reservations, as well as
> clearing and preempting reservations held by other keys.
> 
> These operations are crucial for management and control of
> shared storage resources in a persistent manner.
> 
> Signed-off-by: Changqi Lu 
> Signed-off-by: zhenwei pi 

As mentioned in my reply to a previous version, I don't understand the
buffer allocation/sizing in hw/scsi/ so I haven't been able to fully
review this code for buffer overflows and input validation. cmd.xfer
isn't consistently used for size checks in the new functions. Maybe some
checks are missing?

> ---
>  hw/scsi/scsi-disk.c | 352 
>  1 file changed, 352 insertions(+)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index 4bd7af9d0c..0e964dbd87 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -32,6 +32,7 @@
>  #include "migration/vmstate.h"
>  #include "hw/scsi/emulation.h"
>  #include "scsi/constants.h"
> +#include "scsi/utils.h"
>  #include "sysemu/block-backend.h"
>  #include "sysemu/blockdev.h"
>  #include "hw/block/block.h"
> @@ -42,6 +43,7 @@
>  #include "qemu/cutils.h"
>  #include "trace.h"
>  #include "qom/object.h"
> +#include "block/block_int.h"
>  
>  #ifdef __linux
>  #include 
> @@ -1474,6 +1476,346 @@ static void scsi_disk_emulate_read_data(SCSIRequest 
> *req)
>  scsi_req_complete(>req, GOOD);
>  }
>  
> +typedef struct SCSIPrReadKeys {
> +uint32_t generation;
> +uint32_t num_keys;
> +uint64_t *keys;
> +void *req;

Why is this field void * instead of SCSIDiskReq *?

> +} SCSIPrReadKeys;
> +
> +typedef struct SCSIPrReadReservation {
> +uint32_t generation;
> +uint64_t key;
> +BlockPrType type;
> +void *req;

Same here.

> +} SCSIPrReadReservation;
> +
> +static void scsi_pr_read_keys_complete(void *opaque, int ret)
> +{
> +int num_keys;
> +uint8_t *buf;
> +SCSIPrReadKeys *blk_keys = (SCSIPrReadKeys *)opaque;
> +SCSIDiskReq *r = (SCSIDiskReq *)blk_keys->req;
> +SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> +
> +assert(blk_get_aio_context(s->qdev.conf.blk) ==
> +qemu_get_current_aio_context());
> +
> +assert(r->req.aiocb != NULL);
> +r->req.aiocb = NULL;
> +
> +if (scsi_disk_req_check_error(r, ret, true)) {
> +goto done;
> +}
> +
> +buf = scsi_req_get_buf(>req);
> +num_keys = MIN(blk_keys->num_keys, ret);
> +blk_keys->generation = cpu_to_be32(blk_keys->generation);
> +memcpy([0], _keys->generation, 4);
> +for (int i = 0; i < num_keys; i++) {
> +blk_keys->keys[i] = cpu_to_be64(blk_keys->keys[i]);
> +memcpy([8 + i * 8], _keys->keys[i], 8);
> +}
> +num_keys = cpu_to_be32(num_keys * 8);
> +memcpy([4], _keys, 4);
> +
> +scsi_req_data(>req, r->buflen);
> +done:
> +scsi_req_unref(>req);
> +g_free(blk_keys->keys);
> +g_free(blk_keys);
> +}
> +
> +static int scsi_disk_emulate_pr_read_keys(SCSIRequest *req)
> +{
> +SCSIPrReadKeys *blk_keys;
> +SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> +SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev);
> +int buflen = MIN(r->req.cmd.xfer, r->buflen);
> +int num_keys = (buflen - sizeof(uint32_t) * 2) / sizeof(uint64_t);

If buflen is an untrusted input then num_keys < 0 and maybe num_keys ==
0 need to be rejected with an error.

> +
> +blk_keys = g_new0(SCSIPrReadKeys, 1);
> +blk_keys->generation = 0;
> +/* num_keys is the maximum number of keys that can be transmitted */
> +blk_keys->num_keys = num_keys;
> +blk_keys->keys = g_malloc(sizeof(uint64_t) * num_keys);
> +blk_keys->req = r;
> +
> +/* The request is used as the AIO opaque value, so add a ref.  */
> +scsi_req_ref(>req);
> +r->req.aiocb = blk_aio_pr_read_keys(s->qdev.conf.blk, 
> _keys->generation,
> +blk_keys->num_keys, blk_keys->keys,
> +scsi_pr_read_keys_complete, 
> blk_keys);
> +return 0;
> +}
> +
> +static void scsi_pr_read_reservation_complete(void *opaque, int ret)
> +{
> +uint8_t *buf;
> +uint32_t additional_len = 0;
> +SCSIPrReadReservation *blk_rsv = (SCSIPrReadReservation *)opaque;
> +SCSIDiskReq *r = (SCSIDiskReq *)blk_rsv->req;
> +SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> +
> +assert(blk_get_aio_context(s->qdev.conf.blk) ==
> +qemu_get_current_aio_context());
> +
> +assert(r->req.aiocb != NULL);
> +r->req.aiocb = NULL;
> +
> +if (scsi_disk_req_check_error(r, ret, true)) {
> +goto 

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[Peter Maydell]

On Sat, 29 Jun 2024 at 04:17, David Gibson  wrote:
>
> On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki  
> > wrote:
> > >
> > > FDT properties are aligned by 4 bytes, not 8 bytes.
> > >
> > > Signed-off-by: Akihiko Odaki 
> > > ---
> > >  hw/ppc/vof.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> > > index e3b430a81f4f..b5b6514d79fc 100644
> > > --- a/hw/ppc/vof.c
> > > +++ b/hw/ppc/vof.c
> > > @@ -646,7 +646,7 @@ static void vof_dt_memory_available(void *fdt, GArray 
> > > *claimed, uint64_t base)
> > >  mem0_reg = fdt_getprop(fdt, offset, "reg", );
> > >  g_assert(mem0_reg && proplen == sizeof(uint32_t) * (ac + sc));
> > >  if (sc == 2) {
> > > -mem0_end = be64_to_cpu(*(uint64_t *)(mem0_reg + sizeof(uint32_t) 
> > > * ac));
> > > +mem0_end = ldq_be_p(mem0_reg + sizeof(uint32_t) * ac);
> > >  } else {
> > >  mem0_end = be32_to_cpu(*(uint32_t *)(mem0_reg + sizeof(uint32_t) 
> > > * ac));
> > >  }
> >
> > I did wonder if there was a better way to do what this is doing,
> > but neither we (in system/device_tree.c) nor libfdt seem to
> > provide one.
>
> libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> not an automatic aligned-or-unaligned helper.   Maybe we should add that?

fdt32_ld() and friends only do the "load from this bit of memory"
part, which we already have QEMU utility functions for (and which
are this patch uses).

This particular bit of code is dealing with an fdt property ("memory")
that is an array of (address, size) tuples where address and size
can independently be either 32 or 64 bits, and it wants the
size value of tuple 0. So the missing functionality is something at
a higher level than fdt32_ld() which would let you say "give me
tuple N field X" with some way to specify the tuple layout. (Which
is an awkward kind of API to write in C.)

Slightly less general, but for this case we could perhaps have
something like the getprop equivalent of qemu_fdt_setprop_sized_cells():

  uint64_t value_array[2];
  qemu_fdt_getprop_sized_cells(fdt, nodename, "memory", _array,
   ac, sc);
  /*
   * fills in value_array[0] with address, value_array[1] with size,
   * probably barfs if the varargs-list of cell-sizes doesn't
   * cover the whole property, similar to the current assert on
   * proplen.
   */
  mem0_end = value_array[0];

thanks
-- PMM

Re: [PATCH v2 06/15] ppc/vof: Fix unaligned FDT property access

[Nicholas Piggin]

On Sat Jun 29, 2024 at 1:16 PM AEST, David Gibson wrote:
> On Fri, Jun 28, 2024 at 04:20:02PM +0100, Peter Maydell wrote:
> > On Thu, 27 Jun 2024 at 14:39, Akihiko Odaki  
> > wrote:
> > >
> > > FDT properties are aligned by 4 bytes, not 8 bytes.
> > >
> > > Signed-off-by: Akihiko Odaki 
> > > ---
> > >  hw/ppc/vof.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> > > index e3b430a81f4f..b5b6514d79fc 100644
> > > --- a/hw/ppc/vof.c
> > > +++ b/hw/ppc/vof.c
> > > @@ -646,7 +646,7 @@ static void vof_dt_memory_available(void *fdt, GArray 
> > > *claimed, uint64_t base)
> > >  mem0_reg = fdt_getprop(fdt, offset, "reg", );
> > >  g_assert(mem0_reg && proplen == sizeof(uint32_t) * (ac + sc));
> > >  if (sc == 2) {
> > > -mem0_end = be64_to_cpu(*(uint64_t *)(mem0_reg + sizeof(uint32_t) 
> > > * ac));
> > > +mem0_end = ldq_be_p(mem0_reg + sizeof(uint32_t) * ac);
> > >  } else {
> > >  mem0_end = be32_to_cpu(*(uint32_t *)(mem0_reg + sizeof(uint32_t) 
> > > * ac));
> > >  }
> > 
> > I did wonder if there was a better way to do what this is doing,
> > but neither we (in system/device_tree.c) nor libfdt seem to
> > provide one.
>
> libfdt does provide unaligned access helpers (fdt32_ld() etc.), but
> not an automatic aligned-or-unaligned helper.   Maybe we should add that?

Runtime test if the pointer is aligned?

What about just fdt_prop32_ld() and fdt_prop64_ld() where you know it's
4 byte aligned. Then just do 2 x 4 byte loads for the 64-bit, I don't
think performance would matter so much to try get a single load.

Thanks,
Nick

Re: [PATCH v2 13/15] tests/qtest: Delete previous boot file

[Akihiko Odaki]

On 2024/07/02 16:31, Thomas Huth wrote:

On 27/06/2024 15.37, Akihiko Odaki wrote:

A test run may create boot files several times. Delete the previous boot
file before creating a new one.

Signed-off-by: Akihiko Odaki 
---
  tests/qtest/migration-test.c | 18 +++---
  1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index b7e3406471a6..5c0d669b6df3 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -129,12 +129,23 @@ static char *bootpath;
  #include "tests/migration/aarch64/a-b-kernel.h"
  #include "tests/migration/s390x/a-b-bios.h"
+static void bootfile_delete(void)
+{
+    unlink(bootpath);
+    g_free(bootpath);
+    bootpath = NULL;
+}
+
  static void bootfile_create(char *dir, bool suspend_me)
  {
  const char *arch = qtest_get_arch();
  unsigned char *content;
  size_t len;
+    if (bootpath) {
+    bootfile_delete();
+    }
+
  bootpath = g_strdup_printf("%s/bootsect", dir);
  if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
  /* the assembled x86 boot sector should be exactly one 
sector large */
@@ -164,13 +175,6 @@ static void bootfile_create(char *dir, bool 
suspend_me)

  fclose(bootfile);
  }
-static void bootfile_delete(void)
-{
-    unlink(bootpath);
-    g_free(bootpath);
-    bootpath = NULL;
-}
-
  /*
   * Wait for some output in the serial output file,
   * we get an 'A' followed by an endless string of 'B's



I think the better fix would be to call bootfile_create() only once from 
main() since we don't have to create the bootfile multiple times, do we?


The suspend_me parameter depends on test cases so probably we actually 
need to recreate in such cases.


Regards,
Akihiko Odaki

Re: [PATCH v2 00/15] Fix check-qtest-ppc64 sanitizer errors

[Nicholas Piggin]

On Tue Jul 2, 2024 at 4:23 PM AEST, Thomas Huth wrote:
> On 02/07/2024 00.23, BALATON Zoltan wrote:
> > On Mon, 1 Jul 2024, Michael S. Tsirkin wrote:
> >> On Thu, Jun 27, 2024 at 10:37:43PM +0900, Akihiko Odaki wrote:
> >>> Based-on: 
> >>> <3ad18bc590ef28e1526e8053568086b453e7ffde.1718211878.git.quic_mathb...@quicinc.com>
> >>> ("[PATCH] cpu: fix memleak of 'halt_cond' and 'thread'")
> >>>
> >>> I saw various sanitizer errors when running check-qtest-ppc64. While
> >>> I could just turn off sanitizers, I decided to tackle them this time.
> >>>
> >>> Unfortunately, GLib does not free test data in some cases so some
> >>> sanitizer errors remain. All sanitizer errors will be gone with this
> >>> patch series combined with the following change for GLib:
> >>> https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4120
> >>>
> >>> Signed-off-by: Akihiko Odaki 
> >>
> >>
> >> Reviewed-by: Michael S. Tsirkin 
> >>
> >> who's merging all this?
> > 
> > Maybe needs to be split. Mark had an alternative for macio which was picked 
> > up by Philippe if I'm not mistaken. I've sent an alternative for vt82c686 
> > which is still discussed but could belong to Philippe as well. PPC parts 
> > could be taken by the PPC maintainers if there were any active at the 
> > moment 
> > and I don't know who maintains tests normally or other misc areas.
>
> I can take the qtest patches through my tree.
>   cpu: Free cpu_ases
>   migration: Free removed SaveStateEntry
>   memory: Do not create circular reference with subregion
>   hw/virtio: Free vqs after vhost_dev_cleanup()

These are all core code, maybe Philippe, Peter, or Richard,
Fabiano for migration perhaps?

>   hw/ide: Convert macio ide_irq into GPIO line
>   hw/ide: Remove internal DMA qemu_irq
>   hw/isa/vt82c686: Define a GPIO line between vt82c686 and i8259

I guess I'll let you wrangle these and go through Philippe then?

>   spapr: Free stdout path
>   ppc/vof: Fix unaligned FDT property access

These ppc ones I can take, they seem okay. Sorry for my recent
inactivity.

Thanks,
Nick

>   tests/qtest: Use qtest_add_data_func_full()
>   tests/qtest: Free unused QMP response
>   tests/qtest: Free old machine variable name
>   tests/qtest: Delete previous boot file
>   tests/qtest: Free paths
>   tests/qtest: Free GThread

Re: [PULL 4/4] block: Parse filenames only when explicitly requested

[Michael Tokarev]

04.07.2024 14:29, Kevin Wolf wrote:

Am 03.07.2024 um 23:16 hat Michael Tokarev geschrieben:



I'm attaching backports of this change to 8.2 and 7.2 series.

Please check if the resulting backports are correct, or if they're needed
in the first place.  Note: 7.2 lacks quite some locking in this area, eg
v8.0.0-2069-g8394c35ee148 "block: Fix AioContext locking in bdrv_open_child()"
v8.2.0-rc0-59-g6bc0bcc89f84 "block: Fix deadlocks in bdrv_graph_wrunlock()".


Apart from minor style differences, your 7.2 backport is a perfect match
of the RHEL 9.2 backport which I already reviewed downstream. The 8.2
patch is a bit different from the RHEL 9.4 one because we backported the
final bits of the multiqueue work, but the differences make sense for
upstream QEMU 8.2.

So both patches look good to me.


Ok, this makes sense.  Thank you for the review, Kevin!

/mjt

--
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E  9D8B E14E 3F2A 9DD7 9199  28F1 61AD 3D98 
ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5  6EE1 95D1 886E 8FFB 810D  4324 457C E0A0 
8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt

Re: [PULL 4/4] block: Parse filenames only when explicitly requested

[Kevin Wolf]

Am 03.07.2024 um 23:16 hat Michael Tokarev geschrieben:
> 02.07.2024 19:39, Kevin Wolf wrote:
> > When handling image filenames from legacy options such as -drive or from
> > tools, these filenames are parsed for protocol prefixes, including for
> > the json:{} pseudo-protocol.
> > 
> > This behaviour is intended for filenames that come directly from the
> > command line and for backing files, which may come from the image file
> > itself. Higher level management tools generally take care to verify that
> > untrusted images don't contain a bad (or any) backing file reference;
> > 'qemu-img info' is a suitable tool for this.
> > 
> > However, for other files that can be referenced in images, such as
> > qcow2 data files or VMDK extents, the string from the image file is
> > usually not verified by management tools - and 'qemu-img info' wouldn't
> > be suitable because in contrast to backing files, it already opens these
> > other referenced files. So here the string should be interpreted as a
> > literal local filename. More complex configurations need to be specified
> > explicitly on the command line or in QMP.
> > 
> > This patch changes bdrv_open_inherit() so that it only parses filenames
> > if a new parameter parse_filename is true. It is set for the top level
> > in bdrv_open(), for the file child and for the backing file child. All
> > other callers pass false and disable filename parsing this way.
> 
> I'm attaching backports of this change to 8.2 and 7.2 series.
> 
> Please check if the resulting backports are correct, or if they're needed
> in the first place.  Note: 7.2 lacks quite some locking in this area, eg
> v8.0.0-2069-g8394c35ee148 "block: Fix AioContext locking in bdrv_open_child()"
> v8.2.0-rc0-59-g6bc0bcc89f84 "block: Fix deadlocks in bdrv_graph_wrunlock()".

Apart from minor style differences, your 7.2 backport is a perfect match
of the RHEL 9.2 backport which I already reviewed downstream. The 8.2
patch is a bit different from the RHEL 9.4 one because we backported the
final bits of the multiqueue work, but the differences make sense for
upstream QEMU 8.2.

So both patches look good to me.

Kevin

Re: [PATCH v3 4/8] hw/net:ftgmac100: update TX and RX packet buffers address to 64 bits

[Cédric Le Goater]

On 7/4/24 10:29 AM, Jamin Lin wrote:

ASPEED AST2700 SOC is a 64 bits quad core CPUs (Cortex-a35)
And the base address of dram is "0x4 " which
is 64bits address.

It have "TXDES 2" and "RXDES 2" to save the high part
physical address of packet buffer.
Ex: TX packet buffer address [34:0]
The "TXDES 2" bits [18:16] which corresponds the bits [34:32]
of the 64 bits address of the TX packet buffer address
and "TXDES 3" bits [31:0] which corresponds the bits [31:0]
of the 64 bits address of the TX packet buffer address.

Update TX and RX packet buffers address type to
64 bits for dram 64 bits address DMA support.

Signed-off-by: Jamin Lin 



Reviewed-by: Cédric Le Goater 

Thanks,

C.



---
  hw/net/ftgmac100.c | 21 ++---
  1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 68956aeb94..80f9cd56d5 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -175,6 +175,8 @@
  #define FTGMAC100_TXDES1_TX2FIC  (1 << 30)
  #define FTGMAC100_TXDES1_TXIC(1 << 31)
  
+#define FTGMAC100_TXDES2_TXBUF_BADR_HI(x)   (((x) >> 16) & 0x7)

+
  /*
   * Receive descriptor
   */
@@ -208,13 +210,15 @@
  #define FTGMAC100_RXDES1_UDP_CHKSUM_ERR  (1 << 26)
  #define FTGMAC100_RXDES1_IP_CHKSUM_ERR   (1 << 27)
  
+#define FTGMAC100_RXDES2_RXBUF_BADR_HI(x)   (((x) >> 16) & 0x7)

+
  /*
   * Receive and transmit Buffer Descriptor
   */
  typedef struct {
  uint32_tdes0;
  uint32_tdes1;
-uint32_tdes2;/* not used by HW */
+uint32_tdes2;/* used by HW 64 bits DMA */
  uint32_tdes3;
  } FTGMAC100Desc;
  
@@ -531,6 +535,7 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint64_t tx_ring,

  int frame_size = 0;
  uint8_t *ptr = s->frame;
  uint64_t addr = tx_descriptor;
+uint64_t buf_addr = 0;
  uint32_t flags = 0;
  
  while (1) {

@@ -569,7 +574,12 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint64_t 
tx_ring,
  len =  sizeof(s->frame) - frame_size;
  }
  
-if (dma_memory_read(_space_memory, bd.des3,

+buf_addr = bd.des3;
+if (s->dma64) {
+buf_addr = deposit64(buf_addr, 32, 32,
+ FTGMAC100_TXDES2_TXBUF_BADR_HI(bd.des2));
+}
+if (dma_memory_read(_space_memory, buf_addr,
  ptr, len, MEMTXATTRS_UNSPECIFIED)) {
  qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to read packet @ 
0x%x\n",
__func__, bd.des3);
@@ -1022,7 +1032,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, 
const uint8_t *buf,
  uint32_t flags = 0;
  uint64_t addr;
  uint32_t crc;
-uint32_t buf_addr;
+uint64_t buf_addr = 0;
  uint8_t *crc_ptr;
  uint32_t buf_len;
  size_t size = len;
@@ -1087,7 +1097,12 @@ static ssize_t ftgmac100_receive(NetClientState *nc, 
const uint8_t *buf,
  if (size < 4) {
  buf_len += size - 4;
  }
+
  buf_addr = bd.des3;
+if (s->dma64) {
+buf_addr = deposit64(buf_addr, 32, 32,
+ FTGMAC100_RXDES2_RXBUF_BADR_HI(bd.des2));
+}
  if (first && proto == ETH_P_VLAN && buf_len >= 18) {
  bd.des1 = lduw_be_p(buf + 14) | FTGMAC100_RXDES1_VLANTAG_AVAIL;
  

Re: [PATCH v3 2/8] hw/net:ftgmac100: update ring base address to 64 bits

[Cédric Le Goater]

On 7/4/24 10:29 AM, Jamin Lin wrote:

Update TX and RX ring base address data type to uint64_t for
64 bits dram address DMA support.

Both "Normal Priority Transmit Ring Base Address Register(0x20)" and
"Receive Ring Base Address Register (0x24)" are used for saving the
low part physical address of descriptor manager.

Therefore, changes to set TX and RX descriptor manager address bits [31:0]
in ftgmac100_read and ftgmac100_write functions.

Incrementing the version of vmstate to 2.

Signed-off-by: Jamin Lin 



Reviewed-by: Cédric Le Goater 

Thanks,

C.



---
  hw/net/ftgmac100.c | 33 -
  include/hw/net/ftgmac100.h |  9 -
  2 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 9e1f12cd33..d026242e2b 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -515,12 +515,12 @@ out:
  return frame_size;
  }
  
-static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,

-uint32_t tx_descriptor)
+static void ftgmac100_do_tx(FTGMAC100State *s, uint64_t tx_ring,
+uint64_t tx_descriptor)
  {
  int frame_size = 0;
  uint8_t *ptr = s->frame;
-uint32_t addr = tx_descriptor;
+uint64_t addr = tx_descriptor;
  uint32_t flags = 0;
  
  while (1) {

@@ -726,9 +726,9 @@ static uint64_t ftgmac100_read(void *opaque, hwaddr addr, 
unsigned size)
  case FTGMAC100_MATH1:
  return s->math[1];
  case FTGMAC100_RXR_BADR:
-return s->rx_ring;
+return extract64(s->rx_ring, 0, 32);
  case FTGMAC100_NPTXR_BADR:
-return s->tx_ring;
+return extract64(s->tx_ring, 0, 32);
  case FTGMAC100_ITC:
  return s->itc;
  case FTGMAC100_DBLAC:
@@ -799,9 +799,8 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
HWADDR_PRIx "\n", __func__, value);
  return;
  }
-
-s->rx_ring = value;
-s->rx_descriptor = s->rx_ring;
+s->rx_ring = deposit64(s->rx_ring, 0, 32, value);
+s->rx_descriptor = deposit64(s->rx_descriptor, 0, 32, value);
  break;
  
  case FTGMAC100_RBSR: /* DMA buffer size */

@@ -814,8 +813,8 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
HWADDR_PRIx "\n", __func__, value);
  return;
  }
-s->tx_ring = value;
-s->tx_descriptor = s->tx_ring;
+s->tx_ring = deposit64(s->tx_ring, 0, 32, value);
+s->tx_descriptor = deposit64(s->tx_descriptor, 0, 32, value);
  break;
  
  case FTGMAC100_NPTXPD: /* Trigger transmit */

@@ -957,7 +956,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const 
uint8_t *buf,
  FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc));
  FTGMAC100Desc bd;
  uint32_t flags = 0;
-uint32_t addr;
+uint64_t addr;
  uint32_t crc;
  uint32_t buf_addr;
  uint8_t *crc_ptr;
@@ -1126,18 +1125,14 @@ static void ftgmac100_realize(DeviceState *dev, Error 
**errp)
  
  static const VMStateDescription vmstate_ftgmac100 = {

  .name = TYPE_FTGMAC100,
-.version_id = 1,
-.minimum_version_id = 1,
+.version_id = 2,
+.minimum_version_id = 2,
  .fields = (const VMStateField[]) {
  VMSTATE_UINT32(irq_state, FTGMAC100State),
  VMSTATE_UINT32(isr, FTGMAC100State),
  VMSTATE_UINT32(ier, FTGMAC100State),
  VMSTATE_UINT32(rx_enabled, FTGMAC100State),
-VMSTATE_UINT32(rx_ring, FTGMAC100State),
  VMSTATE_UINT32(rbsr, FTGMAC100State),
-VMSTATE_UINT32(tx_ring, FTGMAC100State),
-VMSTATE_UINT32(rx_descriptor, FTGMAC100State),
-VMSTATE_UINT32(tx_descriptor, FTGMAC100State),
  VMSTATE_UINT32_ARRAY(math, FTGMAC100State, 2),
  VMSTATE_UINT32(itc, FTGMAC100State),
  VMSTATE_UINT32(aptcr, FTGMAC100State),
@@ -1156,6 +1151,10 @@ static const VMStateDescription vmstate_ftgmac100 = {
  VMSTATE_UINT32(phy_int_mask, FTGMAC100State),
  VMSTATE_UINT32(txdes0_edotr, FTGMAC100State),
  VMSTATE_UINT32(rxdes0_edorr, FTGMAC100State),
+VMSTATE_UINT64(rx_ring, FTGMAC100State),
+VMSTATE_UINT64(tx_ring, FTGMAC100State),
+VMSTATE_UINT64(rx_descriptor, FTGMAC100State),
+VMSTATE_UINT64(tx_descriptor, FTGMAC100State),
  VMSTATE_END_OF_LIST()
  }
  };
diff --git a/include/hw/net/ftgmac100.h b/include/hw/net/ftgmac100.h
index 269446e858..aae57ae8cb 100644
--- a/include/hw/net/ftgmac100.h
+++ b/include/hw/net/ftgmac100.h
@@ -42,10 +42,6 @@ struct FTGMAC100State {
  uint32_t isr;
  uint32_t ier;
  uint32_t rx_enabled;
-uint32_t rx_ring;
-uint32_t rx_descriptor;
-uint32_t tx_ring;
-uint32_t tx_descriptor;
  uint32_t math[2];
  uint32_t rbsr;
  uint32_t itc;
@@ -58,7 +54,10 @@ struct FTGMAC100State {
  uint32_t phycr;
  uint32_t phydata;
  uint32_t fcr;
-

Re: [PATCH v3 8/8] machine_aspeed.py: update to test network for AST2700

[Cédric Le Goater]

On 7/4/24 10:29 AM, Jamin Lin wrote:

Update test case to test network connection via SSH.

Test command:
```
cd build
pyvenv/bin/avocado run 
../qemu/tests/avocado/machine_aspeed.py:AST2x00MachineSDK.test_aarch64_ast2700_evb_sdk_v09_02
```

Signed-off-by: Jamin Lin 



Reviewed-by: Cédric Le Goater 

Thanks,

C.



---
  tests/avocado/machine_aspeed.py | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/avocado/machine_aspeed.py b/tests/avocado/machine_aspeed.py
index 13fe128fc9..f66ad38d35 100644
--- a/tests/avocado/machine_aspeed.py
+++ b/tests/avocado/machine_aspeed.py
@@ -313,14 +313,14 @@ def do_test_arm_aspeed_sdk_start(self, image):
  
  def do_test_aarch64_aspeed_sdk_start(self, image):

  self.vm.set_console()
-self.vm.add_args('-drive', 'file=' + image + ',if=mtd,format=raw')
+self.vm.add_args('-drive', 'file=' + image + ',if=mtd,format=raw',
+ '-net', 'nic', '-net', 
'user,hostfwd=:127.0.0.1:0-:22')
  
  self.vm.launch()
  
  self.wait_for_console_pattern('U-Boot 2023.10')

  self.wait_for_console_pattern('## Loading kernel from FIT Image')
  self.wait_for_console_pattern('Starting kernel ...')
-self.wait_for_console_pattern("systemd[1]: Hostname set to")
  
  @skipUnless(os.getenv('QEMU_TEST_FLAKY_TESTS'), 'Test is unstable on GitLab')
  
@@ -436,4 +436,6 @@ def test_aarch64_ast2700_evb_sdk_v09_02(self):
  
  self.vm.add_args('-smp', str(num_cpu))

  self.do_test_aarch64_aspeed_sdk_start(image_dir + 'image-bmc')
+self.wait_for_console_pattern('nodistro.0 ast2700-default ttyS12')
+self.ssh_connect('root', '0penBmc', False)
  

Re: [PATCH v3 5/8] aspeed/soc: set dma64 property for AST2700 ftgmac100

[Cédric Le Goater]

On 7/4/24 10:29 AM, Jamin Lin wrote:

ASPEED AST2700 SOC is a 64 bits quad core CPUs (Cortex-a35)
And the base address of dram is "0x4 " which
is 64bits address.

Set dma64 property for ftgmac100 model to support
64bits dram address DMA.

Signed-off-by: Jamin Lin 



Reviewed-by: Cédric Le Goater 

Thanks,

C.



---
  hw/arm/aspeed_ast27x0.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index 18e6a8b10c..a9fb0d4b88 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -552,9 +552,12 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, 
Error **errp)
  return;
  }
  
+/* Net */

  for (i = 0; i < sc->macs_num; i++) {
  object_property_set_bool(OBJECT(>ftgmac100[i]), "aspeed", true,
   _abort);
+object_property_set_bool(OBJECT(>ftgmac100[i]), "dma64", true,
+ _abort);
  if (!sysbus_realize(SYS_BUS_DEVICE(>ftgmac100[i]), errp)) {
  return;
  }

Re: [PATCH v3 7/8] machine_aspeed.py: update to test ASPEED OpenBMC SDK v09.02 for AST2700

[Cédric Le Goater]

On 7/4/24 10:29 AM, Jamin Lin wrote:

Update test case to test ASPEED OpenBMC SDK v09.02 for AST2700.

ASPEED fixed TX mask issue from linux/drivers/ftgmac100.c.
It is required to use ASPEED OpenBMC SDK since v09.02
for AST2700 QEMU network testing.

A test image is downloaded from the ASPEED Forked OpenBMC GitHub
release repository :
https://github.com/AspeedTech-BMC/openbmc/releases/

Signed-off-by: Jamin Lin 



Reviewed-by: Cédric Le Goater 

Thanks,

C.



---
  tests/avocado/machine_aspeed.py | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/avocado/machine_aspeed.py b/tests/avocado/machine_aspeed.py
index 3a20644fb2..13fe128fc9 100644
--- a/tests/avocado/machine_aspeed.py
+++ b/tests/avocado/machine_aspeed.py
@@ -387,15 +387,15 @@ def test_arm_ast2600_evb_sdk(self):
  year = time.strftime("%Y")
  self.ssh_command_output_contains('/sbin/hwclock -f /dev/rtc1', year);
  
-def test_aarch64_ast2700_evb_sdk_v09_01(self):

+def test_aarch64_ast2700_evb_sdk_v09_02(self):
  """
  :avocado: tags=arch:aarch64
  :avocado: tags=machine:ast2700-evb
  """
  
  image_url = ('https://github.com/AspeedTech-BMC/openbmc/releases/'

- 'download/v09.01/ast2700-default-obmc.tar.gz')
-image_hash = 
'b1cc0fd73c7650d34c9c8459a243f52a91e9e27144b8608b2645ab19461d1e07'
+ 'download/v09.02/ast2700-default-obmc.tar.gz')
+image_hash = 
'ac969c2602f4e6bdb69562ff466b89ae3fe1d86e1f6797bb7969d787f82116a7'
  image_path = self.fetch_asset(image_url, asset_hash=image_hash,
algorithm='sha256')
  archive.extract(image_path, self.workdir)

Re: [PATCH v3 3/8] hw/net:ftgmac100: introduce TX and RX ring base address high registers to support 64 bits

[Cédric Le Goater]

On 7/4/24 10:29 AM, Jamin Lin wrote:

ASPEED AST2700 SOC is a 64 bits quad core CPUs (Cortex-a35)
And the base address of dram is "0x4 " which
is 64bits address.

It have "Normal Priority Transmit Ring Base Address Register High(0x17C)",
"High Priority Transmit Ring Base Address Register High(0x184)" and
"Receive Ring Base Address Register High(0x18C)" to save the high part physical
address of descriptor manager.
Ex: TX descriptor manager address [34:0]
The "Normal Priority Transmit Ring Base Address Register High(0x17C)"
bits [2:0] which corresponds the bits [34:32] of the 64 bits address of
the TX ring buffer address.
The "Normal Priority Transmit Ring Base Address Register(0x20)" bits [31:0]
which corresponds the bits [31:0] of the 64 bits address
of the TX ring buffer address.

Introduce a new sub region which size is 0x100 for the set of new registers
and map it at 0x100 in the container region.
This sub region range is from 0x100 to 0x1ff.

Introduce a new property and object attribute to activate the region for new 
registers.
Introduce a new memop handlers for the new register read and write.

Signed-off-by: Jamin Lin 



Reviewed-by: Cédric Le Goater 

Thanks,

C.



---
  hw/net/ftgmac100.c | 82 ++
  include/hw/net/ftgmac100.h |  4 ++
  2 files changed, 86 insertions(+)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index d026242e2b..68956aeb94 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -56,6 +56,16 @@
  #define FTGMAC100_PHYDATA 0x64
  #define FTGMAC100_FCR 0x68
  
+/*

+ * FTGMAC100 registers high
+ *
+ * values below are offset by - FTGMAC100_REG_HIGH_OFFSET from datasheet
+ * because its memory region is start at FTGMAC100_REG_HIGH_OFFSET
+ */
+#define FTGMAC100_NPTXR_BADR_HIGH   (0x17C - FTGMAC100_REG_HIGH_OFFSET)
+#define FTGMAC100_HPTXR_BADR_HIGH   (0x184 - FTGMAC100_REG_HIGH_OFFSET)
+#define FTGMAC100_RXR_BADR_HIGH (0x18C - FTGMAC100_REG_HIGH_OFFSET)
+
  /*
   * Interrupt status register & interrupt enable register
   */
@@ -913,6 +923,60 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
  ftgmac100_update_irq(s);
  }
  
+static uint64_t ftgmac100_high_read(void *opaque, hwaddr addr, unsigned size)

+{
+FTGMAC100State *s = FTGMAC100(opaque);
+uint64_t val = 0;
+
+switch (addr) {
+case FTGMAC100_NPTXR_BADR_HIGH:
+val = extract64(s->tx_ring, 32, 32);
+break;
+case FTGMAC100_HPTXR_BADR_HIGH:
+/* High Priority Transmit Ring Base High Address */
+qemu_log_mask(LOG_UNIMP, "%s: read to unimplemented register 0x%"
+  HWADDR_PRIx "\n", __func__, addr);
+break;
+case FTGMAC100_RXR_BADR_HIGH:
+val = extract64(s->rx_ring, 32, 32);
+break;
+default:
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad address at offset 0x%"
+  HWADDR_PRIx "\n", __func__, addr);
+break;
+}
+
+return val;
+}
+
+static void ftgmac100_high_write(void *opaque, hwaddr addr,
+  uint64_t value, unsigned size)
+{
+FTGMAC100State *s = FTGMAC100(opaque);
+
+switch (addr) {
+case FTGMAC100_NPTXR_BADR_HIGH:
+s->tx_ring = deposit64(s->tx_ring, 32, 32, value);
+s->tx_descriptor = deposit64(s->tx_descriptor, 32, 32, value);
+break;
+case FTGMAC100_HPTXR_BADR_HIGH:
+/* High Priority Transmit Ring Base High Address */
+qemu_log_mask(LOG_UNIMP, "%s: write to unimplemented register 0x%"
+  HWADDR_PRIx "\n", __func__, addr);
+break;
+case FTGMAC100_RXR_BADR_HIGH:
+s->rx_ring = deposit64(s->rx_ring, 32, 32, value);
+s->rx_descriptor = deposit64(s->rx_descriptor, 32, 32, value);
+break;
+default:
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad address at offset 0x%"
+  HWADDR_PRIx "\n", __func__, addr);
+break;
+}
+
+ftgmac100_update_irq(s);
+}
+
  static int ftgmac100_filter(FTGMAC100State *s, const uint8_t *buf, size_t len)
  {
  unsigned mcast_idx;
@@ -1077,6 +1141,14 @@ static const MemoryRegionOps ftgmac100_ops = {
  .endianness = DEVICE_LITTLE_ENDIAN,
  };
  
+static const MemoryRegionOps ftgmac100_high_ops = {

+.read = ftgmac100_high_read,
+.write = ftgmac100_high_write,
+.valid.min_access_size = 4,
+.valid.max_access_size = 4,
+.endianness = DEVICE_LITTLE_ENDIAN,
+};
+
  static void ftgmac100_cleanup(NetClientState *nc)
  {
  FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc));
@@ -1114,6 +1186,15 @@ static void ftgmac100_realize(DeviceState *dev, Error 
**errp)
TYPE_FTGMAC100 ".regs", FTGMAC100_REG_MEM_SIZE);
  memory_region_add_subregion(>iomem_container, 0x0, >iomem);
  
+if (s->dma64) {

+memory_region_init_io(>iomem_high, OBJECT(s), _high_ops,
+  s, TYPE_FTGMAC100 ".regs.high",
+ 

Re: [PATCH v3 1/8] hw/net:ftgmac100: update memory region size to 64KB

[Cédric Le Goater]

On 7/4/24 10:29 AM, Jamin Lin wrote:

According to the datasheet of ASPEED SOCs,
one MAC controller owns 128KB of register space for AST2500.
However, one MAC controller only owns 64KB of register space for AST2600
and AST2700. It set the memory region size 128KB and it occupied another
controllers Address Spaces.

Update one MAC controller memory region size to 0x1000
because AST2500 did not use register spaces over than 64KB.

Introduce a new container region size to 0x1000 and its range
is from 0 to 0xfff. This container is mapped a sub region
for the current set of register.
This sub region range is from 0 to 0xff.

Signed-off-by: Jamin Lin 



Reviewed-by: Cédric Le Goater 

Thanks,

C.



---
  hw/net/ftgmac100.c | 11 ---
  include/hw/net/ftgmac100.h |  4 
  2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 25e4c0cd5b..9e1f12cd33 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -1107,9 +1107,14 @@ static void ftgmac100_realize(DeviceState *dev, Error 
**errp)
  s->rxdes0_edorr = FTGMAC100_RXDES0_EDORR;
  }
  
-memory_region_init_io(>iomem, OBJECT(dev), _ops, s,

-  TYPE_FTGMAC100, 0x2000);
-sysbus_init_mmio(sbd, >iomem);
+memory_region_init(>iomem_container, OBJECT(s),
+   TYPE_FTGMAC100 ".container", FTGMAC100_MEM_SIZE);
+sysbus_init_mmio(sbd, >iomem_container);
+
+memory_region_init_io(>iomem, OBJECT(s), _ops, s,
+  TYPE_FTGMAC100 ".regs", FTGMAC100_REG_MEM_SIZE);
+memory_region_add_subregion(>iomem_container, 0x0, >iomem);
+
  sysbus_init_irq(sbd, >irq);
  qemu_macaddr_default_if_unset(>conf.macaddr);
  
diff --git a/include/hw/net/ftgmac100.h b/include/hw/net/ftgmac100.h

index 765d1538a4..269446e858 100644
--- a/include/hw/net/ftgmac100.h
+++ b/include/hw/net/ftgmac100.h
@@ -14,6 +14,9 @@
  #define TYPE_FTGMAC100 "ftgmac100"
  OBJECT_DECLARE_SIMPLE_TYPE(FTGMAC100State, FTGMAC100)
  
+#define FTGMAC100_MEM_SIZE 0x1000

+#define FTGMAC100_REG_MEM_SIZE 0x100
+
  #include "hw/sysbus.h"
  #include "net/net.h"
  
@@ -30,6 +33,7 @@ struct FTGMAC100State {

  NICState *nic;
  NICConf conf;
  qemu_irq irq;
+MemoryRegion iomem_container;
  MemoryRegion iomem;
  
  uint8_t frame[FTGMAC100_MAX_FRAME_SIZE];

[PATCH v3 5/8] aspeed/soc: set dma64 property for AST2700 ftgmac100

[Jamin Lin via]

ASPEED AST2700 SOC is a 64 bits quad core CPUs (Cortex-a35)
And the base address of dram is "0x4 " which
is 64bits address.

Set dma64 property for ftgmac100 model to support
64bits dram address DMA.

Signed-off-by: Jamin Lin 
---
 hw/arm/aspeed_ast27x0.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index 18e6a8b10c..a9fb0d4b88 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -552,9 +552,12 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, 
Error **errp)
 return;
 }
 
+/* Net */
 for (i = 0; i < sc->macs_num; i++) {
 object_property_set_bool(OBJECT(>ftgmac100[i]), "aspeed", true,
  _abort);
+object_property_set_bool(OBJECT(>ftgmac100[i]), "dma64", true,
+ _abort);
 if (!sysbus_realize(SYS_BUS_DEVICE(>ftgmac100[i]), errp)) {
 return;
 }
-- 
2.34.1

[PATCH v3 8/8] machine_aspeed.py: update to test network for AST2700

[Jamin Lin via]

Update test case to test network connection via SSH.

Test command:
```
cd build
pyvenv/bin/avocado run 
../qemu/tests/avocado/machine_aspeed.py:AST2x00MachineSDK.test_aarch64_ast2700_evb_sdk_v09_02
```

Signed-off-by: Jamin Lin 
---
 tests/avocado/machine_aspeed.py | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/avocado/machine_aspeed.py b/tests/avocado/machine_aspeed.py
index 13fe128fc9..f66ad38d35 100644
--- a/tests/avocado/machine_aspeed.py
+++ b/tests/avocado/machine_aspeed.py
@@ -313,14 +313,14 @@ def do_test_arm_aspeed_sdk_start(self, image):
 
 def do_test_aarch64_aspeed_sdk_start(self, image):
 self.vm.set_console()
-self.vm.add_args('-drive', 'file=' + image + ',if=mtd,format=raw')
+self.vm.add_args('-drive', 'file=' + image + ',if=mtd,format=raw',
+ '-net', 'nic', '-net', 
'user,hostfwd=:127.0.0.1:0-:22')
 
 self.vm.launch()
 
 self.wait_for_console_pattern('U-Boot 2023.10')
 self.wait_for_console_pattern('## Loading kernel from FIT Image')
 self.wait_for_console_pattern('Starting kernel ...')
-self.wait_for_console_pattern("systemd[1]: Hostname set to")
 
 @skipUnless(os.getenv('QEMU_TEST_FLAKY_TESTS'), 'Test is unstable on 
GitLab')
 
@@ -436,4 +436,6 @@ def test_aarch64_ast2700_evb_sdk_v09_02(self):
 
 self.vm.add_args('-smp', str(num_cpu))
 self.do_test_aarch64_aspeed_sdk_start(image_dir + 'image-bmc')
+self.wait_for_console_pattern('nodistro.0 ast2700-default ttyS12')
+self.ssh_connect('root', '0penBmc', False)
 
-- 
2.34.1

[PATCH v3 6/8] hw/block: m25p80: support quad mode for w25q01jvq

[Jamin Lin via]

According to the w25q01jv datasheet at page 16,
it is required to set QE bit in "Status Register 2".
Besides, users are able to utilize "Write Status Register 1(0x01)"
command to set QE bit in "Status Register 2" and
utilize "Read Status Register 2(0x35)" command to get the QE bit status.

To support quad mode for w25q01jvq, update collecting data needed
2 bytes for WRSR command in decode_new_cmd function and
verify QE bit at the second byte of collecting data bit 2
in complete_collecting_data.

Update RDCR_EQIO command to set bit 2 of return data
if quad mode enable in decode_new_cmd.

Signed-off-by: Troy Lee 
Signed-off-by: Jamin Lin 
Reviewed-by: Cédric Le Goater 
---
 hw/block/m25p80.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 8dec134832..9e99107b42 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -416,6 +416,7 @@ typedef enum {
 /*
  * Micron: 0x35 - enable QPI
  * Spansion: 0x35 - read control register
+ * Winbond: 0x35 - quad enable
  */
 RDCR_EQIO = 0x35,
 RSTQIO = 0xf5,
@@ -798,6 +799,11 @@ static void complete_collecting_data(Flash *s)
 s->four_bytes_address_mode = extract32(s->data[1], 5, 1);
 }
 break;
+case MAN_WINBOND:
+if (s->len > 1) {
+s->quad_enable = !!(s->data[1] & 0x02);
+}
+break;
 default:
 break;
 }
@@ -1254,6 +1260,10 @@ static void decode_new_cmd(Flash *s, uint32_t value)
 s->needed_bytes = 2;
 s->state = STATE_COLLECTING_VAR_LEN_DATA;
 break;
+case MAN_WINBOND:
+s->needed_bytes = 2;
+s->state = STATE_COLLECTING_VAR_LEN_DATA;
+break;
 default:
 s->needed_bytes = 1;
 s->state = STATE_COLLECTING_DATA;
@@ -1431,6 +1441,12 @@ static void decode_new_cmd(Flash *s, uint32_t value)
 case MAN_MACRONIX:
 s->quad_enable = true;
 break;
+case MAN_WINBOND:
+s->data[0] = (!!s->quad_enable) << 1;
+s->pos = 0;
+s->len = 1;
+s->state = STATE_READING_DATA;
+break;
 default:
 break;
 }
-- 
2.34.1

[PATCH v3 4/8] hw/net:ftgmac100: update TX and RX packet buffers address to 64 bits

[Jamin Lin via]

ASPEED AST2700 SOC is a 64 bits quad core CPUs (Cortex-a35)
And the base address of dram is "0x4 " which
is 64bits address.

It have "TXDES 2" and "RXDES 2" to save the high part
physical address of packet buffer.
Ex: TX packet buffer address [34:0]
The "TXDES 2" bits [18:16] which corresponds the bits [34:32]
of the 64 bits address of the TX packet buffer address
and "TXDES 3" bits [31:0] which corresponds the bits [31:0]
of the 64 bits address of the TX packet buffer address.

Update TX and RX packet buffers address type to
64 bits for dram 64 bits address DMA support.

Signed-off-by: Jamin Lin 
---
 hw/net/ftgmac100.c | 21 ++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 68956aeb94..80f9cd56d5 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -175,6 +175,8 @@
 #define FTGMAC100_TXDES1_TX2FIC  (1 << 30)
 #define FTGMAC100_TXDES1_TXIC(1 << 31)
 
+#define FTGMAC100_TXDES2_TXBUF_BADR_HI(x)   (((x) >> 16) & 0x7)
+
 /*
  * Receive descriptor
  */
@@ -208,13 +210,15 @@
 #define FTGMAC100_RXDES1_UDP_CHKSUM_ERR  (1 << 26)
 #define FTGMAC100_RXDES1_IP_CHKSUM_ERR   (1 << 27)
 
+#define FTGMAC100_RXDES2_RXBUF_BADR_HI(x)   (((x) >> 16) & 0x7)
+
 /*
  * Receive and transmit Buffer Descriptor
  */
 typedef struct {
 uint32_tdes0;
 uint32_tdes1;
-uint32_tdes2;/* not used by HW */
+uint32_tdes2;/* used by HW 64 bits DMA */
 uint32_tdes3;
 } FTGMAC100Desc;
 
@@ -531,6 +535,7 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint64_t 
tx_ring,
 int frame_size = 0;
 uint8_t *ptr = s->frame;
 uint64_t addr = tx_descriptor;
+uint64_t buf_addr = 0;
 uint32_t flags = 0;
 
 while (1) {
@@ -569,7 +574,12 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint64_t 
tx_ring,
 len =  sizeof(s->frame) - frame_size;
 }
 
-if (dma_memory_read(_space_memory, bd.des3,
+buf_addr = bd.des3;
+if (s->dma64) {
+buf_addr = deposit64(buf_addr, 32, 32,
+ FTGMAC100_TXDES2_TXBUF_BADR_HI(bd.des2));
+}
+if (dma_memory_read(_space_memory, buf_addr,
 ptr, len, MEMTXATTRS_UNSPECIFIED)) {
 qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to read packet @ 
0x%x\n",
   __func__, bd.des3);
@@ -1022,7 +1032,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, 
const uint8_t *buf,
 uint32_t flags = 0;
 uint64_t addr;
 uint32_t crc;
-uint32_t buf_addr;
+uint64_t buf_addr = 0;
 uint8_t *crc_ptr;
 uint32_t buf_len;
 size_t size = len;
@@ -1087,7 +1097,12 @@ static ssize_t ftgmac100_receive(NetClientState *nc, 
const uint8_t *buf,
 if (size < 4) {
 buf_len += size - 4;
 }
+
 buf_addr = bd.des3;
+if (s->dma64) {
+buf_addr = deposit64(buf_addr, 32, 32,
+ FTGMAC100_RXDES2_RXBUF_BADR_HI(bd.des2));
+}
 if (first && proto == ETH_P_VLAN && buf_len >= 18) {
 bd.des1 = lduw_be_p(buf + 14) | FTGMAC100_RXDES1_VLANTAG_AVAIL;
 
-- 
2.34.1

[PATCH v3 7/8] machine_aspeed.py: update to test ASPEED OpenBMC SDK v09.02 for AST2700

[Jamin Lin via]

Update test case to test ASPEED OpenBMC SDK v09.02 for AST2700.

ASPEED fixed TX mask issue from linux/drivers/ftgmac100.c.
It is required to use ASPEED OpenBMC SDK since v09.02
for AST2700 QEMU network testing.

A test image is downloaded from the ASPEED Forked OpenBMC GitHub
release repository :
https://github.com/AspeedTech-BMC/openbmc/releases/

Signed-off-by: Jamin Lin 
---
 tests/avocado/machine_aspeed.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/avocado/machine_aspeed.py b/tests/avocado/machine_aspeed.py
index 3a20644fb2..13fe128fc9 100644
--- a/tests/avocado/machine_aspeed.py
+++ b/tests/avocado/machine_aspeed.py
@@ -387,15 +387,15 @@ def test_arm_ast2600_evb_sdk(self):
 year = time.strftime("%Y")
 self.ssh_command_output_contains('/sbin/hwclock -f /dev/rtc1', year);
 
-def test_aarch64_ast2700_evb_sdk_v09_01(self):
+def test_aarch64_ast2700_evb_sdk_v09_02(self):
 """
 :avocado: tags=arch:aarch64
 :avocado: tags=machine:ast2700-evb
 """
 
 image_url = ('https://github.com/AspeedTech-BMC/openbmc/releases/'
- 'download/v09.01/ast2700-default-obmc.tar.gz')
-image_hash = 
'b1cc0fd73c7650d34c9c8459a243f52a91e9e27144b8608b2645ab19461d1e07'
+ 'download/v09.02/ast2700-default-obmc.tar.gz')
+image_hash = 
'ac969c2602f4e6bdb69562ff466b89ae3fe1d86e1f6797bb7969d787f82116a7'
 image_path = self.fetch_asset(image_url, asset_hash=image_hash,
   algorithm='sha256')
 archive.extract(image_path, self.workdir)
-- 
2.34.1

[PATCH v3 1/8] hw/net:ftgmac100: update memory region size to 64KB

[Jamin Lin via]

According to the datasheet of ASPEED SOCs,
one MAC controller owns 128KB of register space for AST2500.
However, one MAC controller only owns 64KB of register space for AST2600
and AST2700. It set the memory region size 128KB and it occupied another
controllers Address Spaces.

Update one MAC controller memory region size to 0x1000
because AST2500 did not use register spaces over than 64KB.

Introduce a new container region size to 0x1000 and its range
is from 0 to 0xfff. This container is mapped a sub region
for the current set of register.
This sub region range is from 0 to 0xff.

Signed-off-by: Jamin Lin 
---
 hw/net/ftgmac100.c | 11 ---
 include/hw/net/ftgmac100.h |  4 
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 25e4c0cd5b..9e1f12cd33 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -1107,9 +1107,14 @@ static void ftgmac100_realize(DeviceState *dev, Error 
**errp)
 s->rxdes0_edorr = FTGMAC100_RXDES0_EDORR;
 }
 
-memory_region_init_io(>iomem, OBJECT(dev), _ops, s,
-  TYPE_FTGMAC100, 0x2000);
-sysbus_init_mmio(sbd, >iomem);
+memory_region_init(>iomem_container, OBJECT(s),
+   TYPE_FTGMAC100 ".container", FTGMAC100_MEM_SIZE);
+sysbus_init_mmio(sbd, >iomem_container);
+
+memory_region_init_io(>iomem, OBJECT(s), _ops, s,
+  TYPE_FTGMAC100 ".regs", FTGMAC100_REG_MEM_SIZE);
+memory_region_add_subregion(>iomem_container, 0x0, >iomem);
+
 sysbus_init_irq(sbd, >irq);
 qemu_macaddr_default_if_unset(>conf.macaddr);
 
diff --git a/include/hw/net/ftgmac100.h b/include/hw/net/ftgmac100.h
index 765d1538a4..269446e858 100644
--- a/include/hw/net/ftgmac100.h
+++ b/include/hw/net/ftgmac100.h
@@ -14,6 +14,9 @@
 #define TYPE_FTGMAC100 "ftgmac100"
 OBJECT_DECLARE_SIMPLE_TYPE(FTGMAC100State, FTGMAC100)
 
+#define FTGMAC100_MEM_SIZE 0x1000
+#define FTGMAC100_REG_MEM_SIZE 0x100
+
 #include "hw/sysbus.h"
 #include "net/net.h"
 
@@ -30,6 +33,7 @@ struct FTGMAC100State {
 NICState *nic;
 NICConf conf;
 qemu_irq irq;
+MemoryRegion iomem_container;
 MemoryRegion iomem;
 
 uint8_t frame[FTGMAC100_MAX_FRAME_SIZE];
-- 
2.34.1

[PATCH v3 2/8] hw/net:ftgmac100: update ring base address to 64 bits

[Jamin Lin via]

Update TX and RX ring base address data type to uint64_t for
64 bits dram address DMA support.

Both "Normal Priority Transmit Ring Base Address Register(0x20)" and
"Receive Ring Base Address Register (0x24)" are used for saving the
low part physical address of descriptor manager.

Therefore, changes to set TX and RX descriptor manager address bits [31:0]
in ftgmac100_read and ftgmac100_write functions.

Incrementing the version of vmstate to 2.

Signed-off-by: Jamin Lin 
---
 hw/net/ftgmac100.c | 33 -
 include/hw/net/ftgmac100.h |  9 -
 2 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 9e1f12cd33..d026242e2b 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -515,12 +515,12 @@ out:
 return frame_size;
 }
 
-static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
-uint32_t tx_descriptor)
+static void ftgmac100_do_tx(FTGMAC100State *s, uint64_t tx_ring,
+uint64_t tx_descriptor)
 {
 int frame_size = 0;
 uint8_t *ptr = s->frame;
-uint32_t addr = tx_descriptor;
+uint64_t addr = tx_descriptor;
 uint32_t flags = 0;
 
 while (1) {
@@ -726,9 +726,9 @@ static uint64_t ftgmac100_read(void *opaque, hwaddr addr, 
unsigned size)
 case FTGMAC100_MATH1:
 return s->math[1];
 case FTGMAC100_RXR_BADR:
-return s->rx_ring;
+return extract64(s->rx_ring, 0, 32);
 case FTGMAC100_NPTXR_BADR:
-return s->tx_ring;
+return extract64(s->tx_ring, 0, 32);
 case FTGMAC100_ITC:
 return s->itc;
 case FTGMAC100_DBLAC:
@@ -799,9 +799,8 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
   HWADDR_PRIx "\n", __func__, value);
 return;
 }
-
-s->rx_ring = value;
-s->rx_descriptor = s->rx_ring;
+s->rx_ring = deposit64(s->rx_ring, 0, 32, value);
+s->rx_descriptor = deposit64(s->rx_descriptor, 0, 32, value);
 break;
 
 case FTGMAC100_RBSR: /* DMA buffer size */
@@ -814,8 +813,8 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
   HWADDR_PRIx "\n", __func__, value);
 return;
 }
-s->tx_ring = value;
-s->tx_descriptor = s->tx_ring;
+s->tx_ring = deposit64(s->tx_ring, 0, 32, value);
+s->tx_descriptor = deposit64(s->tx_descriptor, 0, 32, value);
 break;
 
 case FTGMAC100_NPTXPD: /* Trigger transmit */
@@ -957,7 +956,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const 
uint8_t *buf,
 FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc));
 FTGMAC100Desc bd;
 uint32_t flags = 0;
-uint32_t addr;
+uint64_t addr;
 uint32_t crc;
 uint32_t buf_addr;
 uint8_t *crc_ptr;
@@ -1126,18 +1125,14 @@ static void ftgmac100_realize(DeviceState *dev, Error 
**errp)
 
 static const VMStateDescription vmstate_ftgmac100 = {
 .name = TYPE_FTGMAC100,
-.version_id = 1,
-.minimum_version_id = 1,
+.version_id = 2,
+.minimum_version_id = 2,
 .fields = (const VMStateField[]) {
 VMSTATE_UINT32(irq_state, FTGMAC100State),
 VMSTATE_UINT32(isr, FTGMAC100State),
 VMSTATE_UINT32(ier, FTGMAC100State),
 VMSTATE_UINT32(rx_enabled, FTGMAC100State),
-VMSTATE_UINT32(rx_ring, FTGMAC100State),
 VMSTATE_UINT32(rbsr, FTGMAC100State),
-VMSTATE_UINT32(tx_ring, FTGMAC100State),
-VMSTATE_UINT32(rx_descriptor, FTGMAC100State),
-VMSTATE_UINT32(tx_descriptor, FTGMAC100State),
 VMSTATE_UINT32_ARRAY(math, FTGMAC100State, 2),
 VMSTATE_UINT32(itc, FTGMAC100State),
 VMSTATE_UINT32(aptcr, FTGMAC100State),
@@ -1156,6 +1151,10 @@ static const VMStateDescription vmstate_ftgmac100 = {
 VMSTATE_UINT32(phy_int_mask, FTGMAC100State),
 VMSTATE_UINT32(txdes0_edotr, FTGMAC100State),
 VMSTATE_UINT32(rxdes0_edorr, FTGMAC100State),
+VMSTATE_UINT64(rx_ring, FTGMAC100State),
+VMSTATE_UINT64(tx_ring, FTGMAC100State),
+VMSTATE_UINT64(rx_descriptor, FTGMAC100State),
+VMSTATE_UINT64(tx_descriptor, FTGMAC100State),
 VMSTATE_END_OF_LIST()
 }
 };
diff --git a/include/hw/net/ftgmac100.h b/include/hw/net/ftgmac100.h
index 269446e858..aae57ae8cb 100644
--- a/include/hw/net/ftgmac100.h
+++ b/include/hw/net/ftgmac100.h
@@ -42,10 +42,6 @@ struct FTGMAC100State {
 uint32_t isr;
 uint32_t ier;
 uint32_t rx_enabled;
-uint32_t rx_ring;
-uint32_t rx_descriptor;
-uint32_t tx_ring;
-uint32_t tx_descriptor;
 uint32_t math[2];
 uint32_t rbsr;
 uint32_t itc;
@@ -58,7 +54,10 @@ struct FTGMAC100State {
 uint32_t phycr;
 uint32_t phydata;
 uint32_t fcr;
-
+uint64_t rx_ring;
+uint64_t rx_descriptor;
+uint64_t tx_ring;
+uint64_t tx_descriptor;
 
 uint32_t phy_status;
 uint32_t 

[PATCH v3 0/8] support AST2700 network

[Jamin Lin via]

change from v1:
- ftgmac100
 - fix coding style
 - support 64 bits dma dram address for AST2700

change from v2:
- ftgmac100: update memory region size to 0x200.
- ftgmac100: introduce a new class(ftgmac100_high),
class attribute and memop handlers, for FTGMAC100_*_HIGH regs read/write.
- aspeed_ast27x0: update network model to ftgmac100_high to support
  64 bits dram address DMA.
- m25p80: support quad mode for w25q01jvq

change from v3:
- ftgmac100: update memory region size to 64KB.
- ftgmac100: using a property to activate the region for new registers,
  instead of a class
- ftgmac100: introduce TX and RX ring base address high registers
- ftgmac100: split standalone patch for easy review
- ftgmac100: update TX and RX packet buffers address to 64 bits
- aspeed_ast27x0: set dma64 property for AST2700 ftgmac100
- machine_aspeed.py: update to test sdk v09.02 and network for AST2700

Jamin Lin (8):
  hw/net:ftgmac100: update memory region size to 64KB
  hw/net:ftgmac100: update ring base address to 64 bits
  hw/net:ftgmac100: introduce TX and RX ring base address high registers
to support 64 bits
  hw/net:ftgmac100: update TX and RX packet buffers address to 64 bits
  aspeed/soc: set dma64 property for AST2700 ftgmac100
  hw/block: m25p80: support quad mode for w25q01jvq
  machine_aspeed.py: update to test ASPEED OpenBMC SDK v09.02 for
AST2700
  machine_aspeed.py: update to test network for AST2700

 hw/arm/aspeed_ast27x0.c |   3 +
 hw/block/m25p80.c   |  16 
 hw/net/ftgmac100.c  | 147 +++-
 include/hw/net/ftgmac100.h  |  17 ++--
 tests/avocado/machine_aspeed.py |  12 +--
 5 files changed, 162 insertions(+), 33 deletions(-)

-- 
2.34.1

[PATCH v3 3/8] hw/net:ftgmac100: introduce TX and RX ring base address high registers to support 64 bits

[Jamin Lin via]

ASPEED AST2700 SOC is a 64 bits quad core CPUs (Cortex-a35)
And the base address of dram is "0x4 " which
is 64bits address.

It have "Normal Priority Transmit Ring Base Address Register High(0x17C)",
"High Priority Transmit Ring Base Address Register High(0x184)" and
"Receive Ring Base Address Register High(0x18C)" to save the high part physical
address of descriptor manager.
Ex: TX descriptor manager address [34:0]
The "Normal Priority Transmit Ring Base Address Register High(0x17C)"
bits [2:0] which corresponds the bits [34:32] of the 64 bits address of
the TX ring buffer address.
The "Normal Priority Transmit Ring Base Address Register(0x20)" bits [31:0]
which corresponds the bits [31:0] of the 64 bits address
of the TX ring buffer address.

Introduce a new sub region which size is 0x100 for the set of new registers
and map it at 0x100 in the container region.
This sub region range is from 0x100 to 0x1ff.

Introduce a new property and object attribute to activate the region for new 
registers.
Introduce a new memop handlers for the new register read and write.

Signed-off-by: Jamin Lin 
---
 hw/net/ftgmac100.c | 82 ++
 include/hw/net/ftgmac100.h |  4 ++
 2 files changed, 86 insertions(+)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index d026242e2b..68956aeb94 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -56,6 +56,16 @@
 #define FTGMAC100_PHYDATA 0x64
 #define FTGMAC100_FCR 0x68
 
+/*
+ * FTGMAC100 registers high
+ *
+ * values below are offset by - FTGMAC100_REG_HIGH_OFFSET from datasheet
+ * because its memory region is start at FTGMAC100_REG_HIGH_OFFSET
+ */
+#define FTGMAC100_NPTXR_BADR_HIGH   (0x17C - FTGMAC100_REG_HIGH_OFFSET)
+#define FTGMAC100_HPTXR_BADR_HIGH   (0x184 - FTGMAC100_REG_HIGH_OFFSET)
+#define FTGMAC100_RXR_BADR_HIGH (0x18C - FTGMAC100_REG_HIGH_OFFSET)
+
 /*
  * Interrupt status register & interrupt enable register
  */
@@ -913,6 +923,60 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
 ftgmac100_update_irq(s);
 }
 
+static uint64_t ftgmac100_high_read(void *opaque, hwaddr addr, unsigned size)
+{
+FTGMAC100State *s = FTGMAC100(opaque);
+uint64_t val = 0;
+
+switch (addr) {
+case FTGMAC100_NPTXR_BADR_HIGH:
+val = extract64(s->tx_ring, 32, 32);
+break;
+case FTGMAC100_HPTXR_BADR_HIGH:
+/* High Priority Transmit Ring Base High Address */
+qemu_log_mask(LOG_UNIMP, "%s: read to unimplemented register 0x%"
+  HWADDR_PRIx "\n", __func__, addr);
+break;
+case FTGMAC100_RXR_BADR_HIGH:
+val = extract64(s->rx_ring, 32, 32);
+break;
+default:
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad address at offset 0x%"
+  HWADDR_PRIx "\n", __func__, addr);
+break;
+}
+
+return val;
+}
+
+static void ftgmac100_high_write(void *opaque, hwaddr addr,
+  uint64_t value, unsigned size)
+{
+FTGMAC100State *s = FTGMAC100(opaque);
+
+switch (addr) {
+case FTGMAC100_NPTXR_BADR_HIGH:
+s->tx_ring = deposit64(s->tx_ring, 32, 32, value);
+s->tx_descriptor = deposit64(s->tx_descriptor, 32, 32, value);
+break;
+case FTGMAC100_HPTXR_BADR_HIGH:
+/* High Priority Transmit Ring Base High Address */
+qemu_log_mask(LOG_UNIMP, "%s: write to unimplemented register 0x%"
+  HWADDR_PRIx "\n", __func__, addr);
+break;
+case FTGMAC100_RXR_BADR_HIGH:
+s->rx_ring = deposit64(s->rx_ring, 32, 32, value);
+s->rx_descriptor = deposit64(s->rx_descriptor, 32, 32, value);
+break;
+default:
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad address at offset 0x%"
+  HWADDR_PRIx "\n", __func__, addr);
+break;
+}
+
+ftgmac100_update_irq(s);
+}
+
 static int ftgmac100_filter(FTGMAC100State *s, const uint8_t *buf, size_t len)
 {
 unsigned mcast_idx;
@@ -1077,6 +1141,14 @@ static const MemoryRegionOps ftgmac100_ops = {
 .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+static const MemoryRegionOps ftgmac100_high_ops = {
+.read = ftgmac100_high_read,
+.write = ftgmac100_high_write,
+.valid.min_access_size = 4,
+.valid.max_access_size = 4,
+.endianness = DEVICE_LITTLE_ENDIAN,
+};
+
 static void ftgmac100_cleanup(NetClientState *nc)
 {
 FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc));
@@ -1114,6 +1186,15 @@ static void ftgmac100_realize(DeviceState *dev, Error 
**errp)
   TYPE_FTGMAC100 ".regs", FTGMAC100_REG_MEM_SIZE);
 memory_region_add_subregion(>iomem_container, 0x0, >iomem);
 
+if (s->dma64) {
+memory_region_init_io(>iomem_high, OBJECT(s), _high_ops,
+  s, TYPE_FTGMAC100 ".regs.high",
+  FTGMAC100_REG_HIGH_MEM_SIZE);
+memory_region_add_subregion(>iomem_container,
+  

Re: [PATCH v3] virtio: Implement Virtio Backend for SD/MMC in QEMU

[Ми]

> 4 июля 2024 г., в 10:38, Michael S. Tsirkin  написал(а):
> 
> On Thu, Jul 04, 2024 at 10:25:53AM +0300, Mikhail Krasheninnikov wrote:
>> 
>> On Wed, 3 Jul 2024, Michael S. Tsirkin wrote:
>> 
>>> On Wed, Jul 03, 2024 at 10:55:17PM +0300, Mikhail Krasheninnikov wrote:
 
 Hello, Alex!
 
 No, there's no patch to the VirtIO specification yet. This is 
 proof-of-concept solution since I'm not sure that I did everything 
 correct with the design (and as folks' reviews show, for a good reason). 
 As soon as most obvious issues would be out of the way, I think I'll 
 submit a patch.
>>> 
>>> 
>>> Mikhail, if you want people to review your patches but not merge
>>> them yet, pls use an RFC tag in the subject to avoid confusion.
>>> 
>>> Thanks,
>>> 
>>> -- 
>>> MST
>>> 
>>> 
>> 
>> Hello, Michael!
>> 
>> I was planning to submit three patches: to the kernel, emulator and Virtio 
>> specification around the same time - as soon as the obvious bugs are 
>> fixed, I'll submit a patch to the specification. I thought it wasn't 
>> necessary to use the RFC tag in that case, but if you think it is, 
>> I'll include it with the next version of the patch.
> 
> RFC means "this is proof of concept". On the one hand some people
> won't bother reviewing then. On the other your patch will be
> judged less harshly. If your code still has debugging printks,
> it's clearly an RFC at best.
> 
> -- 
> MST
> 

I apologize for the debug printks in other patch, I’m not really sure
how I gazed over it.. If it makes my situation better, it’s my first series
of patches. I’ll make sure to triple-check next time. Thanks for the
feedback!

Re: [PATCH v3] virtio: Implement Virtio Backend for SD/MMC in QEMU

[Michael S. Tsirkin]

On Thu, Jul 04, 2024 at 10:25:53AM +0300, Mikhail Krasheninnikov wrote:
> 
> On Wed, 3 Jul 2024, Michael S. Tsirkin wrote:
> 
> > On Wed, Jul 03, 2024 at 10:55:17PM +0300, Mikhail Krasheninnikov wrote:
> > > 
> > > Hello, Alex!
> > > 
> > > No, there's no patch to the VirtIO specification yet. This is 
> > > proof-of-concept solution since I'm not sure that I did everything 
> > > correct with the design (and as folks' reviews show, for a good reason). 
> > > As soon as most obvious issues would be out of the way, I think I'll 
> > > submit a patch.
> > 
> > 
> > Mikhail, if you want people to review your patches but not merge
> > them yet, pls use an RFC tag in the subject to avoid confusion.
> > 
> > Thanks,
> > 
> > -- 
> > MST
> > 
> > 
> 
> Hello, Michael!
> 
> I was planning to submit three patches: to the kernel, emulator and Virtio 
> specification around the same time - as soon as the obvious bugs are 
> fixed, I'll submit a patch to the specification. I thought it wasn't 
> necessary to use the RFC tag in that case, but if you think it is, 
> I'll include it with the next version of the patch.

RFC means "this is proof of concept". On the one hand some people
won't bother reviewing then. On the other your patch will be
judged less harshly. If your code still has debugging printks,
it's clearly an RFC at best.

-- 
MST

Re: [PATCH v3] virtio: Implement Virtio Backend for SD/MMC in QEMU

[Mikhail Krasheninnikov]

On Wed, 3 Jul 2024, Michael S. Tsirkin wrote:

> On Wed, Jul 03, 2024 at 10:55:17PM +0300, Mikhail Krasheninnikov wrote:
> > 
> > Hello, Alex!
> > 
> > No, there's no patch to the VirtIO specification yet. This is 
> > proof-of-concept solution since I'm not sure that I did everything 
> > correct with the design (and as folks' reviews show, for a good reason). 
> > As soon as most obvious issues would be out of the way, I think I'll 
> > submit a patch.
> 
> 
> Mikhail, if you want people to review your patches but not merge
> them yet, pls use an RFC tag in the subject to avoid confusion.
> 
> Thanks,
> 
> -- 
> MST
> 
> 

Hello, Michael!

I was planning to submit three patches: to the kernel, emulator and Virtio 
specification around the same time - as soon as the obvious bugs are 
fixed, I'll submit a patch to the specification. I thought it wasn't 
necessary to use the RFC tag in that case, but if you think it is, 
I'll include it with the next version of the patch.