On Mon, Oct 24, 2016 at 11:47:29PM -0500, Michael Roth wrote: > In some cases, ibm,client-architecture-support calls can fail. This > could happen in the current code for situations where the modified > device tree segment exceeds the buffer size provided by the guest > via the call parameters. In these cases, QEMU will reset, allowing > an opportunity to regenerate the device tree from scratch via > boot-time handling. There are potentially other scenarios as well, > not currently reachable in the current code, but possible in theory, > such as cases where device-tree properties or nodes need to be removed. > > We currently don't handle either of these properly for option vector > capabilities however. Instead of carrying the negotiated capability > beyond the reset and creating the boot-time device tree accordingly, > we start from scratch, generating the same boot-time device tree as we > did prior to the CAS-generated and the same device tree updates as we > did before. This could (in theory) cause us to get stuck in a reset > loop. This hasn't been observed, but depending on the extensiveness > of CAS-induced device tree updates in the future, could eventually > become an issue. > > Address this by pulling capability-related device tree > updates resulting from CAS calls into a common routine, > spapr_dt_cas_updates(), and adding an sPAPROptionVector* > parameter that allows us to test for newly-negotiated capabilities. > We invoke it as follows: > > 1) When ibm,client-architecture-support gets called, we > call spapr_dt_cas_updates() with the set of capabilities > added since the previous call to ibm,client-architecture-support. > For the initial boot, or a system reset generated by something > other than the CAS call itself, this set will consist of *all* > options supported both the platform and the guest. For calls > to ibm,client-architecture-support immediately after a CAS-induced > reset, we call spapr_dt_cas_updates() with only the set > of capabilities added since the previous call, since the other > capabilities will have already been addressed by the boot-time > device-tree this time around. In the unlikely event that > capabilities are *removed* since the previous CAS, we will > generate a CAS-induced reset. In the unlikely event that we > cannot fit the device-tree updates into the buffer provided > by the guest, well generate a CAS-induced reset. > > 2) When a CAS update results in the need to reset the machine and > include the updates in the boot-time device tree, we call the > spapr_dt_cas_updates() using the full set of negotiated > capabilities as part of the reset path. At initial boot, or after > a reset generated by something other than the CAS call itself, > this set will be empty, resulting in what should be the same > boot-time device-tree as we generated prior to this patch. For > CAS-induced reset, this routine will be called with the full set of > capabilities negotiated by the platform/guest in the previous > CAS call, which should result in CAS updates from previous call > being accounted for in the initial boot-time device tree. > > Signed-off-by: Michael Roth <[email protected]> > Reviewed-by: David Gibson <[email protected]>
One little nit..
[snip]
> @@ -1013,13 +1013,27 @@ static target_ulong
> h_client_architecture_support(PowerPCCPU *cpu_,
> * of guest input. To model these properly we'd want some sort of mask,
> * but since they only currently apply to memory migration as defined
> * by LoPAPR 1.1, 14.5.4.8, which QEMU doesn't implement, we don't need
> - * to worry about this.
> + * to worry about this for now.
> */
> + ov5_cas_old = spapr_ovec_clone(spapr->ov5_cas);
> + /* full range of negotiated ov5 capabilities */
> spapr_ovec_intersect(spapr->ov5_cas, spapr->ov5, ov5_guest);
> spapr_ovec_cleanup(ov5_guest);
> + /* capabilities that have been added since CAS-generated guest reset.
> + * if capabilities have since been removed, generate another reset
> + */
> + ov5_updates = spapr_ovec_new();
> + spapr->cas_reboot = spapr_ovec_diff(ov5_updates,
> + ov5_cas_old, spapr->ov5_cas);
> +
> + if (!spapr->cas_reboot) {
> + spapr->cas_reboot =
> + spapr_h_cas_compose_response(spapr, args[1], args[2], cpu_update,
> + ov5_updates);
spapr->cas_reboot is a bool, whereas spapr_h_cas_compose_response()
returns an int error code. Now that C has real bools, I think the
compiler will do the right thing here, but I'd prefer to see an explicit
cas_reboot = (spapr_h_cas_compose_response() != 0)
for clarity.
> + }
> + spapr_ovec_cleanup(ov5_updates);
>
> - if (spapr_h_cas_compose_response(spapr, args[1], args[2],
> - cpu_update)) {
> + if (spapr->cas_reboot) {
> qemu_system_reset_request();
> }
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index a37eee8..b6f9f1b 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -75,6 +75,7 @@ struct sPAPRMachineState {
> bool has_graphics;
> sPAPROptionVector *ov5; /* QEMU-supported option vectors */
> sPAPROptionVector *ov5_cas; /* negotiated (via CAS) option vectors */
> + bool cas_reboot;
>
> uint32_t check_exception_irq;
> Notifier epow_notifier;
> @@ -586,7 +587,8 @@ void spapr_events_init(sPAPRMachineState *sm);
> void spapr_dt_events(void *fdt, uint32_t check_exception_irq);
> int spapr_h_cas_compose_response(sPAPRMachineState *sm,
> target_ulong addr, target_ulong size,
> - bool cpu_update);
> + bool cpu_update,
> + sPAPROptionVector *ov5_updates);
> sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn);
> void spapr_tce_table_enable(sPAPRTCETable *tcet,
> uint32_t page_shift, uint64_t bus_offset,
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
