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

[Cornelia Huck]

On Mon, 21 Aug 2017 18:24:15 +0200
Halil Pasic <address@hidden> wrote:

> On 08/21/2017 04:58 PM, Pierre Morel wrote:
> > On 21/08/2017 11:16, Cornelia Huck wrote:  
> >> If we do not provide zpci, pci reconfiguration via sclp is not available
> >> either. Don't indicate it in the sclp facilities and return an invalid
> >> command if the guest tries to issue pci configure/deconfigure.
> >>
> >> Reviewed-by: Thomas Huth <address@hidden>
> >> Signed-off-by: Cornelia Huck <address@hidden>
> >> ---
> >>   hw/s390x/sclp.c | 19 +++++++++++++++----
> >>   1 file changed, 15 insertions(+), 4 deletions(-)
> >>

> >> @@ -385,10 +388,18 @@ static void sclp_execute(SCLPDevice *sclp, SCCB 
> >> *sccb, uint32_t code)
> >>           sclp_c->unassign_storage(sclp, sccb);
> >>           break;
> >>       case SCLP_CMDW_CONFIGURE_PCI:
> >> -        s390_pci_sclp_configure(sccb);
> >> +        if (s390_has_feat(S390_FEAT_ZPCI)) {
> >> +            s390_pci_sclp_configure(sccb);
> >> +        } else {
> >> +            sccb->h.response_code = 
> >> cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);  
> > 
> > Hello Conny,
> > 
> > I find more logical if the response would be 0x06F0 : "Adapter type in SCCB 
> > not recognized"
> > Since we could have more than one adapter type... someday.
> > then the SCLP command would be valid but not the adapter type.  
> 
> I agree, 01F0 does not seem to be the correct answer, based on
> the AR as it seems to be tied to the command code.

What I'm wondering here: SCLP_CMDW_CONFIGURE_PCI was introduced at some
point in time (probably when pci was first introduced). On older
machines, invalid command sounds like the most logical response. Do you
know whether there's some tie-in to the machine version?

> Verifying what the machine does would be great though -- frankly
> I cant tell with absolute confidence what's the right thing to
> do based on my understanding of the AR. 

Multiply this by not having access to the AR :)

> > However, I will try to find a real hardware to test this since I noticed 
> > that my logic sometime... :) .
> > 
> > Another point is that don't you think that this test on S390_FEAT_ZPCI 
> > better belong to the dedicated PCI code inside of the 
> > s390_pci_sclp_configure().
> >  
> 
> 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).