[v4,07/10] s390x/sclp: properly guard pci-specific functions

Commit Message

Cornelia Huck Aug. 21, 2017, 9:16 a.m. UTC

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 <[e-mail]>
Signed-off-by: Cornelia Huck <[e-mail]>
---
 hw/s390x/sclp.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

Comments

Halil Pasic Aug. 21, 2017, 11:41 a.m. UTC | #1

On 08/21/2017 11:16 AM, 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 <[e-mail]>
> Signed-off-by: Cornelia Huck <[e-mail]>
> ---
>  hw/s390x/sclp.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 9253dbbc64..d0104cd784 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -59,6 +59,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>      int rnsize, rnmax;
>      int slots = MIN(machine->ram_slots, s390_get_memslot_count(kvm_state));
>      IplParameterBlock *ipib = s390_ipl_get_iplb();
> +    uint64_t sclp_facilities = SCLP_HAS_CPU_INFO;
> 
>      CPU_FOREACH(cpu) {
>          cpu_count++;
> @@ -79,8 +80,10 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
> 
>      prepare_cpu_entries(sclp, read_info->entries, cpu_count);
> 
> -    read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
> -                                        SCLP_HAS_PCI_RECONFIG);
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        sclp_facilities |= SCLP_HAS_PCI_RECONFIG;
> +    }
> +    read_info->facilities = cpu_to_be64(sclp_facilities);
> 
>      /* Memory Hotplug is only supported for the ccw machine type */
>      if (mhd) {
> @@ -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);
> +        }
>          break;
>      case SCLP_CMDW_DECONFIGURE_PCI:
> -        s390_pci_sclp_deconfigure(sccb);
> +        if (s390_has_feat(S390_FEAT_ZPCI)) {
> +            s390_pci_sclp_deconfigure(sccb);
> +        } else {
> +            sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> +        }
>          break;
>      default:
>          efc->command_handler(ef, sccb, code);
> 

LGTM

Cornelia Huck Aug. 21, 2017, 1:16 p.m. UTC | #2

On Mon, 21 Aug 2017 13:41:53 +0200
Halil Pasic <[e-mail]> wrote:

> On 08/21/2017 11:16 AM, 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 <[e-mail]>
> > Signed-off-by: Cornelia Huck <[e-mail]>
> > ---
> >  hw/s390x/sclp.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)

(...)

> LGTM

Is that an Acked-by:? Or a Reviewed-by:? :)

Halil Pasic Aug. 21, 2017, 1:32 p.m. UTC | #3

On 08/21/2017 03:16 PM, Cornelia Huck wrote:
> On Mon, 21 Aug 2017 13:41:53 +0200
> Halil Pasic <[e-mail]> wrote:
> 
>> On 08/21/2017 11:16 AM, 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 <[e-mail]>
>>> Signed-off-by: Cornelia Huck <[e-mail]>
>>> ---
>>>  hw/s390x/sclp.c | 19 +++++++++++++++----
>>>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> (...)
> 
>> LGTM
> 
> Is that an Acked-by:? Or a Reviewed-by:? :)
> 

r-b

(Not sure what Acked-by from a role other that affected
maintainer means.)

Cornelia Huck Aug. 21, 2017, 1:36 p.m. UTC | #4

On Mon, 21 Aug 2017 15:32:23 +0200
Halil Pasic <[e-mail]> wrote:

> On 08/21/2017 03:16 PM, Cornelia Huck wrote:
> > On Mon, 21 Aug 2017 13:41:53 +0200
> > Halil Pasic <[e-mail]> wrote:
> >   
> >> On 08/21/2017 11:16 AM, 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 <[e-mail]>
> >>> Signed-off-by: Cornelia Huck <[e-mail]>
> >>> ---
> >>>  hw/s390x/sclp.c | 19 +++++++++++++++----
> >>>  1 file changed, 15 insertions(+), 4 deletions(-)  
> > 
> > (...)
> >   
> >> LGTM  
> > 
> > Is that an Acked-by:? Or a Reviewed-by:? :)
> >   
> 
> r-b

