On Tue, Mar 26, 2019 at 11:05:44AM +0100, Greg Kurz wrote: > On Tue, 26 Mar 2019 14:45:57 +0530 > Aravinda Prasad <aravi...@linux.vnet.ibm.com> wrote: > > > On Tuesday 26 March 2019 02:00 PM, Greg Kurz wrote: > > > On Tue, 26 Mar 2019 10:32:35 +1100 > > > David Gibson <da...@gibson.dropbear.id.au> wrote: > > > > > >> On Mon, Mar 25, 2019 at 01:56:50PM +0530, Aravinda Prasad wrote: > > >>> > > >>> > > >>> On Monday 25 March 2019 12:00 PM, David Gibson wrote: > > >>>> On Fri, Mar 22, 2019 at 12:04:07PM +0530, Aravinda Prasad wrote: > > >>>>> This patch builds the rtas error log, copies it to the > > >>>>> rtas_addr and then invokes the guest registered machine > > >>>>> check handler. > > >>>> > > >>>> This commit message needs more context. When is this occurring, why > > >>>> do we need this? > > >>>> > > >>>> [I can answer those questions now, but whether I - or anyone else - > > >>>> will be able to looking back at this commit from years in the future > > >>>> is a different question] > > >>> > > >>> will add more info. > > >> > > >> Thanks. > > >> > > >> [snip] > > >>>>> +static uint64_t spapr_get_rtas_addr(void) > > >>>>> +{ > > >>>>> + SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > > >>>>> + int rtas_node; > > >>>>> + const struct fdt_property *rtas_addr_prop; > > >>>>> + void *fdt = spapr->fdt_blob; > > >>>>> + uint32_t rtas_addr; > > >>>>> + > > >>>>> + /* fetch rtas addr from fdt */ > > >>>>> + rtas_node = fdt_path_offset(fdt, "/rtas"); > > >>>>> + g_assert(rtas_node >= 0); > > >>>>> + > > >>>>> + rtas_addr_prop = fdt_get_property(fdt, rtas_node, > > >>>>> "linux,rtas-base", NULL); > > >>>>> + g_assert(rtas_addr_prop); > > >>>>> + > > >>>>> + rtas_addr = fdt32_to_cpu(*(uint32_t *)rtas_addr_prop->data); > > >>>>> + return (uint64_t)rtas_addr; > > >>>> > > >>>> It seems a bit roundabout to pull the rtas address out of the device > > >>>> tree, since it was us that put it in there in the first place. > > >>> > > >>> Slof can change the rtas address. So we need to get the updated rtas > > >>> address. > > >> > > >> Ah, ok. > > >> > > > > > > Yeah, and knowing that the DT is guest originated makes me a bit > > > nervous when I see the g_assert()... a misbehaving guest could > > > possibly abort QEMU. Either there should be some sanity checks > > > performed earlier or an non-fatal error path should be added in > > > this function IMHO. > > > > Is it not the QEMU that builds the DT and provides it to the guest? > > > > Yes, but then the guest can push a new DT with the KVMPPC_H_UPDATE_DT hcall. > We only do some minimalist sanity checks in h_update_dt(). I don't think > we want to abort QEMU because the guest sent a DT where "linux,rtas-base" > is missing for example.
Yeah, that's a good point. > > Also, spapr_get_rtas_addr() is called during physical memory corruption > > which is a fatal error. > > Not that fatal since we care to report it to the guest. > > > So, if we cannot fetch rtas_addr (required to > > build and pass the error info to the guest) then I think we should abort. > > > > Maybe we cannot do anything better at this point, but then we should > do some earlier checks and switch to the old machine check behaviour > if what we need is missing from the updated DT for example. > > > > > > > -- 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