Re: [PATCH v4 1/2] x86: return modified setup_data only if read as memory, not as file

[Paolo Bonzini]

> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 050eedc0c8..933bbdd836 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -764,6 +764,18 @@ static bool load_elfboot(const char *kernel_filename,
>      return true;
>  }
>
> +struct setup_data_fixup {
> +    void *pos;
> +    hwaddr val;
> +    uint32_t addr;
> +};

Just a small comment, addr should be little-endian (see
fw_cfg_add_i32).  It's not used outside x86_load_linux, so it is
possible to just use cpu_to_le32 there.

Also I think it's cleaner if a reset callback puts the value back to
zero. fw_cfg already has fw_cfg_machine_reset, so perhaps the easiest
way is to add a FWCfgCallback reset_cb argument to just
fw_cfg_add_bytes_callback. If I am missing something and it's not
necessary I can do the cpu_to_le32 change myself or wait for you; in
any case I'll wait for either your ack or a v5.

By the way, does this supersede v1..v3 that use the new protocol (I'd
guess so from the presence of the same 2/2 patch), or are the two
patches doing belts-and-suspenders?

Thanks,

Paolo

> +static void fixup_setup_data(void *opaque)
> +{
> +    struct setup_data_fixup *fixup = opaque;
> +    stq_p(fixup->pos, fixup->val);
> +}
> +
>  void x86_load_linux(X86MachineState *x86ms,
>                      FWCfgState *fw_cfg,
>                      int acpi_data_size,
> @@ -1088,8 +1100,11 @@ void x86_load_linux(X86MachineState *x86ms,
>          qemu_guest_getrandom_nofail(setup_data->data, RNG_SEED_LENGTH);
>      }
>
> -    /* Offset 0x250 is a pointer to the first setup_data link. */
> -    stq_p(header + 0x250, first_setup_data);
> +    fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, prot_addr);
> +    fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_SIZE, kernel_size);
> +    fw_cfg_add_bytes(fw_cfg, FW_CFG_KERNEL_DATA, kernel, kernel_size);
> +    sev_load_ctx.kernel_data = (char *)kernel;
> +    sev_load_ctx.kernel_size = kernel_size;
>
>      /*
>       * If we're starting an encrypted VM, it will be OVMF based, which uses 
> the
> @@ -1099,16 +1114,18 @@ void x86_load_linux(X86MachineState *x86ms,
>       * file the user passed in.
>       */
>      if (!sev_enabled()) {
> +        struct setup_data_fixup *fixup = g_malloc(sizeof(*fixup));
> +
>          memcpy(setup, header, MIN(sizeof(header), setup_size));
> +        /* Offset 0x250 is a pointer to the first setup_data link. */
> +        fixup->pos = setup + 0x250;
> +        fixup->val = first_setup_data;
> +        fixup->addr = real_addr;
> +        fw_cfg_add_bytes_callback(fw_cfg, FW_CFG_SETUP_ADDR, 
> fixup_setup_data, NULL,
> +                                  fixup, &fixup->addr, sizeof(fixup->addr), 
> true);
> +    } else {
> +        fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_ADDR, real_addr);
>      }
> -
> -    fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, prot_addr);
> -    fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_SIZE, kernel_size);
> -    fw_cfg_add_bytes(fw_cfg, FW_CFG_KERNEL_DATA, kernel, kernel_size);
> -    sev_load_ctx.kernel_data = (char *)kernel;
> -    sev_load_ctx.kernel_size = kernel_size;
> -
> -    fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_ADDR, real_addr);
>      fw_cfg_add_i32(fw_cfg, FW_CFG_SETUP_SIZE, setup_size);
>      fw_cfg_add_bytes(fw_cfg, FW_CFG_SETUP_DATA, setup, setup_size);
>      sev_load_ctx.setup_data = (char *)setup;
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index d605f3f45a..564bda3395 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -692,12 +692,12 @@ static const VMStateDescription vmstate_fw_cfg = {
>      }
>  };
>
> -static void fw_cfg_add_bytes_callback(FWCfgState *s, uint16_t key,
> -                                      FWCfgCallback select_cb,
> -                                      FWCfgWriteCallback write_cb,
> -                                      void *callback_opaque,
> -                                      void *data, size_t len,
> -                                      bool read_only)
> +void fw_cfg_add_bytes_callback(FWCfgState *s, uint16_t key,
> +                               FWCfgCallback select_cb,
> +                               FWCfgWriteCallback write_cb,
> +                               void *callback_opaque,
> +                               void *data, size_t len,
> +                               bool read_only)
>  {
>      int arch = !!(key & FW_CFG_ARCH_LOCAL);
>
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 0e7a8bc7af..e4fef393be 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -117,6 +117,28 @@ struct FWCfgMemState {
>   */
>  void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len);
>
> +/**
> + * fw_cfg_add_bytes_callback:
> + * @s: fw_cfg device being modified
> + * @key: selector key value for new fw_cfg item
> + * @select_cb: callback function when selecting
> + * @write_cb: callback function after a write
> + * @callback_opaque: argument to be passed into callback function
> + * @data: pointer to start of item data
> + * @len: size of item data
> + * @read_only: is file read only
> + *
> + * Add a new fw_cfg item, available by selecting the given key, as a raw
> + * "blob" of the given size. The data referenced by the starting pointer
> + * is only linked, NOT copied, into the data structure of the fw_cfg device.
> + */
> +void fw_cfg_add_bytes_callback(FWCfgState *s, uint16_t key,
> +                               FWCfgCallback select_cb,
> +                               FWCfgWriteCallback write_cb,
> +                               void *callback_opaque,
> +                               void *data, size_t len,
> +                               bool read_only);
> +
>  /**
>   * fw_cfg_add_string:
>   * @s: fw_cfg device being modified
> --
> 2.37.3
>