If you write that out in future, I can simply copy'n'paste it :)

> 
> (Not sure what Acked-by from a role other that affected
> maintainer means.)

I've always used it in the sense of "I don't know the gory details, and
I haven't tried to find them out, but it generally looks sane to me".

Pierre Morel Aug. 21, 2017, 2:58 p.m. UTC | #5

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 <[e-mail]>
> Signed-off-by: Cornelia Huck <[e-mail]>
> ---
>   hw/s390x/sclp.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 9253dbbc64..d0104cd784 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -59,6 +59,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>       int rnsize, rnmax;
>       int slots = MIN(machine->ram_slots, s390_get_memslot_count(kvm_state));
>       IplParameterBlock *ipib = s390_ipl_get_iplb();
> +    uint64_t sclp_facilities = SCLP_HAS_CPU_INFO;
> 
>       CPU_FOREACH(cpu) {
>           cpu_count++;
> @@ -79,8 +80,10 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
> 
>       prepare_cpu_entries(sclp, read_info->entries, cpu_count);
> 
> -    read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
> -                                        SCLP_HAS_PCI_RECONFIG);
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        sclp_facilities |= SCLP_HAS_PCI_RECONFIG;
> +    }
> +    read_info->facilities = cpu_to_be64(sclp_facilities);
> 
>       /* Memory Hotplug is only supported for the ccw machine type */
>       if (mhd) {
> @@ -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.

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().

best regards,

Pierre


> +        }
>           break;
>       case SCLP_CMDW_DECONFIGURE_PCI:
> -        s390_pci_sclp_deconfigure(sccb);
> +        if (s390_has_feat(S390_FEAT_ZPCI)) {
> +            s390_pci_sclp_deconfigure(sccb);
> +        } else {
> +            sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
> +        }
>           break;
>       default:
>           efc->command_handler(ef, sccb, code);
>

Halil Pasic Aug. 21, 2017, 4:24 p.m. UTC | #6

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 <[e-mail]>
>> Signed-off-by: Cornelia Huck <[e-mail]>
>> ---
>>   hw/s390x/sclp.c | 19 +++++++++++++++----
>>   1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index 9253dbbc64..d0104cd784 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -59,6 +59,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>       int rnsize, rnmax;
>>       int slots = MIN(machine->ram_slots, s390_get_memslot_count(kvm_state));
>>       IplParameterBlock *ipib = s390_ipl_get_iplb();
>> +    uint64_t sclp_facilities = SCLP_HAS_CPU_INFO;
>>
>>       CPU_FOREACH(cpu) {
>>           cpu_count++;
>> @@ -79,8 +80,10 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>>
>>       prepare_cpu_entries(sclp, read_info->entries, cpu_count);
>>
>> -    read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
>> -                                        SCLP_HAS_PCI_RECONFIG);
>> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
>> +        sclp_facilities |= SCLP_HAS_PCI_RECONFIG;
>> +    }
>> +    read_info->facilities = cpu_to_be64(sclp_facilities);
>>
>>       /* Memory Hotplug is only supported for the ccw machine type */
>>       if (mhd) {
>> @@ -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.

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. 


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

Regards,
Halil

> best regards,
> 
> Pierre
> 
> 
>> +        }
>>           break;
>>       case SCLP_CMDW_DECONFIGURE_PCI:
>> -        s390_pci_sclp_deconfigure(sccb);
>> +        if (s390_has_feat(S390_FEAT_ZPCI)) {
>> +            s390_pci_sclp_deconfigure(sccb);
>> +        } else {
>> +            sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
>> +        }
>>           break;
>>       default:
>>           efc->command_handler(ef, sccb, code);
>>
> 
>

Cornelia Huck Aug. 22, 2017, 8:39 a.m. UTC | #7

On Mon, 21 Aug 2017 18:24:15 +0200
Halil Pasic <[e-mail]> 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 <[e-mail]>
> >> Signed-off-by: Cornelia Huck <[e-mail]>
> >> ---
> >>   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).

