| Message ID | 20170707122159.24714-6-cohuck@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/07/2017 02:21 PM, Cornelia Huck wrote: > If a guest running on a non-pci build issues a pci instruction, > throw them an exception. > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > --- > target/s390x/kvm.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index a3d00196f4..c5c7c27a21 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -1160,6 +1160,9 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run) > { > uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16; > > +#ifndef CONFIG_PCI > + return -1; > +#endif Instead of this ifdefing, can you use the cpu model to decide if the instruction should be available? We need to do this anyway for proper handling. You can then fence off the PCI bits in the CPU model for CONFIG_PCI == off.
On 07.07.2017 14:21, Cornelia Huck wrote: > If a guest running on a non-pci build issues a pci instruction, > throw them an exception. > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > --- > target/s390x/kvm.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index a3d00196f4..c5c7c27a21 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -1160,6 +1160,9 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run) > { > uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16; > > +#ifndef CONFIG_PCI > + return -1; > +#endif > return clp_service_call(cpu, r2); > } > > @@ -1168,6 +1171,9 @@ static int kvm_pcilg_service_call(S390CPU *cpu, struct kvm_run *run) > uint8_t r1 = (run->s390_sieic.ipb & 0x00f00000) >> 20; > uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16; > > +#ifndef CONFIG_PCI > + return -1; > +#endif > return pcilg_service_call(cpu, r1, r2); > } pcilg_service_call() seems to be defined in s390-pci-inst.c ... which you later remove from the !CONFIG_PCI builds... so I wonder why this still compiles ... I guess GCC is smart enough to optimize it away. Anyway, to be on the safe side (and to be able to compile with -O0), you should maybe rather do this instead: #ifndef CONFIG_PCI return -1; #else return pcilg_service_call(cpu, r1, r2); #endif ? Thomas
On Fri, 7 Jul 2017 14:55:23 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 07/07/2017 02:21 PM, Cornelia Huck wrote: > > If a guest running on a non-pci build issues a pci instruction, > > throw them an exception. > > > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > > --- > > target/s390x/kvm.c | 24 ++++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > > index a3d00196f4..c5c7c27a21 100644 > > --- a/target/s390x/kvm.c > > +++ b/target/s390x/kvm.c > > @@ -1160,6 +1160,9 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run) > > { > > uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16; > > > > +#ifndef CONFIG_PCI > > + return -1; > > +#endif > > Instead of this ifdefing, can you use the cpu model to decide if the instruction > should be available? We need to do this anyway for proper handling. > > You can then fence off the PCI bits in the CPU model for > CONFIG_PCI == off. Sounds like a good idea, I'll give it a try. We'll probably also want to fence off the sclp facility bit via that mechanism.
On Fri, 7 Jul 2017 15:00:20 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 07.07.2017 14:21, Cornelia Huck wrote: > > If a guest running on a non-pci build issues a pci instruction, > > throw them an exception. > > > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > > --- > > target/s390x/kvm.c | 24 ++++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > > index a3d00196f4..c5c7c27a21 100644 > > --- a/target/s390x/kvm.c > > +++ b/target/s390x/kvm.c > > @@ -1160,6 +1160,9 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run) > > { > > uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16; > > > > +#ifndef CONFIG_PCI > > + return -1; > > +#endif > > return clp_service_call(cpu, r2); > > } > > > > @@ -1168,6 +1171,9 @@ static int kvm_pcilg_service_call(S390CPU *cpu, struct kvm_run *run) > > uint8_t r1 = (run->s390_sieic.ipb & 0x00f00000) >> 20; > > uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16; > > > > +#ifndef CONFIG_PCI > > + return -1; > > +#endif > > return pcilg_service_call(cpu, r1, r2); > > } > > pcilg_service_call() seems to be defined in s390-pci-inst.c ... which > you later remove from the !CONFIG_PCI builds... so I wonder why this > still compiles ... I guess GCC is smart enough to optimize it away. Yes, as the second return is not reachable anymore. > Anyway, to be on the safe side (and to be able to compile with -O0), you > should maybe rather do this instead: > > #ifndef CONFIG_PCI > return -1; > #else > return pcilg_service_call(cpu, r1, r2); > #endif > > ? I'll try Christian's suggestion to use the cpu model first. However, we'll need some ifdeffery somewhere... maybe we should introduce a zpci-stub.c for the referenced functions.
On Fri, 7 Jul 2017 15:04:52 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Fri, 7 Jul 2017 14:55:23 +0200 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > On 07/07/2017 02:21 PM, Cornelia Huck wrote: > > > If a guest running on a non-pci build issues a pci instruction, > > > throw them an exception. > > > > > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > > > --- > > > target/s390x/kvm.c | 24 ++++++++++++++++++++++++ > > > 1 file changed, 24 insertions(+) > > > > > > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > > > index a3d00196f4..c5c7c27a21 100644 > > > --- a/target/s390x/kvm.c > > > +++ b/target/s390x/kvm.c > > > @@ -1160,6 +1160,9 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run) > > > { > > > uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16; > > > > > > +#ifndef CONFIG_PCI > > > + return -1; > > > +#endif > > > > Instead of this ifdefing, can you use the cpu model to decide if the instruction > > should be available? We need to do this anyway for proper handling. > > > > You can then fence off the PCI bits in the CPU model for > > CONFIG_PCI == off. > > Sounds like a good idea, I'll give it a try. > > We'll probably also want to fence off the sclp facility bit via that > mechanism. Slight problem here... we don't have the relevant facilities defined yet, and they are not in the POP (other than "Assigned to IBM internal use"). While I'm pretty sure that the magic number is 69 (judging from the Linux code), I think they should be introduced in a patch by someone who has access to the documentation including the proper names.
On 07/10/2017 01:04 PM, Cornelia Huck wrote: > On Fri, 7 Jul 2017 15:04:52 +0200 > Cornelia Huck <cohuck@redhat.com> wrote: > >> On Fri, 7 Jul 2017 14:55:23 +0200 >> Christian Borntraeger <borntraeger@de.ibm.com> wrote: >> >>> On 07/07/2017 02:21 PM, Cornelia Huck wrote: >>>> If a guest running on a non-pci build issues a pci instruction, >>>> throw them an exception. >>>> >>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> >>>> --- >>>> target/s390x/kvm.c | 24 ++++++++++++++++++++++++ >>>> 1 file changed, 24 insertions(+) >>>> >>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >>>> index a3d00196f4..c5c7c27a21 100644 >>>> --- a/target/s390x/kvm.c >>>> +++ b/target/s390x/kvm.c >>>> @@ -1160,6 +1160,9 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run) >>>> { >>>> uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16; >>>> >>>> +#ifndef CONFIG_PCI >>>> + return -1; >>>> +#endif >>> >>> Instead of this ifdefing, can you use the cpu model to decide if the instruction >>> should be available? We need to do this anyway for proper handling. >>> >>> You can then fence off the PCI bits in the CPU model for >>> CONFIG_PCI == off. >> >> Sounds like a good idea, I'll give it a try. >> >> We'll probably also want to fence off the sclp facility bit via that >> mechanism. > > Slight problem here... we don't have the relevant facilities defined > yet, and they are not in the POP (other than "Assigned to IBM internal > use"). > > While I'm pretty sure that the magic number is 69 (judging from the > Linux code), I think they should be introduced in a patch by someone > who has access to the documentation including the proper names. I will try to get some patches out for PCI in the next days that will contain the PCI related facilities.
On Mon, 10 Jul 2017 14:41:47 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 07/10/2017 01:04 PM, Cornelia Huck wrote: > > On Fri, 7 Jul 2017 15:04:52 +0200 > > Cornelia Huck <cohuck@redhat.com> wrote: > > > >> On Fri, 7 Jul 2017 14:55:23 +0200 > >> Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> > >>> On 07/07/2017 02:21 PM, Cornelia Huck wrote: > >>>> If a guest running on a non-pci build issues a pci instruction, > >>>> throw them an exception. > >>>> > >>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com> > >>>> --- > >>>> target/s390x/kvm.c | 24 ++++++++++++++++++++++++ > >>>> 1 file changed, 24 insertions(+) > >>>> > >>>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > >>>> index a3d00196f4..c5c7c27a21 100644 > >>>> --- a/target/s390x/kvm.c > >>>> +++ b/target/s390x/kvm.c > >>>> @@ -1160,6 +1160,9 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run) > >>>> { > >>>> uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16; > >>>> > >>>> +#ifndef CONFIG_PCI > >>>> + return -1; > >>>> +#endif > >>> > >>> Instead of this ifdefing, can you use the cpu model to decide if the instruction > >>> should be available? We need to do this anyway for proper handling. > >>> > >>> You can then fence off the PCI bits in the CPU model for > >>> CONFIG_PCI == off. > >> > >> Sounds like a good idea, I'll give it a try. > >> > >> We'll probably also want to fence off the sclp facility bit via that > >> mechanism. > > > > Slight problem here... we don't have the relevant facilities defined > > yet, and they are not in the POP (other than "Assigned to IBM internal > > use"). > > > > While I'm pretty sure that the magic number is 69 (judging from the > > Linux code), I think they should be introduced in a patch by someone > > who has access to the documentation including the proper names. > > I will try to get some patches out for PCI in the next days that will contain > the PCI related facilities. > Cool, thanks!
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index a3d00196f4..c5c7c27a21 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -1160,6 +1160,9 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run) { uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16; +#ifndef CONFIG_PCI + return -1; +#endif return clp_service_call(cpu, r2); } @@ -1168,6 +1171,9 @@ static int kvm_pcilg_service_call(S390CPU *cpu, struct kvm_run *run) uint8_t r1 = (run->s390_sieic.ipb & 0x00f00000) >> 20; uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16; +#ifndef CONFIG_PCI + return -1; +#endif return pcilg_service_call(cpu, r1, r2); } @@ -1176,6 +1182,9 @@ static int kvm_pcistg_service_call(S390CPU *cpu, struct kvm_run *run) uint8_t r1 = (run->s390_sieic.ipb & 0x00f00000) >> 20; uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16; +#ifndef CONFIG_PCI + return -1; +#endif return pcistg_service_call(cpu, r1, r2); } @@ -1188,11 +1197,17 @@ static int kvm_stpcifc_service_call(S390CPU *cpu, struct kvm_run *run) cpu_synchronize_state(CPU(cpu)); fiba = get_base_disp_rxy(cpu, run, &ar); +#ifndef CONFIG_PCI + return -1; +#endif return stpcifc_service_call(cpu, r1, fiba, ar); } static int kvm_sic_service_call(S390CPU *cpu, struct kvm_run *run) { +#ifndef CONFIG_PCI + return -1; +#endif /* NOOP */ return 0; } @@ -1202,6 +1217,9 @@ static int kvm_rpcit_service_call(S390CPU *cpu, struct kvm_run *run) uint8_t r1 = (run->s390_sieic.ipb & 0x00f00000) >> 20; uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16; +#ifndef CONFIG_PCI + return -1; +#endif return rpcit_service_call(cpu, r1, r2); } @@ -1212,6 +1230,9 @@ static int kvm_pcistb_service_call(S390CPU *cpu, struct kvm_run *run) uint64_t gaddr; uint8_t ar; +#ifndef CONFIG_PCI + return -1; +#endif cpu_synchronize_state(CPU(cpu)); gaddr = get_base_disp_rsy(cpu, run, &ar); @@ -1224,6 +1245,9 @@ static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run) uint64_t fiba; uint8_t ar; +#ifndef CONFIG_PCI + return -1; +#endif cpu_synchronize_state(CPU(cpu)); fiba = get_base_disp_rxy(cpu, run, &ar);
If a guest running on a non-pci build issues a pci instruction, throw them an exception. Signed-off-by: Cornelia Huck <cohuck@redhat.com> --- target/s390x/kvm.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)