| Message ID | 1432511251-22515-8-git-send-email-aurelien@aurel32.net |
|---|---|
| State | New |
| Headers | show |
On 25.05.15 01:47, Aurelien Jarno wrote: > Cc: Alexander Graf <agraf@suse.de> > Cc: Richard Henderson <rth@twiddle.net> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> Shouldn't this get populated based on the selected -cpu type? Alex > --- > target-s390x/cpu.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h > index 8bda2e0..35bfdec 100644 > --- a/target-s390x/cpu.h > +++ b/target-s390x/cpu.h > @@ -169,7 +169,7 @@ typedef struct CPUS390XState { > static const uint64_t facilities_dw[] = { > (1ULL << 63) | /* b 0: z/Architecture new instructions added to ESA/390 */ > (1ULL << 62) | /* b 1: z/Architecture architectural */ > - (0ULL << 61) | /* b 2: z/Architecture architectural active */ > + (1ULL << 61) | /* b 2: z/Architecture architectural active */ > (0ULL << 60) | /* b 3: IDTE */ > (0ULL << 59) | /* b 4: IDTE selective clearing when segtab invalidated */ > (0ULL << 58) | /* b 5: IDTE selective clearing when regtab invalidated */ > @@ -188,7 +188,7 @@ static const uint64_t facilities_dw[] = { > (0ULL << 45) | /* b18: Long-displacement facility */ > (0ULL << 44) | /* b19: High performance long-displacement facility */ > (0ULL << 43) | /* b20: HFP-multiply-and-add/subtract facility */ > - (0ULL << 42) | /* b21: Extended-immediate facility */ > + (1ULL << 42) | /* b21: Extended-immediate facility */ > (0ULL << 41) | /* b22: Extended-translation facility 3 */ > (0ULL << 40) | /* b23: HFP-unnormalized-extension facility */ > (0ULL << 39) | /* b24: ETF2-enhancement facility */ > @@ -201,7 +201,7 @@ static const uint64_t facilities_dw[] = { > (0ULL << 31) | /* b32: Compare-and-swap-and-store facility */ > (0ULL << 30) | /* b33: Compare-and-swap-and-store facility 2 */ > (0ULL << 29) | /* b34: General-instructions-extension facility */ > - (0ULL << 28) | /* b35: Execute-extensions facility */ > + (1ULL << 28) | /* b35: Execute-extensions facility */ > (0ULL << 27) | /* b36: Enhanced-monitor facility */ > (0ULL << 26) | /* b37: Floating-point extension facility */ > (0ULL << 24) | /* b39: IBM internal use */ >
On 2015-05-25 22:39, Alexander Graf wrote: > > > On 25.05.15 01:47, Aurelien Jarno wrote: > > Cc: Alexander Graf <agraf@suse.de> > > Cc: Richard Henderson <rth@twiddle.net> > > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> > > Shouldn't this get populated based on the selected -cpu type? In the long term yes, but given we only implement one CPU type (or rather none) in TCG mode, we can consider that's already the case.
On 25.05.15 23:02, Aurelien Jarno wrote: > On 2015-05-25 22:39, Alexander Graf wrote: >> >> >> On 25.05.15 01:47, Aurelien Jarno wrote: >>> Cc: Alexander Graf <agraf@suse.de> >>> Cc: Richard Henderson <rth@twiddle.net> >>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> >> >> Shouldn't this get populated based on the selected -cpu type? > > In the long term yes, but given we only implement one CPU type (or > rather none) in TCG mode, we can consider that's already the case. There are patches coming from IBM to at least add a list of a good number of s390x cpu types. I'd really like to make use of that and have actual CPU types selectable. At least let's move towards that model. So the code in question should take the facility capabilities from the first cpu object (or the class?) for example and we bump it to the currently supported feature set in there. Alex
On 2015-05-25 23:04, Alexander Graf wrote: > > > On 25.05.15 23:02, Aurelien Jarno wrote: > > On 2015-05-25 22:39, Alexander Graf wrote: > >> > >> > >> On 25.05.15 01:47, Aurelien Jarno wrote: > >>> Cc: Alexander Graf <agraf@suse.de> > >>> Cc: Richard Henderson <rth@twiddle.net> > >>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> > >> > >> Shouldn't this get populated based on the selected -cpu type? > > > > In the long term yes, but given we only implement one CPU type (or > > rather none) in TCG mode, we can consider that's already the case. > > There are patches coming from IBM to at least add a list of a good > number of s390x cpu types. I'd really like to make use of that and have > actual CPU types selectable. I guess they are for the KVM mode. Do they provide the corresponding facilities list? Probably otherwise that doesn't really differentiate various CPUs. Please make sure of that when reviewing these patches. > At least let's move towards that model. So the code in question should > take the facility capabilities from the first cpu object (or the class?) > for example and we bump it to the currently supported feature set in there. Yes, that would work for STFL/STFLE, though we should have a list of facilities implemented by TCG so we can mask out the non-implemented facilities. This basically corresponds to the informations provided by the current patch. That said that won't work for actually disabling the corresponding instructions as we don't have a 1 to 1 mapping between the facilities and the group of instructions. Anyway we don't even check that right now.
On 25.05.15 23:13, Aurelien Jarno wrote: > On 2015-05-25 23:04, Alexander Graf wrote: >> >> >> On 25.05.15 23:02, Aurelien Jarno wrote: >>> On 2015-05-25 22:39, Alexander Graf wrote: >>>> >>>> >>>> On 25.05.15 01:47, Aurelien Jarno wrote: >>>>> Cc: Alexander Graf <agraf@suse.de> >>>>> Cc: Richard Henderson <rth@twiddle.net> >>>>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> >>>> >>>> Shouldn't this get populated based on the selected -cpu type? >>> >>> In the long term yes, but given we only implement one CPU type (or >>> rather none) in TCG mode, we can consider that's already the case. >> >> There are patches coming from IBM to at least add a list of a good >> number of s390x cpu types. I'd really like to make use of that and have >> actual CPU types selectable. > > I guess they are for the KVM mode. Do they provide the corresponding > facilities list? Probably otherwise that doesn't really differentiate > various CPUs. Please make sure of that when reviewing these patches. I could definitely use help on review - it's probably my weakest point ;). >> At least let's move towards that model. So the code in question should >> take the facility capabilities from the first cpu object (or the class?) >> for example and we bump it to the currently supported feature set in there. > > Yes, that would work for STFL/STFLE, though we should have a list of > facilities implemented by TCG so we can mask out the non-implemented > facilities. This basically corresponds to the informations provided by > the current patch. Ah, so you consider the current list the "these are the features TCG knows about" list? > That said that won't work for actually disabling the corresponding > instructions as we don't have a 1 to 1 mapping between the facilities > and the group of instructions. Anyway we don't even check that right > now. I agree, but the TCG code annotates which facility each opcode belongs to which means actually limiting it should become trivial. That's really all I'm asking for - I want to see the light at the end of the tunnel ;). Alex
On 2015-05-25 23:47, Alexander Graf wrote: > > > On 25.05.15 23:13, Aurelien Jarno wrote: > > On 2015-05-25 23:04, Alexander Graf wrote: > >> > >> > >> On 25.05.15 23:02, Aurelien Jarno wrote: > >>> On 2015-05-25 22:39, Alexander Graf wrote: > >>>> > >>>> > >>>> On 25.05.15 01:47, Aurelien Jarno wrote: > >>>>> Cc: Alexander Graf <agraf@suse.de> > >>>>> Cc: Richard Henderson <rth@twiddle.net> > >>>>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> > >>>> > >>>> Shouldn't this get populated based on the selected -cpu type? > >>> > >>> In the long term yes, but given we only implement one CPU type (or > >>> rather none) in TCG mode, we can consider that's already the case. > >> > >> There are patches coming from IBM to at least add a list of a good > >> number of s390x cpu types. I'd really like to make use of that and have > >> actual CPU types selectable. > > > > I guess they are for the KVM mode. Do they provide the corresponding > > facilities list? Probably otherwise that doesn't really differentiate > > various CPUs. Please make sure of that when reviewing these patches. > > I could definitely use help on review - it's probably my weakest point ;). > > >> At least let's move towards that model. So the code in question should > >> take the facility capabilities from the first cpu object (or the class?) > >> for example and we bump it to the currently supported feature set in there. > > > > Yes, that would work for STFL/STFLE, though we should have a list of > > facilities implemented by TCG so we can mask out the non-implemented > > facilities. This basically corresponds to the informations provided by > > the current patch. > > Ah, so you consider the current list the "these are the features TCG > knows about" list? > > > That said that won't work for actually disabling the corresponding > > instructions as we don't have a 1 to 1 mapping between the facilities > > and the group of instructions. Anyway we don't even check that right > > now. > > I agree, but the TCG code annotates which facility each opcode belongs > to which means actually limiting it should become trivial. That's really > all I'm asking for - I want to see the light at the end of the tunnel ;). It's trivial doing so for the facilities annotated in TCG. Just that they don't match one to one with the facilities bits in STFL/STFLE. Some bits enable multiple facilities and QEMU has also grouped some facilities together. Also some bits do not actually concern instructions but rather MMU features. Some other gives additional properties to a facility: some facilities might be present by disabled, some other might have a slow or fast implementation. We therefore need a conversion function before being able to do that, and we need to know which format IBM is going to provide in their patches: list of facilities or STFL/STFLE bits?
On 26.05.15 08:02, Aurelien Jarno wrote: > On 2015-05-25 23:47, Alexander Graf wrote: >> >> >> On 25.05.15 23:13, Aurelien Jarno wrote: >>> On 2015-05-25 23:04, Alexander Graf wrote: >>>> >>>> >>>> On 25.05.15 23:02, Aurelien Jarno wrote: >>>>> On 2015-05-25 22:39, Alexander Graf wrote: >>>>>> >>>>>> >>>>>> On 25.05.15 01:47, Aurelien Jarno wrote: >>>>>>> Cc: Alexander Graf <agraf@suse.de> >>>>>>> Cc: Richard Henderson <rth@twiddle.net> >>>>>>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> >>>>>> >>>>>> Shouldn't this get populated based on the selected -cpu type? >>>>> >>>>> In the long term yes, but given we only implement one CPU type (or >>>>> rather none) in TCG mode, we can consider that's already the case. >>>> >>>> There are patches coming from IBM to at least add a list of a good >>>> number of s390x cpu types. I'd really like to make use of that and have >>>> actual CPU types selectable. >>> >>> I guess they are for the KVM mode. Do they provide the corresponding >>> facilities list? Probably otherwise that doesn't really differentiate >>> various CPUs. Please make sure of that when reviewing these patches. >> >> I could definitely use help on review - it's probably my weakest point ;). >> >>>> At least let's move towards that model. So the code in question should >>>> take the facility capabilities from the first cpu object (or the class?) >>>> for example and we bump it to the currently supported feature set in there. >>> >>> Yes, that would work for STFL/STFLE, though we should have a list of >>> facilities implemented by TCG so we can mask out the non-implemented >>> facilities. This basically corresponds to the informations provided by >>> the current patch. >> >> Ah, so you consider the current list the "these are the features TCG >> knows about" list? >> >>> That said that won't work for actually disabling the corresponding >>> instructions as we don't have a 1 to 1 mapping between the facilities >>> and the group of instructions. Anyway we don't even check that right >>> now. >> >> I agree, but the TCG code annotates which facility each opcode belongs >> to which means actually limiting it should become trivial. That's really >> all I'm asking for - I want to see the light at the end of the tunnel ;). > > It's trivial doing so for the facilities annotated in TCG. Just that > they don't match one to one with the facilities bits in STFL/STFLE. Some > bits enable multiple facilities and QEMU has also grouped some > facilities together. Also some bits do not actually concern instructions > but rather MMU features. Some other gives additional properties to a > facility: some facilities might be present by disabled, some other might > have a slow or fast implementation. > > We therefore need a conversion function before being able to do that, > and we need to know which format IBM is going to provide in their > patches: list of facilities or STFL/STFLE bits? Please check out this patch set: https://lists.nongnu.org/archive/html/qemu-devel/2015-04/msg03436.html Alex
On 2015-05-26 10:29, Alexander Graf wrote: > > > On 26.05.15 08:02, Aurelien Jarno wrote: > > On 2015-05-25 23:47, Alexander Graf wrote: > >> > >> > >> On 25.05.15 23:13, Aurelien Jarno wrote: > >>> On 2015-05-25 23:04, Alexander Graf wrote: > >>>> > >>>> > >>>> On 25.05.15 23:02, Aurelien Jarno wrote: > >>>>> On 2015-05-25 22:39, Alexander Graf wrote: > >>>>>> > >>>>>> > >>>>>> On 25.05.15 01:47, Aurelien Jarno wrote: > >>>>>>> Cc: Alexander Graf <agraf@suse.de> > >>>>>>> Cc: Richard Henderson <rth@twiddle.net> > >>>>>>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> > >>>>>> > >>>>>> Shouldn't this get populated based on the selected -cpu type? > >>>>> > >>>>> In the long term yes, but given we only implement one CPU type (or > >>>>> rather none) in TCG mode, we can consider that's already the case. > >>>> > >>>> There are patches coming from IBM to at least add a list of a good > >>>> number of s390x cpu types. I'd really like to make use of that and have > >>>> actual CPU types selectable. > >>> > >>> I guess they are for the KVM mode. Do they provide the corresponding > >>> facilities list? Probably otherwise that doesn't really differentiate > >>> various CPUs. Please make sure of that when reviewing these patches. > >> > >> I could definitely use help on review - it's probably my weakest point ;). > >> > >>>> At least let's move towards that model. So the code in question should > >>>> take the facility capabilities from the first cpu object (or the class?) > >>>> for example and we bump it to the currently supported feature set in there. > >>> > >>> Yes, that would work for STFL/STFLE, though we should have a list of > >>> facilities implemented by TCG so we can mask out the non-implemented > >>> facilities. This basically corresponds to the informations provided by > >>> the current patch. > >> > >> Ah, so you consider the current list the "these are the features TCG > >> knows about" list? > >> > >>> That said that won't work for actually disabling the corresponding > >>> instructions as we don't have a 1 to 1 mapping between the facilities > >>> and the group of instructions. Anyway we don't even check that right > >>> now. > >> > >> I agree, but the TCG code annotates which facility each opcode belongs > >> to which means actually limiting it should become trivial. That's really > >> all I'm asking for - I want to see the light at the end of the tunnel ;). > > > > It's trivial doing so for the facilities annotated in TCG. Just that > > they don't match one to one with the facilities bits in STFL/STFLE. Some > > bits enable multiple facilities and QEMU has also grouped some > > facilities together. Also some bits do not actually concern instructions > > but rather MMU features. Some other gives additional properties to a > > facility: some facilities might be present by disabled, some other might > > have a slow or fast implementation. > > > > We therefore need a conversion function before being able to do that, > > and we need to know which format IBM is going to provide in their > > patches: list of facilities or STFL/STFLE bits? > > Please check out this patch set: > > https://lists.nongnu.org/archive/html/qemu-devel/2015-04/msg03436.html Thanks, at a first glance it seems we have what we need. I'll try to base my STFL/STFLE patches on this patchset in the next days.
On 05/26/2015 11:05 AM, Aurelien Jarno wrote: > On 2015-05-26 10:29, Alexander Graf wrote: >> >> On 26.05.15 08:02, Aurelien Jarno wrote: >>> On 2015-05-25 23:47, Alexander Graf wrote: >>>> >>>> On 25.05.15 23:13, Aurelien Jarno wrote: >>>>> On 2015-05-25 23:04, Alexander Graf wrote: >>>>>> >>>>>> On 25.05.15 23:02, Aurelien Jarno wrote: >>>>>>> On 2015-05-25 22:39, Alexander Graf wrote: >>>>>>>> >>>>>>>> On 25.05.15 01:47, Aurelien Jarno wrote: >>>>>>>>> Cc: Alexander Graf <agraf@suse.de> >>>>>>>>> Cc: Richard Henderson <rth@twiddle.net> >>>>>>>>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> >>>>>>>> Shouldn't this get populated based on the selected -cpu type? >>>>>>> In the long term yes, but given we only implement one CPU type (or >>>>>>> rather none) in TCG mode, we can consider that's already the case. >>>>>> There are patches coming from IBM to at least add a list of a good >>>>>> number of s390x cpu types. I'd really like to make use of that and have >>>>>> actual CPU types selectable. >>>>> I guess they are for the KVM mode. Do they provide the corresponding >>>>> facilities list? Probably otherwise that doesn't really differentiate >>>>> various CPUs. Please make sure of that when reviewing these patches. >>>> I could definitely use help on review - it's probably my weakest point ;). >>>> >>>>>> At least let's move towards that model. So the code in question should >>>>>> take the facility capabilities from the first cpu object (or the class?) >>>>>> for example and we bump it to the currently supported feature set in there. >>>>> Yes, that would work for STFL/STFLE, though we should have a list of >>>>> facilities implemented by TCG so we can mask out the non-implemented >>>>> facilities. This basically corresponds to the informations provided by >>>>> the current patch. >>>> Ah, so you consider the current list the "these are the features TCG >>>> knows about" list? >>>> >>>>> That said that won't work for actually disabling the corresponding >>>>> instructions as we don't have a 1 to 1 mapping between the facilities >>>>> and the group of instructions. Anyway we don't even check that right >>>>> now. >>>> I agree, but the TCG code annotates which facility each opcode belongs >>>> to which means actually limiting it should become trivial. That's really >>>> all I'm asking for - I want to see the light at the end of the tunnel ;). >>> It's trivial doing so for the facilities annotated in TCG. Just that >>> they don't match one to one with the facilities bits in STFL/STFLE. Some >>> bits enable multiple facilities and QEMU has also grouped some >>> facilities together. Also some bits do not actually concern instructions >>> but rather MMU features. Some other gives additional properties to a >>> facility: some facilities might be present by disabled, some other might >>> have a slow or fast implementation. >>> >>> We therefore need a conversion function before being able to do that, >>> and we need to know which format IBM is going to provide in their >>> patches: list of facilities or STFL/STFLE bits? >> Please check out this patch set: >> >> https://lists.nongnu.org/archive/html/qemu-devel/2015-04/msg03436.html > Thanks, at a first glance it seems we have what we need. I'll try > to base my STFL/STFLE patches on this patchset in the next days. Awesome, thanks a lot! Alex
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h index 8bda2e0..35bfdec 100644 --- a/target-s390x/cpu.h +++ b/target-s390x/cpu.h @@ -169,7 +169,7 @@ typedef struct CPUS390XState { static const uint64_t facilities_dw[] = { (1ULL << 63) | /* b 0: z/Architecture new instructions added to ESA/390 */ (1ULL << 62) | /* b 1: z/Architecture architectural */ - (0ULL << 61) | /* b 2: z/Architecture architectural active */ + (1ULL << 61) | /* b 2: z/Architecture architectural active */ (0ULL << 60) | /* b 3: IDTE */ (0ULL << 59) | /* b 4: IDTE selective clearing when segtab invalidated */ (0ULL << 58) | /* b 5: IDTE selective clearing when regtab invalidated */ @@ -188,7 +188,7 @@ static const uint64_t facilities_dw[] = { (0ULL << 45) | /* b18: Long-displacement facility */ (0ULL << 44) | /* b19: High performance long-displacement facility */ (0ULL << 43) | /* b20: HFP-multiply-and-add/subtract facility */ - (0ULL << 42) | /* b21: Extended-immediate facility */ + (1ULL << 42) | /* b21: Extended-immediate facility */ (0ULL << 41) | /* b22: Extended-translation facility 3 */ (0ULL << 40) | /* b23: HFP-unnormalized-extension facility */ (0ULL << 39) | /* b24: ETF2-enhancement facility */ @@ -201,7 +201,7 @@ static const uint64_t facilities_dw[] = { (0ULL << 31) | /* b32: Compare-and-swap-and-store facility */ (0ULL << 30) | /* b33: Compare-and-swap-and-store facility 2 */ (0ULL << 29) | /* b34: General-instructions-extension facility */ - (0ULL << 28) | /* b35: Execute-extensions facility */ + (1ULL << 28) | /* b35: Execute-extensions facility */ (0ULL << 27) | /* b36: Enhanced-monitor facility */ (0ULL << 26) | /* b37: Floating-point extension facility */ (0ULL << 24) | /* b39: IBM internal use */
Cc: Alexander Graf <agraf@suse.de> Cc: Richard Henderson <rth@twiddle.net> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> --- target-s390x/cpu.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)