Halil Pasic Aug. 22, 2017, 9:20 a.m. UTC | #8

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.

Regarding the code. I think we should wait for Pierre.

Regards,
Halil

Cornelia Huck Aug. 22, 2017, 9:39 a.m. UTC | #9

On Tue, 22 Aug 2017 11:20:51 +0200
Halil Pasic <[e-mail]> 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
- have a switch/case over byte 8 with only one case (pci)
- move the pci feature check into the pci code(? - not sure)

There's still the question of when this sclp command first became
available...

Cornelia Huck Aug. 22, 2017, 12:57 p.m. UTC | #10

On Tue, 22 Aug 2017 11:39:14 +0200
Cornelia Huck <[e-mail]> wrote:

> On Tue, 22 Aug 2017 11:20:51 +0200
> Halil Pasic <[e-mail]> 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
> - have a switch/case over byte 8 with only one case (pci)

- switch to response code 0x06f0 instead of invalid command

> - move the pci feature check into the pci code(? - not sure)
> 
> There's still the question of when this sclp command first became
> available...

...because we probably want to indicate invalid command for older
machine types (or is there another facility bit?)

Another question: There's the sclp facilities bit SCLP_HAS_PCI_RECONFIG
- is that really pci, or I/O adapter as for the actual commands?

[The Linux kernel uses the _PCI naming scheme, so I can't derive
anything from that.]

Halil Pasic Aug. 22, 2017, 12:58 p.m. UTC | #11

On 08/22/2017 11:39 AM, Cornelia Huck wrote:
> On Tue, 22 Aug 2017 11:20:51 +0200
> Halil Pasic <[e-mail]> 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.

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

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.

I would appreciate someone with AR access double checking.

> 
> 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 don't know where should I look for the historical details
which go beyond the AR.

Halil

Halil Pasic Aug. 22, 2017, 1 p.m. UTC | #12

On 08/22/2017 02:57 PM, Cornelia Huck wrote:
> On Tue, 22 Aug 2017 11:39:14 +0200
> Cornelia Huck <[e-mail]> wrote:
> 
>> On Tue, 22 Aug 2017 11:20:51 +0200
>> Halil Pasic <[e-mail]> 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
>> - have a switch/case over byte 8 with only one case (pci)
> 
> - switch to response code 0x06f0 instead of invalid command
> 
>> - move the pci feature check into the pci code(? - not sure)
>>
>> There's still the question of when this sclp command first became
>> available...
> 
> ...because we probably want to indicate invalid command for older
> machine types (or is there another facility bit?)
> 
> Another question: There's the sclp facilities bit SCLP_HAS_PCI_RECONFIG
> - is that really pci, or I/O adapter as for the actual commands?
> 
> [The Linux kernel uses the _PCI naming scheme, so I can't derive
> anything from that.]
> 

Ah, we to each other almost simultaneously... See my other mail.

Cornelia Huck Aug. 22, 2017, 1:24 p.m. UTC | #13

On Tue, 22 Aug 2017 14:58:37 +0200
Halil Pasic <[e-mail]> wrote:

> On 08/22/2017 11:39 AM, Cornelia Huck wrote:
> > On Tue, 22 Aug 2017 11:20:51 +0200
> > Halil Pasic <[e-mail]> 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.

Halil Pasic Aug. 22, 2017, 1:54 p.m. UTC | #14

On 08/22/2017 03:24 PM, Cornelia Huck wrote:
> On Tue, 22 Aug 2017 14:58:37 +0200
> Halil Pasic <[e-mail]> wrote:
> 
>> On 08/22/2017 11:39 AM, Cornelia Huck wrote:
>>> On Tue, 22 Aug 2017 11:20:51 +0200
>>> Halil Pasic <[e-mail]> 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.
> 

nod

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

It (SCLP_HAS_PCI_RECONFIG) exactly  covers adapter reconfiguration.
That's what I tried to say with the paragraph above.

