Re: [Qemu-devel] [PATCH 13/27] acpi: memory hotplug ACPI hardware implementation

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3132 bytes --]

On 11/20/2013 07:38 PM, Igor Mammedov wrote:
> - implements QEMU hardware part of memory hotplug protocol
>   described at "docs/specs/acpi_mem_hotplug.txt"
> - handles only memory add notification event for now
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---

> +0xa00:
> +  read access:
> +      [0x0-0x3] Lo part of memory device phys address
> +      [0x4-0x7] Hi part of memory device phys address
> +      [0x8-0xb] Lo part of memory device size in bytes
> +      [0xc-0xf] Hi part of memory device size in bytes
> +      [0x14] highest memory hot-plug interface version supported by QEMU

Nothing about [0x10-0x13]? [Ah, I see you mention it later]

> +      [0x15] Memory device status fields
> +          bits:
> +              1: device is enabled and may be used by guest
> +              2: device insert event, used by ACPI BIOS to distinguish
> +                 device for which no device check event to OSPM was issued

Why start at bit 1 instead of bit 0?  Also, should you document that
remaining bits (2-7) should be ignored on read?

> +      [0x16-0x17] reserved
> +
> +  write access:
> +      [0x0-0x3] Memory device slot selector, selects active memory device.
> +                All following accesses to other registers in 0xaf80-0xaf97
> +                region will read/store data from/to selected memory device.
> +      [0x4-0x7] OST event code reported by OSPM
> +      [0x8-0xb] OST status code reported by OSPM
> +      [0x15] Memory device status fields

Nothing about behavior of [0xc-0x14]?  Is it an error to write there, or
are the writes just ignored, or...?

> +          bits:
> +              2: if set to 1 clears device insert event, set by ACPI BIOS
> +                 after it has sent device check event to OSPM for
> +                 seleted memory device

s/seleted/selected/

What must be written into the other bits?  Must reserved bits be written
as 0, or the user do a read-modify-write, or...?

> +
> +Selecting memory device slot beyond present range has no effect on platform:
> +   - not documented above write accesses to memory hot-plug registers
> +     are ignored;

Reads awkwardly.  Better is:

write accesses to memory hot-plug registers not documented above are ignored

> +   - not documented above read accesses to memory hot-plug registers return 0xFF

Similar awkward wording.

> +    case 0xc: /* Hi part of DIMM size */
> +        val = object_property_get_int(OBJECT(mdev->dimm), "size", NULL) >> 32;
> +        break;
> +    case 0x10: /* node proximity for _PXM method */
> +        val = object_property_get_int(OBJECT(mdev->dimm), "node", NULL);
> +        break;

This wasn't documented.

> +    case 0x14: /* intf version */
> +        val = 1;
> +        break;
> +    case 0x15: /* pack and return is_* fields */
> +        val |= mdev->is_enabled   ? 1 : 0;
> +        val |= mdev->is_inserting ? 2 : 0;

This is bit 0 and 1, not bit 1 and 2.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]