Re: [Qemu-devel] [PATCH v6 wave 2 1/3] hw/isa/lpc_ich9: add SMI feature negotiation via fw_cfg

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 16 Jan 2017 21:51:34 +0100
Laszlo Ersek <lersek@redhat.com> wrote:

> On 01/13/17 14:34, Igor Mammedov wrote:
> > On Thu, 12 Jan 2017 19:24:44 +0100
> > Laszlo Ersek <lersek@redhat.com> wrote:
> > 
> > [...]  
> >> +static void smi_features_ok_callback(void *opaque)
> >> +{
> >> +    ICH9LPCState *lpc = opaque;
> >> +    uint64_t guest_features;
> >> +
> >> +    if (lpc->smi_features_ok) {
> >> +        /* negotiation already complete, features locked */
> >> +        return;
> >> +    }
> >> +
> >> +    memcpy(&guest_features, lpc->smi_guest_features_le, sizeof guest_features);
> >> +    le64_to_cpus(&guest_features);
> >> +    if (guest_features & ~lpc->smi_host_features) {
> >> +        /* guest requests invalid features, leave @features_ok at zero */
> >> +        return;
> >> +    }
> >> +
> >> +    /* valid feature subset requested, lock it down, report success */
> >> +    lpc->smi_negotiated_features = guest_features;
> >> +    lpc->smi_features_ok = 1;
> >> +}  
> > [...]
> >   
> >> +    .fields = (VMStateField[]) {
> >> +        VMSTATE_UINT8_ARRAY(smi_guest_features_le, ICH9LPCState,
> >> +                            sizeof(uint64_t)),
> >> +        VMSTATE_UINT8(smi_features_ok, ICH9LPCState),
> >> +        VMSTATE_UINT64(smi_negotiated_features, ICH9LPCState),  
> > 
> > Couldn't smi_negotiated_features be derived from
> > smi_guest_features_le on target side?  
> 
> No, that's something we must expressly not do.
> 
> The "guest-features" file remains writeable (and back-readable) to the
> guest after feature validation/lockdown (i.e., after selecting
> "features-ok"), it just has no effect on QEMU.
> 
> And, if the guest rewrites "guest-features" between feature lockdown and
> migration, that must have no effect on the target host either. Hence we
> must carry forward the negotiated features originally computed on the
> source host.
ok,
patch looks good to me, so

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> Thanks
> Laszlo
> 
> > 
> >   
> >> +        VMSTATE_END_OF_LIST()
> >> +    }
> >> +};
> >> +
> >>  static const VMStateDescription vmstate_ich9_lpc = {
> >>      .name = "ICH9LPC",
> >>      .version_id = 1,
> >> @@ -683,6 +761,7 @@ static const VMStateDescription vmstate_ich9_lpc = {
> >>      },
> >>      .subsections = (const VMStateDescription*[]) {
> >>          &vmstate_ich9_rst_cnt,
> >> +        &vmstate_ich9_smi_feat,
> >>          NULL
> >>      }
> >>  };  
> >   
> 
>