> 
>>
>> I would appreciate someone with AR access double checking.
> 
> I'd have hoped you had AR access :p

Yes, my statements are based solely on my reading of the AR (me
still lacks druid-knowledge). What I've asked for is a second
opinion (because AR-s are a twisty maze).

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

Yes. The SCLP facility with the facility code 33 (aka SCLP_HAS_PCI_RECONFIG).
It is the dedicated facility.

I don't think zPCI architecturally implies the presence of this SCLP
command. And logically I would say it's either the other way around
adapter reconfiguration implies zPCI (because otherwise adapter
reconfiguration would be completely useless) or bidirectional. 

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

IMHO we should SCLP_RC_INVALID_SCLP_COMMAND iff we don't provide
SCLP_HAS_PCI_RECONFIG (which has bad name again s/PCI/IOA).

I don't know of any facility except basic SCLP on which 
SCLP_HAS_PCI_RECONFIG depends on. 

For me both presenting and not presenting SCLP_HAS_PCI_RECONFIG
to the guest (via read SCP info SCLP command) in the absence of
the zPCI feature makes sense. The late because of the likely historical
reasons, the former because I think the AR allows it and it gives us more
flexibility.

>

Cornelia Huck Aug. 22, 2017, 2:06 p.m. UTC | #15

On Tue, 22 Aug 2017 15:24:34 +0200
Cornelia Huck <[e-mail]> wrote:

> On Tue, 22 Aug 2017 14:58:37 +0200
> Halil Pasic <[e-mail]> wrote:
> 
> > On 08/22/2017 11:39 AM, Cornelia Huck wrote:  
> > > On Tue, 22 Aug 2017 11:20:51 +0200
> > > Halil Pasic <[e-mail]> wrote:

> > >> 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, I need one more piece of information. 

We obviously need to check whether the sccb we got is long enough
before we try to access the command-specific field. How long is the
sccb supposed to be for configure I/O adapter? For pci, 16 bytes; in
general, I would guess that it needs to include at least atype and some
placeholder for the payload. What does the AR say?

Looking at the pci code, I also noted that it cheerfully uses the aid
field of the sccb before checking whether it is actually long enough...

Cornelia Huck Aug. 22, 2017, 2:15 p.m. UTC | #16

On Tue, 22 Aug 2017 15:54:32 +0200
Halil Pasic <[e-mail]> wrote:

> On 08/22/2017 03:24 PM, Cornelia Huck wrote:
> > On Tue, 22 Aug 2017 14:58:37 +0200
> > Halil Pasic <[e-mail]> wrote:

> >> 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?)  
> 
> It (SCLP_HAS_PCI_RECONFIG) exactly  covers adapter reconfiguration.
> That's what I tried to say with the paragraph above.

Sorry, I did not get that before. So we have another confusing name...

I'll just provide SCLP_HAS_PCI_RECONFIG unconditionally. Maybe
s/PCI/IOA/ here as well?

> 
> >   
> >>
> >> I would appreciate someone with AR access double checking.  
> > 
> > I'd have hoped you had AR access :p  
> 
> Yes, my statements are based solely on my reading of the AR (me
> still lacks druid-knowledge). What I've asked for is a second
> opinion (because AR-s are a twisty maze).

Be careful that you don't get eaten by a grue.

> 
> >   
> >>  
> >>>
> >>> 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?  
> 
> Yes. The SCLP facility with the facility code 33 (aka SCLP_HAS_PCI_RECONFIG).
> It is the dedicated facility.

OK.

> 
> I don't think zPCI architecturally implies the presence of this SCLP
> command. And logically I would say it's either the other way around
> adapter reconfiguration implies zPCI (because otherwise adapter
> reconfiguration would be completely useless) or bidirectional. 

Not sure how useful pci would be without this. I'll just assume that we
have the facility, regardless whether pci is enabled for that
particular machine or not.

> 
> >   
> >>
> >> 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.  
> 
> IMHO we should SCLP_RC_INVALID_SCLP_COMMAND iff we don't provide
> SCLP_HAS_PCI_RECONFIG (which has bad name again s/PCI/IOA).

