type mismatch in SSDT

type mismatch in SSDT

[Michael S. Tsirkin]

Just noticed this when disassembling:

Parsing completed
ACPI Warning: NsLookup: Type mismatch on ODAT (RegionField), searching for (Buffer) (20210604/nsaccess-760)
Disassembly completed
ASL Output:    /tmp/old-asl2/tests/data/acpi/virt/SSDT.memhp.dsl - 14945 bytes

Did not look into this yet but it seems new.

-- 
MST

Re: type mismatch in SSDT

[Igor Mammedov]

On Thu, 27 Oct 2022 01:59:22 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Just noticed this when disassembling:
> 
> Parsing completed
> ACPI Warning: NsLookup: Type mismatch on ODAT (RegionField), searching for (Buffer) (20210604/nsaccess-760)
> Disassembly completed
> ASL Output:    /tmp/old-asl2/tests/data/acpi/virt/SSDT.memhp.dsl - 14945 bytes
> 
> Did not look into this yet but it seems new.
It was there practically 'forever'.

ODAT should be treated as Buffer according to implicit Field/data conversion rules,
that's probably the reason why it works. So warning looks a bit bogus to me.

however:
  DefCreateByteField := CreateByteFieldOp SourceBuff ByteIndex NameString
  SourceBuff := TermArg => Buffer
  TermArg := ExpressionOpcode | DataObject | ArgObj | LocalObj

and none of that explicitly leads to 

  TermObj := Object | StatementOpcode | ExpressionOpcode
  Object := NameSpaceModifierObj | NamedObj

So if we are to be as pedantic as IASL, we need to supply
field to CreateByteField not by name but via one of TermArg.
We could copy/assign whole buffer to a LocalObj
or summarily use ExpressionOpcode => ToBuffer() // this one has a bit controversial definition in 6.4 spec
or to avoid any copying add 'useless' DerefOf(RefOf())
wrapper around name to make argument of ExpressionOpcode kind.

