Re: [PATCH v3 2/2] hw/arm/virt: Support for virtio-mem-pci

[Peter Maydell]

On Fri, 3 Dec 2021 at 23:34, Gavin Shan <gshan@redhat.com> wrote:
>
> This supports virtio-mem-pci device on "virt" platform, by simply
> following the implementation on x86.
>
>    * This implements the hotplug handlers to support virtio-mem-pci
>      device hot-add, while the hot-remove isn't supported as we have
>      on x86.
>
>    * The block size is 512MB on ARM64 instead of 128MB on x86.
>
>    * It has been passing the tests with various combinations like 64KB
>      and 4KB page sizes on host and guest, different memory device
>      backends like normal, transparent huge page and HugeTLB, plus
>      migration.
>
> Co-developed-by: David Hildenbrand <david@redhat.com>
> Co-developed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>


> +static void virt_virtio_md_pci_pre_plug(HotplugHandler *hotplug_dev,
> +                                        DeviceState *dev, Error **errp)
> +{
> +    HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
> +    Error *local_err = NULL;
> +
> +    if (!hotplug_dev2 && dev->hotplugged) {
> +        /*
> +         * Without a bus hotplug handler, we cannot control the plug/unplug
> +         * order. We should never reach this point when hotplugging on x86,
> +         * however, better add a safety net.
> +         */

This comment looks like it was cut-n-pasted from x86 -- is whatever
it is that prevents us from reaching this point also true for arm ?
(What is the thing that prevents us reaching this point?)

> +        error_setg(errp, "hotplug of virtio based memory devices not 
> supported"
> +                   " on this bus.");
> +        return;
> +    }
> +    /*
> +     * First, see if we can plug this memory device at all. If that
> +     * succeeds, branch of to the actual hotplug handler.
> +     */
> +    memory_device_pre_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev), NULL,
> +                           &local_err);
> +    if (!local_err && hotplug_dev2) {
> +        hotplug_handler_pre_plug(hotplug_dev2, dev, &local_err);
> +    }
> +    error_propagate(errp, local_err);
> +}



> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> index b20595a496..21e4d572ab 100644
> --- a/hw/virtio/virtio-mem.c
> +++ b/hw/virtio/virtio-mem.c
> @@ -125,7 +125,7 @@ static uint64_t virtio_mem_default_block_size(RAMBlock 
> *rb)
>   * The memory block size corresponds mostly to the section size.
>   *
>   * This allows e.g., to add 20MB with a section size of 128MB on x86_64, and
> - * a section size of 1GB on arm64 (as long as the start address is properly
> + * a section size of 512MB on arm64 (as long as the start address is properly
>   * aligned, similar to ordinary DIMMs).
>   *
>   * We can change this at any time and maybe even make it configurable if
> @@ -134,6 +134,8 @@ static uint64_t virtio_mem_default_block_size(RAMBlock 
> *rb)
>   */
>  #if defined(TARGET_X86_64) || defined(TARGET_I386)
>  #define VIRTIO_MEM_USABLE_EXTENT (2 * (128 * MiB))
> +#elif defined(TARGET_ARM)
> +#define VIRTIO_MEM_USABLE_EXTENT (2 * (512 * MiB))
>  #else
>  #error VIRTIO_MEM_USABLE_EXTENT not defined
>  #endif

Could this comment explain where the 128MB and 512MB come from
and why the value is different for different architectures ?

thanks
-- PMM