Nod.

> 
> I don't know of any facility except basic SCLP on which 
> SCLP_HAS_PCI_RECONFIG depends on. 
> 
> For me both presenting and not presenting SCLP_HAS_PCI_RECONFIG
> to the guest (via read SCP info SCLP command) in the absence of
> the zPCI feature makes sense. The late because of the likely historical
> reasons, the former because I think the AR allows it and it gives us more
> flexibility.

I'll go with always presenting it. We'll just fail with invalid adapter
type for !pci.

Thanks for digging through the AR!

Halil Pasic Aug. 22, 2017, 2:27 p.m. UTC | #17

On 08/22/2017 04:06 PM, Cornelia Huck wrote:
> On Tue, 22 Aug 2017 15:24:34 +0200
> Cornelia Huck <[e-mail]> wrote:
> 
>> On Tue, 22 Aug 2017 14:58:37 +0200
>> Halil Pasic <[e-mail]> wrote:
>>
>>> On 08/22/2017 11:39 AM, Cornelia Huck wrote:  
>>>> On Tue, 22 Aug 2017 11:20:51 +0200
>>>> Halil Pasic <[e-mail]> wrote:
> 
>>>>> 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, I need one more piece of information. 
> 
> We obviously need to check whether the sccb we got is long enough
> before we try to access the command-specific field. How long is the
> sccb supposed to be for configure I/O adapter? For pci, 16 bytes; in
> general, I would guess that it needs to include at least atype and some
> placeholder for the payload. What does the AR say?
>

The first 2 bytes of the SCCB designate it's length. For this particular
command it's at least 16 bytes (regardless of pci). The length is
marked as may be changed by the SCLP.
 
 
> Looking at the pci code, I also noted that it cheerfully uses the aid
> field of the sccb before checking whether it is actually long enough...
>

Cornelia Huck Aug. 22, 2017, 2:34 p.m. UTC | #18

On Tue, 22 Aug 2017 16:27:38 +0200
Halil Pasic <[e-mail]> wrote:

> On 08/22/2017 04:06 PM, Cornelia Huck wrote:
> > On Tue, 22 Aug 2017 15:24:34 +0200
> > Cornelia Huck <[e-mail]> wrote:
> >   
> >> On Tue, 22 Aug 2017 14:58:37 +0200
> >> Halil Pasic <[e-mail]> wrote:
> >>  
> >>> On 08/22/2017 11:39 AM, Cornelia Huck wrote:    
> >>>> On Tue, 22 Aug 2017 11:20:51 +0200
> >>>> Halil Pasic <[e-mail]> wrote:  
> >   
> >>>>> 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, I need one more piece of information. 
> > 
> > We obviously need to check whether the sccb we got is long enough
> > before we try to access the command-specific field. How long is the
> > sccb supposed to be for configure I/O adapter? For pci, 16 bytes; in
> > general, I would guess that it needs to include at least atype and some
> > placeholder for the payload. What does the AR say?
> >  
> 
> The first 2 bytes of the SCCB designate it's length. For this particular
> command it's at least 16 bytes (regardless of pci). The length is
> marked as may be changed by the SCLP.

Thanks for the info, this makes implementing it correctly much easier!

>  
>  
> > Looking at the pci code, I also noted that it cheerfully uses the aid
> > field of the sccb before checking whether it is actually long enough...
> >   
>

Halil Pasic Aug. 22, 2017, 2:34 p.m. UTC | #19

On 08/22/2017 04:15 PM, Cornelia Huck wrote:
> On Tue, 22 Aug 2017 15:54:32 +0200
> Halil Pasic <[e-mail]> wrote:
> 
>> On 08/22/2017 03:24 PM, Cornelia Huck wrote:
>>> On Tue, 22 Aug 2017 14:58:37 +0200
>>> Halil Pasic <[e-mail]> wrote:
> 
>>>> 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?)  
>>
>> It (SCLP_HAS_PCI_RECONFIG) exactly  covers adapter reconfiguration.
>> That's what I tried to say with the paragraph above.
> 
> Sorry, I did not get that before. So we have another confusing name...
> 
> I'll just provide SCLP_HAS_PCI_RECONFIG unconditionally. Maybe
> s/PCI/IOA/ here as well?
> 

