Re: [Qemu-devel] [PATCH v5 19/20] hw/arm/virt-acpi-build: Add PCIe controller in ACPI DSDT table

[Igor Mammedov]

On Wed, 29 Apr 2015 21:37:11 +0800
Shannon Zhao <address@hidden> wrote:

> 
> 
> On 2015/4/29 16:47, Igor Mammedov wrote:
> > On Wed, 29 Apr 2015 11:12:04 +0800
> > Shannon Zhao <address@hidden> wrote:
> >
> >> On 2015/4/28 23:54, Michael S. Tsirkin wrote:
> >>> On Tue, Apr 28, 2015 at 05:13:10PM +0200, Igor Mammedov wrote:
> >>>>> Here I need to set the value of buffer to 1 or 0, we could
> >>>>> createbitfield, but if we want to set the value to non one or zero and
> >>>>> the BufferSize is large, how could we do? CreateByteField? It's a little
> >>>>> complex for user.
> >>>> that's what one would have to do writing it in ASL if bits
> >>>> are flipped on/off dynamically.
> >>>>
> >>>> In ASL you also can declare buffer with static initializer
> >>>>
> >>>>     Buffer (0x01) { 0x03 }
> >>>>
> >>>> and compiler is smart enough to set appropriate bits but it doesn't
> >>>> allow you to do so with large values. For example:
> >>>>
> >>>>     Buffer (0x01) { 0xAABBCCDD }
> >>>>
> >>>> gives error:
> >>>> Error 6139 - Constant out of range ^  (0xAABBCCDD, allowable: 0x0-0xFF)
> >>>>
> >>>> If one wants to manipulate specific fields in Buffer, ASL has
> >>>> a bunch of CreateFOOField operators, so lets follow spec and use
> >>>> API consistently to avoid confusion.
> >>>>
> >>>> BTW:
> >>>> packaging value as int (even without prefix) is wrong since
> >>>> its LE encoding will shuffle bytes and you won't get bits in
> >>>> positions that you expect if value is more than 1 byte.
> >>>
> >>> I don't care about ASL, we are writing in C
> >>> But AML is same:
> >>> DefBuffer := BufferOp PkgLength BufferSize ByteList
> >>> BufferOp := 0x11
> >>> BufferSize := TermArg => Integer
> >>>
> >>> So really just a bytelist.
> >>> We don't have any users for aml_buffer, maybe just add
> >>> const uint8_t *bytes, unsigned len as parameters.
> >>>
> >>
> >> Agree. It's consistent with the spec. If want to modify the value, could
> >> use CreateFOOField.
> >>
> >> So use following fuction to initialize Buffer?
> >>
> >> /* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefBuffer */
> >> Aml *aml_buffer(int buffer_size, uint8_t *byte_list)
> >> {
> >>      int i;
> >>      Aml *var = aml_bundle(0x11 /* BufferOp */, AML_BUFFER);
> >>      for (i = 0; i < buffer_size; i++) {
> >>          build_append_byte(var->buf, *(byte_list + i));
> >>      }
> >>      return var;
> >> }
> > maybe
> >
> > Aml *aml_buffer_initialized(int buffer_size, uint8_t *byte_list);
> > Aml *aml_buffer(int buffer_size);
> >
> > the second one is needed for implementing code like:
> > Name(BUFF, Buffer(4){}) // Create SerialBus data buffer as BUFF
> > CreateByteField(BUFF, 0x00, STAT) // STAT = Status (Byte)
> > CreateWordField(BUFF, 0x02, DATA) // DATA = Data (Byte)
> >
> > and could reuse aml_buffer_initialized() to reserve space.
> >
> 
> maybe we could use only one. For the uninitialized buffer we can pass 
> byte_list as NULL and within aml_buffer check byte_list, if byte_list is 
> NULL, just reserve space.
works for me with doc comment

> 
> /* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefBuffer */
> Aml *aml_buffer(int buffer_size, uint8_t *byte_list)
> {
>      int i;
>      Aml *var = aml_bundle(0x11 /* BufferOp */, AML_BUFFER);
> 
>      for (i = 0; i < buffer_size; i++) {
>          if (byte_list == NULL)
>           build_append_byte(var->buf, 0);
>          else
>              build_append_byte(var->buf, *(byte_list + i));
>     }
> 
>     return var;
> }
> 
> >>
> >>> Would that be enough?
> >>>
> >>>
> >>>>>
> >>>>>> ifctx1 = aml_if(aml_equal(aml_arg(2), aml_int(0)));
> >>>>>> aml_append(ifctx1, aml_store(aml_name("FNEN", aml_int(1)));
> >>>>>> ...
> >>>>>> /* create bit field for every supported function if supported */
> >>>>>> ...
> >>>>>> aml_append(method, aml_return(aml_name("RET")));
> >>>>>>
> >>>>>>
> >>>>>>>
> >>>>>>>>>
> >>>>>>>>>> +    aml_append(ifctx1, aml_return(buf));
> >>>>>>>>>> +    aml_append(ifctx, ifctx1);
> >>>>>>>>>> +    aml_append(method, ifctx);
> >>>>>>>>>> +
> >>>>>>>>>> +    buf = aml_buffer();
> >>>>>>>>>> +    build_append_int_noprefix(buf->buf, 0x00, 1);
> >>>>>>>>>> +    aml_append(method, aml_return(buf));
> >>>>>>>>>> +    aml_append(dev, method);
> >>>>>>>>>> +
> >>>>>>>>>> +    Aml *dev_rp0 = aml_device("%s", "RP0");
> >>>>>>>>>> +    aml_append(dev_rp0, aml_name_decl("_ADR", aml_int(0)));
> >>>>>>>>>> +    aml_append(dev, dev_rp0);
> >>>>>>>>>> +    aml_append(scope, dev);
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>>   /* RSDP */
> >>>>>>>>>>   static GArray *
> >>>>>>>>>>   build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
> >>>>>>>>>> @@ -318,6 +468,8 @@ build_dsdt(GArray *table_data, GArray *linker, 
> >>>>>>>>>> VirtGuestInfo *guest_info)
> >>>>>>>>>>       acpi_dsdt_add_flash(scope, info->flash_memmap);
> >>>>>>>>>>       acpi_dsdt_add_virtio(scope, info->virtio_mmio_memmap,
> >>>>>>>>>>                info->virtio_mmio_irq, info->virtio_mmio_num);
> >>>>>>>>>> +    acpi_dsdt_add_pci(scope, guest_info->pcie_info);
> >>>>>>>>>> +
> >>>>>>>>>>       aml_append(dsdt, scope);
> >>>>>>>>>>
> >>>>>>>>>>       /* copy AML table into ACPI tables blob and patch header 
> >>>>>>>>>> there */
> >>>>>>>>
> >>>>>>>> .
> >>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>> .
> >>>>>>
> >>>>>
> >>>>>
> >>>
> >>> .
> >>>
> >>
> >>
> >