Re: [Qemu-devel] [RFC PATCH 4/4] nvdimm acpi: build flush hint address structure if required

[Stefan Hajnoczi]

On Fri, Mar 31, 2017 at 04:41:47PM +0800, Haozhong Zhang wrote:
> 
> Add an boolean option 'flush-hint' to device 'nvdimm'. If it's on, a
> flush hint address structure will be constructed for each nvdimm
> device.

Users should not need to set the flush hint option.  NVDIMM
configurations that persist data properly without Flush Hint Addresses
shouldn't use them (for best performance).  Configurations that rely on
flush hints *must* use them to guarantee data integrity.

I don't remember if there's a way to detect the configuration from host
userspace, but we should focus on setting this correctly without
requiring users to know which setting is necessary.

> Signed-off-by: Haozhong Zhang <address@hidden>
> ---
>  hw/acpi/nvdimm.c        | 106 
> +++++++++++++++++++++++++++++++++++++++++++++---
>  hw/i386/pc.c            |   5 ++-
>  hw/mem/nvdimm.c         |  26 ++++++++++++
>  include/hw/mem/nvdimm.h |   2 +-
>  4 files changed, 132 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index ea2ac3e..a7ff0b2 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -32,6 +32,8 @@
>  #include "hw/acpi/bios-linker-loader.h"
>  #include "hw/nvram/fw_cfg.h"
>  #include "hw/mem/nvdimm.h"
> +#include "exec/address-spaces.h"
> +#include "qapi/error.h"
>  
>  static int nvdimm_device_list(Object *obj, void *opaque)
>  {
> @@ -168,6 +170,22 @@ struct NvdimmNfitControlRegion {
>  typedef struct NvdimmNfitControlRegion NvdimmNfitControlRegion;
>  
>  /*
> + * NVDIMM Flush Hint Address Structure
> + *
> + * It enables the data durability mechanism via ACPI.
> + */
> +struct NvdimmNfitFlushHintAddress {
> +    uint16_t type;
> +    uint16_t length;
> +    uint32_t nfit_handle;
> +    uint16_t nr_flush_hint_addr;
> +    uint8_t  reserved[6];
> +#define NR_FLUSH_HINT_ADDR 1
> +    uint64_t flush_hint_addr[NR_FLUSH_HINT_ADDR];

How should the number of flush hints be set?  This patch hardcodes it to
1 but the Linux nvdimm drivers tries to balance between flush hint
addresses to improve performance (to prevent cache line bouncing?).

> +} QEMU_PACKED;
> +typedef struct NvdimmNfitFlushHintAddress NvdimmNfitFlushHintAddress;
> +
> +/*
>   * Module serial number is a unique number for each device. We use the
>   * slot id of NVDIMM device to generate this number so that each device
>   * associates with a different number.
> @@ -343,10 +361,79 @@ static void nvdimm_build_structure_dcr(GArray 
> *structures, DeviceState *dev)
>                                           (DSM) in DSM Spec Rev1.*/);
>  }
>  
> -static GArray *nvdimm_build_device_structure(void)
> +static uint64_t nvdimm_flush_hint_read(void *opaque, hwaddr addr, unsigned 
> size)
> +{
> +    return 0;
> +}
> +
> +static void nvdimm_flush_hint_write(void *opaque, hwaddr addr,
> +                                    uint64_t data, unsigned size)
> +{
> +    nvdimm_debug("Write Flush Hint: offset 0x%"HWADDR_PRIx", data 
> 0x%"PRIx64"\n",
> +                 addr, data);
> +    nvdimm_flush((NVDIMMDevice *)opaque);

C automatically casts void * to any other pointer type.  The cast is
unnecessary.

signature.asc
Description: PGP signature