Yeah, I had the same idea a coupe of lines below.

>>
>>>   
>>>>
>>>> I would appreciate someone with AR access double checking.  
>>>
>>> I'd have hoped you had AR access :p  
>>
>> Yes, my statements are based solely on my reading of the AR (me
>> still lacks druid-knowledge). What I've asked for is a second
>> opinion (because AR-s are a twisty maze).
> 
> Be careful that you don't get eaten by a grue.
> 
>>
>>>   
>>>>  
>>>>>
>>>>> 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?  
>>
>> Yes. The SCLP facility with the facility code 33 (aka SCLP_HAS_PCI_RECONFIG).
>> It is the dedicated facility.
> 
> OK.
> 
>>
>> I don't think zPCI architecturally implies the presence of this SCLP
>> command. And logically I would say it's either the other way around
>> adapter reconfiguration implies zPCI (because otherwise adapter
>> reconfiguration would be completely useless) or bidirectional. 
> 
> Not sure how useful pci would be without this. I'll just assume that we
> have the facility, regardless whether pci is enabled for that
> particular machine or not.

I have no idea if there is another mechanism to put a pci adapter
into a configuration. If there isn't then we can agree on not too
useful.

> 
>>
>>>   
>>>>
>>>> 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.  
>>
>> IMHO we should SCLP_RC_INVALID_SCLP_COMMAND iff we don't provide
>> SCLP_HAS_PCI_RECONFIG (which has bad name again s/PCI/IOA).
> 
> Nod.
> 
>>
>> I don't know of any facility except basic SCLP on which 
>> SCLP_HAS_PCI_RECONFIG depends on. 
>>
>> For me both presenting and not presenting SCLP_HAS_PCI_RECONFIG
>> to the guest (via read SCP info SCLP command) in the absence of
>> the zPCI feature makes sense. The late because of the likely historical
>> reasons, the former because I think the AR allows it and it gives us more
>> flexibility.
> 
> I'll go with always presenting it. We'll just fail with invalid adapter
> type for !pci.
> 
> Thanks for digging through the AR!
> 

You are welcome. I think we are in agreement. Looking forward to v2.

Halil

Patch

diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 9253dbbc64..d0104cd784 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -59,6 +59,7 @@  static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
     int rnsize, rnmax;
     int slots = MIN(machine->ram_slots, s390_get_memslot_count(kvm_state));
     IplParameterBlock *ipib = s390_ipl_get_iplb();
+    uint64_t sclp_facilities = SCLP_HAS_CPU_INFO;
 
     CPU_FOREACH(cpu) {
         cpu_count++;
@@ -79,8 +80,10 @@  static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
 
     prepare_cpu_entries(sclp, read_info->entries, cpu_count);
 
-    read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
-                                        SCLP_HAS_PCI_RECONFIG);
+    if (s390_has_feat(S390_FEAT_ZPCI)) {
+        sclp_facilities |= SCLP_HAS_PCI_RECONFIG;
+    }
+    read_info->facilities = cpu_to_be64(sclp_facilities);
 
     /* Memory Hotplug is only supported for the ccw machine type */
     if (mhd) {
@@ -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);
+        }
         break;
     case SCLP_CMDW_DECONFIGURE_PCI:
-        s390_pci_sclp_deconfigure(sccb);
+        if (s390_has_feat(S390_FEAT_ZPCI)) {
+            s390_pci_sclp_deconfigure(sccb);
+        } else {
+            sccb->h.response_code = cpu_to_be16(SCLP_RC_INVALID_SCLP_COMMAND);
+        }
         break;
     default:
         efc->command_handler(ef, sccb, code);