Re: [Qemu-devel] [PATCH v5 04/36] ppc/xive: introduce the XiveRouter model

[David Gibson]

On Tue, Nov 27, 2018 at 08:30:15AM +0100, Cédric Le Goater wrote:
> On 11/27/18 1:11 AM, David Gibson wrote:
> > On Mon, Nov 26, 2018 at 10:39:44AM +0100, Cédric Le Goater wrote:
> >> On 11/26/18 6:44 AM, David Gibson wrote:
> >>> On Fri, Nov 23, 2018 at 11:28:24AM +0100, Cédric Le Goater wrote:
> >>>> On 11/23/18 2:10 AM, David Gibson wrote:
> >>>>> On Thu, Nov 22, 2018 at 05:50:07PM +1100, Benjamin Herrenschmidt wrote:
> >>>>>> On Thu, 2018-11-22 at 15:44 +1100, David Gibson wrote:
> >>>>>>>
> >>>>>>> Sorry, didn't think of this in my first reply.
> >>>>>>>
> >>>>>>> 1) Does the hardware ever actually write back to the EAS?  I know it
> >>>>>>> does for the END, but it's not clear why it would need to for the
> >>>>>>> EAS.  If not, we don't need the setter.
> >>>>>>
> >>>>>> Nope, though the PAPR model will via hcalls
> >>>>>
> >>>>> Right, bit AIUI the set_eas hook is about abstracting PAPR vs bare
> >>>>> metal details.  Since the hcall knows it's PAPR it can just update the
> >>>>> backing information for the EAS directly, and no need for an
> >>>>> abstracted hook.
> >>>>
> >>>> Indeed, the first versions of the XIVE patchset did not use such hooks, 
> >>>> but when discussed we said we wanted abstract methods for the router 
> >>>> to validate the overall XIVE model, which is useful for PowerNV. 
> >>>>
> >>>> We can change again and have the hcalls get/set directly in the EAT
> >>>> and ENDT. It would certainly simplify the sPAPR model.
> >>>
> >>> I think that's the better approach.
> >>
> >> ok. let's keep that in mind for  : 
> >>
> >>  [PATCH v5 11/36] spapr/xive: use the VCPU id as a NVT identifier
> >>  [PATCH v5 16/36] spapr: add hcalls support for the XIVE exploitation
> >>
> >> which are using the XiveRouter methods to access the controller EAT 
> >> and ENDT. I thought that was good practice to validate the model but 
> >> we can use direct sPAPR table accessors or none at all.
> > 
> > Ok.  Consistency is good as a general rule, but I don't think it makes
> > sense to force the EAT and the ENDT into the same model.  
> 
> What do you mean by model ? the QEMU machine IC model ?

Oh, sorry, nothing that formal.  By "model" I just meant the same
pattern of accessor hooks.  They certainly do belong in the same qemu
object.

> > The EAT is
> > pure configuration, whereas the the ENDT has both configuration and
> > status.  Or to look at it another way, the EAT is purely software
> > controlled, whereas the ENDT is at least partially hardware
> > controlled.
> 
> yes but the EAT and the ENDT are XIVE internal tables of the same XIVE 
> sub-engine, the IVRE, Interrupt Virtualization Routing Engine, formely 
> known as VC, for Virtualization Controller. the EAS is just an entry 
> point to the ENDT. I don't see why we would use different models for 
> them.
> 
> > (I realize that gets a bit fuzzy when considering PAPR, but I think
> > from the point of view of the XIVE model it makes sense to treat the
> > PAPR hypervisor logic as "software", even though it's "hardware" from the
> > guest point of view).
> 
> That I agree but the resulting code is too ugly in the hcalls. Tell me 
> when you reach patch 11, which links the machine IC model sPAPRXive to 
> the generic XiveRouter and also check patch 16 introducing the hcalls, 
> which update the XIVE internal tables.
> 
> Thanks,
> 
> C. 
> 
>  
> >>
> >>
> >> I think these prereq patches could be merged now :
> >>
> >>  [PATCH v5 12/36] spapr: initialize VSMT before initializing the IRQ
> >>  [PATCH v5 13/36] spapr: introduce a spapr_irq_init() routine
> >>  [PATCH v5 14/36] spapr: modify the irq backend 'init' method
> >>
> >> This one also :
> >>
> >>  [PATCH v5 21/36] spapr: extend the sPAPR IRQ backend for XICS
> >>
> >> Thanks,
> >>
> >> C. 
> >>
> > 
> 

-- 
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