Re: [Qemu-devel] [PATCH 4/9] acpi: build_append_nameseg(): add padding if necessary

[Igor Mammedov]

On Tue, 9 Dec 2014 12:38:12 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

> On Tue, Dec 09, 2014 at 11:32:45AM +0100, Igor Mammedov wrote:
> > On Mon, 8 Dec 2014 23:15:32 +0200
> > "Michael S. Tsirkin" <address@hidden> wrote:
> > 
> > > On Mon, Dec 08, 2014 at 04:08:03PM +0000, Igor Mammedov wrote:
> > > > According to ACPI spec NameSeg shorter than 4 characters
> > > > must be padded up to 4 characters with "_" symbol.
> > > > ACPI 5.0:  20.2.2 "Name Objects Encoding"
> > > > 
> > > > Do it in build_append_nameseg() so that caller shouldn't know
> > > > or care about it.
> > > > 
> > > > Signed-off-by: Igor Mammedov <address@hidden>
> > > 
> > > To me just doing it right in callers seems just as easy, but
> > > I guess you disagree :)
> > That means, author MUST know about padding, if he/she doesn't
> > or forget about it that would introduce error usually resulting
> > in BSOD.
> 
> Not really. assert will trigger on qemu start.
it sure will, but a bit confusing according to ASL spec name is up to
4 charactes and only AML spec specifies that there should be padding
if necessary.
I'm trying to get away from constructing AML by hands and the next step
for API would be ASL like one which is my eventual target in the end.

> 
> > This patch allows to avoid such mistakes.
> 
> Even the most basic testing (e.g. make check) will find these mistakes.
> It's more a question of which API we prefer.
> 
> Anyway, could you respond on g_assert vs assert please?
Ah, I'm sorry about that, haven't noticed comments below.

> 
> > > 
> > > > ---
> > > >  hw/i386/acpi-build.c | 18 +++++++++++++-----
> > > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > index f5ec66a..a8b7a2b 100644
> > > > --- a/hw/i386/acpi-build.c
> > > > +++ b/hw/i386/acpi-build.c
> > > > @@ -292,6 +292,8 @@ static inline void build_append_array(GArray 
> > > > *array, GArray *val)
> > > >      g_array_append_vals(array, val->data, val->len);
> > > >  }
> > > >  
> > > > +#define ACPI_NAMESEG_LEN 4
> > > > +
> > > >  static void GCC_FMT_ATTR(2, 3)
> > > >  build_append_nameseg(GArray *array, const char *format, ...)
> > > >  {
> > > > @@ -299,13 +301,19 @@ build_append_nameseg(GArray *array, const char 
> > > > *format, ...)
> > > >      char s[] = "XXXX";
> > > >      int len;
> > > >      va_list args;
> > > > +    const char padding = '_';
> > > >  
> > > >      va_start(args, format);
> > > >      len = vsnprintf(s, sizeof s, format, args);
> > > >      va_end(args);
> > > >  
> > > > -    assert(len == 4);
> > > > +    g_assert(len <= ACPI_NAMESEG_LEN);
> > > 
> > > I'm not sure when is g_assert preferable to assert.
> > > What's the motivation here?
None, I'm not sure what policy on it either
I can keep assert here if preferable.

> > > 
> > > 
> > > > +
> > > >      g_array_append_vals(array, s, len);
> > > > +    while (len != ACPI_NAMESEG_LEN) {
> > > > +        g_array_append_val(array, padding);
> > > > +        ++len;
> > > > +    }
> > > 
> > > Easier
> > > 
> > >   /* Pad up to 4 characters if necessary. */
> > >   g_array_append_vals(array, "____", 4 - len);
Thanks, I'll fix it up.

> > > 
> > > 
> > > 
> > > >  }
> > > >  
> > > >  /* 5.4 Definition Block Encoding */
> > > > @@ -846,7 +854,7 @@ static void build_pci_bus_end(PCIBus *bus, void 
> > > > *bus_state)
> > > >  
> > > >      if (bus->parent_dev) {
> > > >          op = 0x82; /* DeviceOp */
> > > > -        build_append_nameseg(bus_table, "S%.02X_",
> > > > +        build_append_nameseg(bus_table, "S%.02X",
> > > >                               bus->parent_dev->devfn);
> > > >          build_append_byte(bus_table, 0x08); /* NameOp */
> > > >          build_append_nameseg(bus_table, "_SUN");
> > > > @@ -966,7 +974,7 @@ static void build_pci_bus_end(PCIBus *bus, void 
> > > > *bus_state)
> > > >              build_append_int(notify, 0x1U << i);
> > > >              build_append_byte(notify, 0x00); /* NullName */
> > > >              build_append_byte(notify, 0x86); /* NotifyOp */
> > > > -            build_append_nameseg(notify, "S%.02X_", PCI_DEVFN(i, 0));
> > > > +            build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
> > > >              build_append_byte(notify, 0x69); /* Arg1Op */
> > > >  
> > > >              /* Pack it up */
> > > > @@ -1023,7 +1031,7 @@ static void build_pci_bus_end(PCIBus *bus, void 
> > > > *bus_state)
> > > >          if (bus->parent_dev) {
> > > >              build_append_byte(parent->notify_table, '^'); /* 
> > > > ParentPrefixChar */
> > > >              build_append_byte(parent->notify_table, 0x2E); /* 
> > > > DualNamePrefix */
> > > > -            build_append_nameseg(parent->notify_table, "S%.02X_",
> > > > +            build_append_nameseg(parent->notify_table, "S%.02X",
> > > >                                   bus->parent_dev->devfn);
> > > >              build_append_nameseg(parent->notify_table, "PCNT");
> > > >          }
> > > > @@ -1093,7 +1101,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> > > >          GArray *sb_scope = build_alloc_array();
> > > >          uint8_t op = 0x10; /* ScopeOp */
> > > >  
> > > > -        build_append_nameseg(sb_scope, "_SB_");
> > > > +        build_append_nameseg(sb_scope, "_SB");
> > > >  
> > > >          /* build Processor object for each processor */
> > > >          for (i = 0; i < acpi_cpus; i++) {
> > > > -- 
> > > > 1.8.3.1