Re: [Qemu-devel] [PATCH v4 07/10] s390x/sclp: properly guard pci-specific functions

[Cornelia Huck <cohuck@redhat.com>]

On Tue, 22 Aug 2017 14:58:37 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 08/22/2017 11:39 AM, Cornelia Huck wrote:
> > On Tue, 22 Aug 2017 11:20:51 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> On 08/22/2017 10:39 AM, Cornelia Huck wrote:  
> >>>> I'm fine either way. If I imagine having a lots of adapter types, then I
> >>>> would expect a switch or a jumptable on the type before handling control
> >>>> to the pci specific function. In this case statically not supported types
> >>>> would probably get caught by the default branch of the switch and for a
> >>>> jumptable it could even handle the dynamic case (based on the facilities)
> >>>> trivially. In short both approaches can make sense.    
> >>> I'm also wondering at the naming (the command sounds very
> >>> pci-specific). I'd just stick with this approach (modulo a possible
> >>> change of the response code, for which I need to rely on you guys).
> >>>     
> >>
> >>
> >> Well, the QEMU name of the command is misleading misleading. In the AR
> >> it's called 'Configure I/O Adapter'. The PCI comes into the picture via
> >> byte 8 of the SCCB, the so called adapter type. Valid values for the
> >> adapter type are: 00-01 reserved; 02 PCI function; 03-FF reserved. So
> >> at this point we only have PCI.  
> > 
> > OK, misleading naming combined with missing documentation leads to
> > confusion...
> > 
> > So:
> > 
> > - s/PCI/IOA/ for SCLP_CMDW_{CONFIGURE,DECONFIGURE}_PCI  
> nod
> 
> > - have a switch/case over byte 8 with only one case (pci)  
> 
> Let's say some kind of a check for bit 8 is a good idea.

Yes.

> 
> > - move the pci feature check into the pci code(? - not sure)  
> 
> Don't know. Architecturally I don't see any direct connection
> between the pci feature and this command.

I'd either have the check in the pci case for the adapter type, or in
the called function. It's probably cleaner to do the check before
calling the pci function.

> 
> The availability of SCLP_CMDW_{,DE}CONFIGURE_IOA is indicated
> by the result of the read scp info command read info in
> ReadInfo.facilities. I think we should assume that the read scp
> info command is always there.

Sure. But see the question in my other mail regarding the sclp
facilities bit (does it cover pci or adapter reconfiguration?)

> 
> I would appreciate someone with AR access double checking.

I'd have hoped you had AR access :p

> 
> > 
> > There's still the question of when this sclp command first became
> > available...
> >   
> 
> I would argue that it should not be important for AR
> compliance. Currently it operates only on PCI so I doubt it
> pre-dates PCI. But I don't think the current implementation
> is buggy because it offers the sclp command regardless
> of the zPCI facility.

I'm just wondering if there's another facility we're missing. The zpci
facility might imply presence of adapter reconfiguration, but are there
other facilities implying that as well, or even a dedicated facility?

> 
> I don't know where should I look for the historical details
> which go beyond the AR.

If there is no independent facility, it is probably best to just always
provide the command, but fail for pci adapter type if the zpci facility
is off.