following should silence warning.

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 31e46df0bd..7488007540 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -1127,7 +1127,7 @@ static void nvdimm_build_common_dsm(Aml *dev,
     /* If RLEN >= Integer size, just use CreateField() operator */
     aml_append(method, aml_store(aml_shiftleft(dsm_out_buf_size, aml_int(3)),
                                  dsm_out_buf_size));
-    aml_append(method, aml_create_field(aml_name(NVDIMM_DSM_OUT_BUF),
+    aml_append(method, aml_create_field(aml_derefof(aml_refof(aml_name(NVDIMM_DSM_OUT_BUF))),
                aml_int(0), dsm_out_buf_size, "OBUF"));
     aml_append(method, aml_return(aml_name("OBUF")));
 

Re: type mismatch in SSDT

[Michael S. Tsirkin]

On Thu, Oct 27, 2022 at 03:52:53PM +0200, Igor Mammedov wrote:
> On Thu, 27 Oct 2022 01:59:22 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > Just noticed this when disassembling:
> > 
> > Parsing completed
> > ACPI Warning: NsLookup: Type mismatch on ODAT (RegionField), searching for (Buffer) (20210604/nsaccess-760)
> > Disassembly completed
> > ASL Output:    /tmp/old-asl2/tests/data/acpi/virt/SSDT.memhp.dsl - 14945 bytes
> > 
> > Did not look into this yet but it seems new.
> It was there practically 'forever'.
> 
> ODAT should be treated as Buffer according to implicit Field/data conversion rules,
> that's probably the reason why it works. So warning looks a bit bogus to me.
> 
> however:
>   DefCreateByteField := CreateByteFieldOp SourceBuff ByteIndex NameString
>   SourceBuff := TermArg => Buffer
>   TermArg := ExpressionOpcode | DataObject | ArgObj | LocalObj
> 
> and none of that explicitly leads to 
> 
>   TermObj := Object | StatementOpcode | ExpressionOpcode
>   Object := NameSpaceModifierObj | NamedObj
> 
> So if we are to be as pedantic as IASL, we need to supply
> field to CreateByteField not by name but via one of TermArg.
> We could copy/assign whole buffer to a LocalObj
> or summarily use ExpressionOpcode => ToBuffer() // this one has a bit controversial definition in 6.4 spec
> or to avoid any copying add 'useless' DerefOf(RefOf())
> wrapper around name to make argument of ExpressionOpcode kind.
> 
> following should silence warning.
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 31e46df0bd..7488007540 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -1127,7 +1127,7 @@ static void nvdimm_build_common_dsm(Aml *dev,
>      /* If RLEN >= Integer size, just use CreateField() operator */
>      aml_append(method, aml_store(aml_shiftleft(dsm_out_buf_size, aml_int(3)),
>                                   dsm_out_buf_size));
> -    aml_append(method, aml_create_field(aml_name(NVDIMM_DSM_OUT_BUF),
> +    aml_append(method, aml_create_field(aml_derefof(aml_refof(aml_name(NVDIMM_DSM_OUT_BUF))),
>                 aml_int(0), dsm_out_buf_size, "OBUF"));
>      aml_append(method, aml_return(aml_name("OBUF")));


Thanks! Let's try to raise this with ACPI committee?

-- 
MST

Re: type mismatch in SSDT

[Igor Mammedov]

On Thu, 27 Oct 2022 10:15:58 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Oct 27, 2022 at 03:52:53PM +0200, Igor Mammedov wrote:
> > On Thu, 27 Oct 2022 01:59:22 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > Just noticed this when disassembling:
> > > 
> > > Parsing completed
> > > ACPI Warning: NsLookup: Type mismatch on ODAT (RegionField), searching for (Buffer) (20210604/nsaccess-760)
> > > Disassembly completed
> > > ASL Output:    /tmp/old-asl2/tests/data/acpi/virt/SSDT.memhp.dsl - 14945 bytes
> > > 
> > > Did not look into this yet but it seems new.  
> > It was there practically 'forever'.
> > 
> > ODAT should be treated as Buffer according to implicit Field/data conversion rules,
> > that's probably the reason why it works. So warning looks a bit bogus to me.
> > 
> > however:
> >   DefCreateByteField := CreateByteFieldOp SourceBuff ByteIndex NameString
> >   SourceBuff := TermArg => Buffer
> >   TermArg := ExpressionOpcode | DataObject | ArgObj | LocalObj
> > 
> > and none of that explicitly leads to 
> > 
> >   TermObj := Object | StatementOpcode | ExpressionOpcode
> >   Object := NameSpaceModifierObj | NamedObj
> > 
> > So if we are to be as pedantic as IASL, we need to supply
> > field to CreateByteField not by name but via one of TermArg.
> > We could copy/assign whole buffer to a LocalObj
> > or summarily use ExpressionOpcode => ToBuffer() // this one has a bit controversial definition in 6.4 spec
> > or to avoid any copying add 'useless' DerefOf(RefOf())
> > wrapper around name to make argument of ExpressionOpcode kind.
> > 
> > following should silence warning.
> > 
> > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > index 31e46df0bd..7488007540 100644
> > --- a/hw/acpi/nvdimm.c
> > +++ b/hw/acpi/nvdimm.c
> > @@ -1127,7 +1127,7 @@ static void nvdimm_build_common_dsm(Aml *dev,
> >      /* If RLEN >= Integer size, just use CreateField() operator */
> >      aml_append(method, aml_store(aml_shiftleft(dsm_out_buf_size, aml_int(3)),
> >                                   dsm_out_buf_size));
> > -    aml_append(method, aml_create_field(aml_name(NVDIMM_DSM_OUT_BUF),
> > +    aml_append(method, aml_create_field(aml_derefof(aml_refof(aml_name(NVDIMM_DSM_OUT_BUF))),
> >                 aml_int(0), dsm_out_buf_size, "OBUF"));
> >      aml_append(method, aml_return(aml_name("OBUF")));  
> 
> 
> Thanks! Let's try to raise this with ACPI committee?

Do you have a contact there or know how to rise the issue?