Re: [Qemu-devel] [PATCH V6 5/6] hw/arm/virt: add pvpanic device invirt acpi table

[Andrew Jones]

On Fri, Nov 16, 2018 at 09:45:42AM +0800, address@hidden wrote:
> >> add pvpanic device in virt acpi table, so when kenrel command line uses
> >> acpi=force, kernel can get info from acpi table in aarch64.
> 
> [...]
> 
> >>
> >> +static void acpi_dsdt_add_pvpanic(Aml *scope, const MemMapEntry 
> >> *pvpanic_memmap)
> >> +{
> >> +    Aml *dev = aml_device("PANC");
> >
> >Shouldn't this be "PEVT" ("panic event"), like it is for x86?
> >
> yeah, I will change it.
> >> +    aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0001")));
> >> +    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> >
> >Why add _UID? Also, I see x86 had some issues with not having _STA and
> >then having a _STA without the UI flag. It now has _STA=0xf
> >(PRESENT|ENABLED|UI|FUNCTIONING). I'm not saying we need to do that to,
> >but I'd like to know if it was considered and decided we don't need to.
> >
> The way the kernel code processes _STA is that if _STA is not found, the 
> default 
> setting is (PRESENT|ENABLED|UI|FUNCTIONING). So I think it is not necessary
>  to add it. It is only parsed in the pvpanic driver. 

Good enough for me.

> for _UID, I think it is used for device index. I just fill it with 0 because 
> there is 
> only one pvpanic device.

OK, but I'm still not sure it's necessary.

> 
> by the way, How to get the value of ACPI conveniently? how dou you get the 
> value of _STA?

Not sure what you mean here. From where do you want to check the value?
If you mean from guest userspace, then there's nothing convenient that I
know of. You'll have to disassemble the tables you extract from sysfs,
afaik.

Thanks,
drew

> 
> Thanks.
> >> +
> >> +    Aml *crs = aml_resource_template();
> >> +    aml_append(crs, aml_memory32_fixed(pvpanic_memmap->base,
> >> +                                       pvpanic_memmap->size, 
> >> AML_READ_WRITE));
> >> +
> >> +    aml_append(dev, aml_name_decl("_CRS", crs));
> >> +
> >> +    aml_append(scope, dev);
> >> +}
> >> +
> >>  static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
> >>                                             uint32_t uart_irq)
> >>  {
> >> @@ -770,6 +785,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, 
> >> VirtMachineState *vms)
> >>      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
> >>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
> >>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> >> +    acpi_dsdt_add_pvpanic(scope, &memmap[VIRT_PVPANIC]);
> >>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
> >>      acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
> >>      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
> >> --
> >> 1.8.3.1
> >>
> >>
> >
> >Thanks,
> >drew