| Message ID | 20171004154144.88995-3-pasic@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04.10.2017 17:41, Halil Pasic wrote: > CSS code needs to tell the IO instruction handlers located in how should located in how? > the emulated instruction be ended. Currently this is done by returning > generic (POSIX) error codes, and mapping them to outcomes like condition > codes. This makes bugs easy to create and hard to recognise. > > As a preparation for moving a way form (mis)using generic error codes for > flow control let us introduce a struct which tells the instruction > handler function how to end the instruction, in a more straight-forward > and less ambiguous way. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > include/hw/s390x/css.h | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h > index 0653d3c9be..66916b6546 100644 > --- a/include/hw/s390x/css.h > +++ b/include/hw/s390x/css.h > @@ -75,6 +75,18 @@ typedef struct CMBE { > uint32_t reserved[7]; > } QEMU_PACKED CMBE; > > +/* IO instructions conclude according this */ > +typedef struct IOInstEnding { > + /* > + * General semantic of cc codes of IO instructions is (brief): > + * 0 -- produced expected result > + * 1 -- status conditions were present or produced alternate result > + * 2 -- ineffective, because busy with previously initiated function > + * 3 -- ineffective, not operational > + */ > + int cc; > +} IOInstEnding; Why do you need a struct for this? Do you plan to extend it later? If so, I think you should mention that in the patch description. If not, please use a named enum or a "typedef unsigned int IOInstEnding" instead. Thomas
On 10/09/2017 10:20 AM, Thomas Huth wrote: > On 04.10.2017 17:41, Halil Pasic wrote: >> CSS code needs to tell the IO instruction handlers located in how should > > located in how? > First, thanks for your review! Wanted to say: in target/s390x/ioinst.c just forgot to copy paste. >> the emulated instruction be ended. Currently this is done by returning >> generic (POSIX) error codes, and mapping them to outcomes like condition >> codes. This makes bugs easy to create and hard to recognise. >> >> As a preparation for moving a way form (mis)using generic error codes for >> flow control let us introduce a struct which tells the instruction >> handler function how to end the instruction, in a more straight-forward >> and less ambiguous way. >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> --- >> include/hw/s390x/css.h | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h >> index 0653d3c9be..66916b6546 100644 >> --- a/include/hw/s390x/css.h >> +++ b/include/hw/s390x/css.h >> @@ -75,6 +75,18 @@ typedef struct CMBE { >> uint32_t reserved[7]; >> } QEMU_PACKED CMBE; >> >> +/* IO instructions conclude according this */ >> +typedef struct IOInstEnding { >> + /* >> + * General semantic of cc codes of IO instructions is (brief): >> + * 0 -- produced expected result >> + * 1 -- status conditions were present or produced alternate result >> + * 2 -- ineffective, because busy with previously initiated function >> + * 3 -- ineffective, not operational >> + */ >> + int cc; >> +} IOInstEnding; > > Why do you need a struct for this? Do you plan to extend it later? If > so, I think you should mention that in the patch description. If not, > please use a named enum or a "typedef unsigned int IOInstEnding" instead. > > Thomas We may, we may not. In the previous version we also had to support do end a certain instruction with an addressing exception, but this is going away in patch #3. Honestly I don't expect this being extended. I have other reasons for the struct. Type safety and clear semantics, and frankly at least for s390 and linux I don't see any downsides given what is written in the "zSeries ELF Application Binary Interface Supplement". Can you please explain to me what is the problem with using this struct, and what is the benefit switching to a unsigned int? Regards, Halil >
On 09.10.2017 12:54, Halil Pasic wrote: > > > On 10/09/2017 10:20 AM, Thomas Huth wrote: >> On 04.10.2017 17:41, Halil Pasic wrote: >>> CSS code needs to tell the IO instruction handlers located in how should >> >> located in how? >> > > First, thanks for your review! > > Wanted to say: in target/s390x/ioinst.c just forgot to copy paste. > >>> the emulated instruction be ended. Currently this is done by returning >>> generic (POSIX) error codes, and mapping them to outcomes like condition >>> codes. This makes bugs easy to create and hard to recognise. >>> >>> As a preparation for moving a way form (mis)using generic error codes for >>> flow control let us introduce a struct which tells the instruction >>> handler function how to end the instruction, in a more straight-forward >>> and less ambiguous way. >>> >>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>> --- >>> include/hw/s390x/css.h | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h >>> index 0653d3c9be..66916b6546 100644 >>> --- a/include/hw/s390x/css.h >>> +++ b/include/hw/s390x/css.h >>> @@ -75,6 +75,18 @@ typedef struct CMBE { >>> uint32_t reserved[7]; >>> } QEMU_PACKED CMBE; >>> >>> +/* IO instructions conclude according this */ >>> +typedef struct IOInstEnding { >>> + /* >>> + * General semantic of cc codes of IO instructions is (brief): >>> + * 0 -- produced expected result >>> + * 1 -- status conditions were present or produced alternate result >>> + * 2 -- ineffective, because busy with previously initiated function >>> + * 3 -- ineffective, not operational >>> + */ >>> + int cc; >>> +} IOInstEnding; >> >> Why do you need a struct for this? Do you plan to extend it later? If >> so, I think you should mention that in the patch description. If not, >> please use a named enum or a "typedef unsigned int IOInstEnding" instead. >> >> Thomas > > We may, we may not. In the previous version we also had to support > do end a certain instruction with an addressing exception, but this > is going away in patch #3. Honestly I don't expect this being extended. > > I have other reasons for the struct. Type safety and clear semantics, > and frankly at least for s390 and linux I don't see any downsides given > what is written in the "zSeries ELF Application Binary Interface Supplement". > Can you please explain to me what is the problem with using this struct, and > what is the benefit switching to a unsigned int? First, returning a struct is ugly in most cases, since it might need to be passed on the stack if it is bigger than 8 bytes. Ok, that's likely not the case here (if the compiler / ABI is smart enough - I did not check), but still, if I see something like this, there is an alarm signal somewhere in my head that starts to ring... Then, in the follow up patches, you do something like this: return (IOInstEnding){.cc = 0}; ... and that just looks very, very ugly in my eyes. The more I look at it, the more I think we really want to have a named enum instead. That will give you some sort of basic type safety and semantics, too, and we'll also get proper names for those magic values - otherwise I'll always have to look up what cc = 2 or cc = 3 means... (I always keep forgetting what each value means...) Thomas
On Mon, 9 Oct 2017 12:54:03 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > On 10/09/2017 10:20 AM, Thomas Huth wrote: > > On 04.10.2017 17:41, Halil Pasic wrote: > >> +/* IO instructions conclude according this */ > >> +typedef struct IOInstEnding { > >> + /* > >> + * General semantic of cc codes of IO instructions is (brief): > >> + * 0 -- produced expected result > >> + * 1 -- status conditions were present or produced alternate result > >> + * 2 -- ineffective, because busy with previously initiated function > >> + * 3 -- ineffective, not operational > >> + */ > >> + int cc; > >> +} IOInstEnding; > > > > Why do you need a struct for this? Do you plan to extend it later? If > > so, I think you should mention that in the patch description. If not, > > please use a named enum or a "typedef unsigned int IOInstEnding" instead. > > > > Thomas > > We may, we may not. In the previous version we also had to support > do end a certain instruction with an addressing exception, but this > is going away in patch #3. Honestly I don't expect this being extended. > > I have other reasons for the struct. Type safety and clear semantics, > and frankly at least for s390 and linux I don't see any downsides given > what is written in the "zSeries ELF Application Binary Interface Supplement". > Can you please explain to me what is the problem with using this struct, and > what is the benefit switching to a unsigned int? Honestly, I fail to see the benefit of using a struct here... it's just a condition code, and while adding a comment what the various codes mean for I/O instructions is a good idea, I think having to use a IOInstEnding struct just renders the code less readable. [I haven't had time to look at the rest of the patches yet.]
On 10/09/2017 01:07 PM, Thomas Huth wrote: > On 09.10.2017 12:54, Halil Pasic wrote: >> >> >> On 10/09/2017 10:20 AM, Thomas Huth wrote: >>> On 04.10.2017 17:41, Halil Pasic wrote: >>>> CSS code needs to tell the IO instruction handlers located in how should >>> >>> located in how? >>> >> >> First, thanks for your review! >> >> Wanted to say: in target/s390x/ioinst.c just forgot to copy paste. >> >>>> the emulated instruction be ended. Currently this is done by returning >>>> generic (POSIX) error codes, and mapping them to outcomes like condition >>>> codes. This makes bugs easy to create and hard to recognise. >>>> >>>> As a preparation for moving a way form (mis)using generic error codes for >>>> flow control let us introduce a struct which tells the instruction >>>> handler function how to end the instruction, in a more straight-forward >>>> and less ambiguous way. >>>> >>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>>> --- >>>> include/hw/s390x/css.h | 12 ++++++++++++ >>>> 1 file changed, 12 insertions(+) >>>> >>>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h >>>> index 0653d3c9be..66916b6546 100644 >>>> --- a/include/hw/s390x/css.h >>>> +++ b/include/hw/s390x/css.h >>>> @@ -75,6 +75,18 @@ typedef struct CMBE { >>>> uint32_t reserved[7]; >>>> } QEMU_PACKED CMBE; >>>> >>>> +/* IO instructions conclude according this */ >>>> +typedef struct IOInstEnding { >>>> + /* >>>> + * General semantic of cc codes of IO instructions is (brief): >>>> + * 0 -- produced expected result >>>> + * 1 -- status conditions were present or produced alternate result >>>> + * 2 -- ineffective, because busy with previously initiated function >>>> + * 3 -- ineffective, not operational >>>> + */ >>>> + int cc; >>>> +} IOInstEnding; >>> >>> Why do you need a struct for this? Do you plan to extend it later? If >>> so, I think you should mention that in the patch description. If not, >>> please use a named enum or a "typedef unsigned int IOInstEnding" instead. >>> >>> Thomas >> >> We may, we may not. In the previous version we also had to support >> do end a certain instruction with an addressing exception, but this >> is going away in patch #3. Honestly I don't expect this being extended. >> >> I have other reasons for the struct. Type safety and clear semantics, >> and frankly at least for s390 and linux I don't see any downsides given >> what is written in the "zSeries ELF Application Binary Interface Supplement". >> Can you please explain to me what is the problem with using this struct, and >> what is the benefit switching to a unsigned int? > > First, returning a struct is ugly in most cases, since it might need to > be passed on the stack if it is bigger than 8 bytes. Ok, that's likely > not the case here (if the compiler / ABI is smart enough - I did not > check), but still, if I see something like this, there is an alarm > signal somewhere in my head that starts to ring... > Yeah, the ABI is smart enough (where it matters) and this one is obviously less that 8 bytes. I read this as you assumed that the return won't be passed via register (general purpose register 2 for a z host + ELF assumed), and that would have been ugly indeed. Btw I have seen putting an integral into a struct for type checking in the linux kernel too. For instance from: arch/s390/include/asm/page.h """ /* * These are used to make use of C type-checking.. */ typedef struct { unsigned long pgprot; } pgprot_t; typedef struct { unsigned long pgste; } pgste_t; typedef struct { unsigned long pte; } pte_t; typedef struct { unsigned long pmd; } pmd_t; typedef struct { unsigned long pud; } pud_t; typedef struct { unsigned long pgd; } pgd_t; typedef pte_t *pgtable_t; #define pgprot_val(x) ((x).pgprot) #define pgste_val(x) ((x).pgste) #define pte_val(x) ((x).pte) #define pmd_val(x) ((x).pmd) #define pud_val(x) ((x).pud) #define pgd_val(x) ((x).pgd) #define __pgste(x) ((pgste_t) { (x) } ) #define __pte(x) ((pte_t) { (x) } ) #define __pmd(x) ((pmd_t) { (x) } ) #define __pud(x) ((pud_t) { (x) } ) #define __pgd(x) ((pgd_t) { (x) } ) #define __pgprot(x) ((pgprot_t) { (x) } ) """ If you think, I could add a similar comment indicating that my struct is also just for type-checking. > Then, in the follow up patches, you do something like this: > > return (IOInstEnding){.cc = 0}; > > ... and that just looks very, very ugly in my eyes. The more I look at Interesting, I found this quite expressive. > it, the more I think we really want to have a named enum instead. That > will give you some sort of basic type safety and semantics, too, and AFAIK we don't have strongly typed enums in C, at least not from the language perspective. In c++ we got enum class and enum struct with c++11 for that reason. > we'll also get proper names for those magic values - otherwise I'll > always have to look up what cc = 2 or cc = 3 means... (I always keep > forgetting what each value means...) Can you suggest some proper names for those magic values? Unfortunately the PoP refers to this stuff as setting condition code [0-3], so in my reading the numbers are the canonical names for this stuff. The best 'proper names' I can think of for these are CC_0, CC_1, CC_2, and CC_3. IMHO better names and type safety are two different problems, so if you do come up with better names, we can use that regardless of the type safety solution. I'm also not sure when do we tend to use defines and when enums. This is clearly a the value associated with the tag matters mightily situation so I would at least want to assign explicit values to the individual enumerator constants. Sorry, I may be a bit to persistent on this one: I don't think it's a huge difference, but I don't feel great about changing something to what I think is (slightly) worse without being first convinced that I was wrong. Regards, Halil > > Thomas >
On 10/09/2017 01:09 PM, Cornelia Huck wrote: > On Mon, 9 Oct 2017 12:54:03 +0200 > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >> On 10/09/2017 10:20 AM, Thomas Huth wrote: >>> On 04.10.2017 17:41, Halil Pasic wrote: > >>>> +/* IO instructions conclude according this */ >>>> +typedef struct IOInstEnding { >>>> + /* >>>> + * General semantic of cc codes of IO instructions is (brief): >>>> + * 0 -- produced expected result >>>> + * 1 -- status conditions were present or produced alternate result >>>> + * 2 -- ineffective, because busy with previously initiated function >>>> + * 3 -- ineffective, not operational >>>> + */ >>>> + int cc; >>>> +} IOInstEnding; >>> >>> Why do you need a struct for this? Do you plan to extend it later? If >>> so, I think you should mention that in the patch description. If not, >>> please use a named enum or a "typedef unsigned int IOInstEnding" instead. >>> >>> Thomas >> >> We may, we may not. In the previous version we also had to support >> do end a certain instruction with an addressing exception, but this >> is going away in patch #3. Honestly I don't expect this being extended. >> >> I have other reasons for the struct. Type safety and clear semantics, >> and frankly at least for s390 and linux I don't see any downsides given >> what is written in the "zSeries ELF Application Binary Interface Supplement". >> Can you please explain to me what is the problem with using this struct, and >> what is the benefit switching to a unsigned int? > > Honestly, I fail to see the benefit of using a struct here... it's just Type safety. For instance had I forgotten let's say a return -ENODEV somewhere, my version would not compile. On the contrary with an enum (like Thomas has proposed) it compiles just fine with my setup -- I did not try what would happen if we call setcc(cpu, -ENODEV) > a condition code, and while adding a comment what the various codes Right, but it's a different _type_ of condition code (than the POSIX errno codes for instance) so that's why it's modeled as a different type. > mean for I/O instructions is a good idea, I think having to use a > IOInstEnding struct just renders the code less readable. It's probably a matter of taste. I find, for example return (IOInstEnding){.cc = 1} in this case more readable than return -ENOSYS because from the first one I know we have detected a condition which we want to handle by setting cc 1 for the instruction. The later requires me figuring out in the context of which instruction handler am I called, go trough the whole call chain looking for possible re-mappings (like for -EACCES) and finally examining the switch statement of the instruction handler carefully. Could you please tell me what exactly is difficult to read for you (form what I've proposed). Regards, Halil > > [I haven't had time to look at the rest of the patches yet.] >
On 09.10.2017 17:00, Halil Pasic wrote: > > > On 10/09/2017 01:07 PM, Thomas Huth wrote: >> On 09.10.2017 12:54, Halil Pasic wrote: >>> >>> >>> On 10/09/2017 10:20 AM, Thomas Huth wrote: >>>> On 04.10.2017 17:41, Halil Pasic wrote: >>>>> CSS code needs to tell the IO instruction handlers located in how should >>>> >>>> located in how? >>>> >>> >>> First, thanks for your review! >>> >>> Wanted to say: in target/s390x/ioinst.c just forgot to copy paste. >>> >>>>> the emulated instruction be ended. Currently this is done by returning >>>>> generic (POSIX) error codes, and mapping them to outcomes like condition >>>>> codes. This makes bugs easy to create and hard to recognise. >>>>> >>>>> As a preparation for moving a way form (mis)using generic error codes for >>>>> flow control let us introduce a struct which tells the instruction >>>>> handler function how to end the instruction, in a more straight-forward >>>>> and less ambiguous way. >>>>> >>>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>>>> --- >>>>> include/hw/s390x/css.h | 12 ++++++++++++ >>>>> 1 file changed, 12 insertions(+) >>>>> >>>>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h >>>>> index 0653d3c9be..66916b6546 100644 >>>>> --- a/include/hw/s390x/css.h >>>>> +++ b/include/hw/s390x/css.h >>>>> @@ -75,6 +75,18 @@ typedef struct CMBE { >>>>> uint32_t reserved[7]; >>>>> } QEMU_PACKED CMBE; >>>>> >>>>> +/* IO instructions conclude according this */ >>>>> +typedef struct IOInstEnding { >>>>> + /* >>>>> + * General semantic of cc codes of IO instructions is (brief): >>>>> + * 0 -- produced expected result >>>>> + * 1 -- status conditions were present or produced alternate result >>>>> + * 2 -- ineffective, because busy with previously initiated function >>>>> + * 3 -- ineffective, not operational >>>>> + */ >>>>> + int cc; >>>>> +} IOInstEnding; >>>> >>>> Why do you need a struct for this? Do you plan to extend it later? If >>>> so, I think you should mention that in the patch description. If not, >>>> please use a named enum or a "typedef unsigned int IOInstEnding" instead. >>>> >>>> Thomas >>> >>> We may, we may not. In the previous version we also had to support >>> do end a certain instruction with an addressing exception, but this >>> is going away in patch #3. Honestly I don't expect this being extended. >>> >>> I have other reasons for the struct. Type safety and clear semantics, >>> and frankly at least for s390 and linux I don't see any downsides given >>> what is written in the "zSeries ELF Application Binary Interface Supplement". >>> Can you please explain to me what is the problem with using this struct, and >>> what is the benefit switching to a unsigned int? >> >> First, returning a struct is ugly in most cases, since it might need to >> be passed on the stack if it is bigger than 8 bytes. Ok, that's likely >> not the case here (if the compiler / ABI is smart enough - I did not >> check), but still, if I see something like this, there is an alarm >> signal somewhere in my head that starts to ring... >> > > Yeah, the ABI is smart enough (where it matters) and this one is obviously > less that 8 bytes. I read this as you assumed that the return won't be > passed via register (general purpose register 2 for a z host + ELF assumed), > and that would have been ugly indeed. > > Btw I have seen putting an integral into a struct for type checking > in the linux kernel too. For instance from: > arch/s390/include/asm/page.h > > """ > /* > * These are used to make use of C type-checking.. > */ > > typedef struct { unsigned long pgprot; } pgprot_t; > typedef struct { unsigned long pgste; } pgste_t; > typedef struct { unsigned long pte; } pte_t; > typedef struct { unsigned long pmd; } pmd_t; > typedef struct { unsigned long pud; } pud_t; > typedef struct { unsigned long pgd; } pgd_t; > typedef pte_t *pgtable_t; > > #define pgprot_val(x) ((x).pgprot) > #define pgste_val(x) ((x).pgste) > #define pte_val(x) ((x).pte) > #define pmd_val(x) ((x).pmd) > #define pud_val(x) ((x).pud) > #define pgd_val(x) ((x).pgd) > > #define __pgste(x) ((pgste_t) { (x) } ) > #define __pte(x) ((pte_t) { (x) } ) > #define __pmd(x) ((pmd_t) { (x) } ) > #define __pud(x) ((pud_t) { (x) } ) > #define __pgd(x) ((pgd_t) { (x) } ) > #define __pgprot(x) ((pgprot_t) { (x) } ) > """ > > If you think, I could add a similar comment indicating that my > struct is also just for type-checking. No, I think you've got the reason here slightly wrong. The kernel only uses this since the pte_t and friends need to be urgently portable between the different architectures. Have a look at the kernel Documentation/CodingStyle file, it explicitly mentions this in chapter 5 ("Typedefs"). >> Then, in the follow up patches, you do something like this: >> >> return (IOInstEnding){.cc = 0}; >> >> ... and that just looks very, very ugly in my eyes. The more I look at > > Interesting, I found this quite expressive. C'mon, we're writing C code, not Java ;-) >> it, the more I think we really want to have a named enum instead. That >> will give you some sort of basic type safety and semantics, too, and > > AFAIK we don't have strongly typed enums in C, at least not from > the language perspective. In c++ we got enum class and enum struct > with c++11 for that reason. > >> we'll also get proper names for those magic values - otherwise I'll >> always have to look up what cc = 2 or cc = 3 means... (I always keep >> forgetting what each value means...) > > Can you suggest some proper names for those magic values? Unfortunately > the PoP refers to this stuff as setting condition code [0-3], so in my > reading the numbers are the canonical names for this stuff. The best > 'proper names' I can think of for these are CC_0, CC_1, CC_2, and CC_3. Well, you already gave a description in your comment in the struct IOInstEnding, so maybe something similar? Or maybe this could even be merged with the definitions for the SIGP status codes: #define SIGP_CC_ORDER_CODE_ACCEPTED 0 #define SIGP_CC_STATUS_STORED 1 #define SIGP_CC_BUSY 2 #define SIGP_CC_NOT_OPERATIONAL 3 ? > Sorry, I may be a bit to persistent on this one: I don't think it's > a huge difference, but I don't feel great about changing something to > what I think is (slightly) worse without being first convinced that > I was wrong. In the end, the code has to be accepted by the maintainers, so let's leave the decision up to them whether they like this typedef struct IOInstEnding or not... Thomas
On Tue, 10 Oct 2017 12:28:35 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 09.10.2017 17:00, Halil Pasic wrote: > > > > > > On 10/09/2017 01:07 PM, Thomas Huth wrote: > >> Then, in the follow up patches, you do something like this: > >> > >> return (IOInstEnding){.cc = 0}; > >> > >> ... and that just looks very, very ugly in my eyes. The more I look at > > > > Interesting, I found this quite expressive. > > C'mon, we're writing C code, not Java ;-) Every time I read that construct, I die a little bit inside... > Well, you already gave a description in your comment in the struct > IOInstEnding, so maybe something similar? Or maybe this could even be > merged with the definitions for the SIGP status codes: > > #define SIGP_CC_ORDER_CODE_ACCEPTED 0 > #define SIGP_CC_STATUS_STORED 1 > #define SIGP_CC_BUSY 2 > #define SIGP_CC_NOT_OPERATIONAL 3 I'd rather not reuse the definitions for a different instruction, even if they are similar in semantics. > > Sorry, I may be a bit to persistent on this one: I don't think it's > > a huge difference, but I don't feel great about changing something to > > what I think is (slightly) worse without being first convinced that > > I was wrong. > > In the end, the code has to be accepted by the maintainers, so let's > leave the decision up to them whether they like this typedef struct > IOInstEnding or not... Here's a strong 'do not like' from me... using an enum or define is fine with me.
[..] >> >> Yeah, the ABI is smart enough (where it matters) and this one is obviously >> less that 8 bytes. I read this as you assumed that the return won't be >> passed via register (general purpose register 2 for a z host + ELF assumed), >> and that would have been ugly indeed. >> >> Btw I have seen putting an integral into a struct for type checking >> in the linux kernel too. For instance from: >> arch/s390/include/asm/page.h >> >> """ >> /* >> * These are used to make use of C type-checking.. >> */ >> >> typedef struct { unsigned long pgprot; } pgprot_t; >> typedef struct { unsigned long pgste; } pgste_t; >> typedef struct { unsigned long pte; } pte_t; >> typedef struct { unsigned long pmd; } pmd_t; >> typedef struct { unsigned long pud; } pud_t; >> typedef struct { unsigned long pgd; } pgd_t; >> typedef pte_t *pgtable_t; >> >> #define pgprot_val(x) ((x).pgprot) >> #define pgste_val(x) ((x).pgste) >> #define pte_val(x) ((x).pte) >> #define pmd_val(x) ((x).pmd) >> #define pud_val(x) ((x).pud) >> #define pgd_val(x) ((x).pgd) >> >> #define __pgste(x) ((pgste_t) { (x) } ) >> #define __pte(x) ((pte_t) { (x) } ) >> #define __pmd(x) ((pmd_t) { (x) } ) >> #define __pud(x) ((pud_t) { (x) } ) >> #define __pgd(x) ((pgd_t) { (x) } ) >> #define __pgprot(x) ((pgprot_t) { (x) } ) >> """ >> >> If you think, I could add a similar comment indicating that my >> struct is also just for type-checking. > > No, I think you've got the reason here slightly wrong. The kernel only > uses this since the pte_t and friends need to be urgently portable > between the different architectures. Have a look at the kernel > Documentation/CodingStyle file, it explicitly mentions this in chapter 5 > ("Typedefs"). > Disclaimer: I agree with your conclusion, the maintainers will have to make the final call here. I will still reply point by point for the fun of pointless academic discussions. IMHO we don't talk about typedefs here but about type checking. Btw QEMU has the exact opposite policy regarding typedefs and structs than Linux. You want to say that the comment at the top of my quote is wrong, and that it should be rather "These are used for hiding representation.." that "These are used to make use of C type-checking.."? I'm also curious which of the rules would your original suggestion of doing "typedef unsigned int IOInstEnding" match (from chapter 5 "Typedefs")? ;) >>> Then, in the follow up patches, you do something like this: >>> >>> return (IOInstEnding){.cc = 0}; >>> >>> ... and that just looks very, very ugly in my eyes. The more I look at >> >> Interesting, I found this quite expressive. > > C'mon, we're writing C code, not Java ;-) Yeah, me written much C++ and also Java before might be a part of the problem. Btw there is no such construct in Java AFAIR :P. > >>> it, the more I think we really want to have a named enum instead. That >>> will give you some sort of basic type safety and semantics, too, and >> >> AFAIK we don't have strongly typed enums in C, at least not from >> the language perspective. In c++ we got enum class and enum struct >> with c++11 for that reason. >> >>> we'll also get proper names for those magic values - otherwise I'll >>> always have to look up what cc = 2 or cc = 3 means... (I always keep >>> forgetting what each value means...) >> >> Can you suggest some proper names for those magic values? Unfortunately >> the PoP refers to this stuff as setting condition code [0-3], so in my >> reading the numbers are the canonical names for this stuff. The best >> 'proper names' I can think of for these are CC_0, CC_1, CC_2, and CC_3. > > Well, you already gave a description in your comment in the struct > IOInstEnding, so maybe something similar? Or maybe this could even be > merged with the definitions for the SIGP status codes: > > #define SIGP_CC_ORDER_CODE_ACCEPTED 0 > #define SIGP_CC_STATUS_STORED 1 > #define SIGP_CC_BUSY 2 > #define SIGP_CC_NOT_OPERATIONAL 3 > > ? > Maybe. >> Sorry, I may be a bit to persistent on this one: I don't think it's >> a huge difference, but I don't feel great about changing something to >> what I think is (slightly) worse without being first convinced that >> I was wrong. > > In the end, the code has to be accepted by the maintainers, so let's > leave the decision up to them whether they like this typedef struct > IOInstEnding or not... I agree. Especially if the change in behavior in #3 does not get trough I will have to revert indicating exceptions too (like in v1), and then I really need the struct (which can be still less than 8 bytes). I assume you have seen my reply to Connie about the benefit: among others it prevents using something like -EFAULT as a cc by accident which neither an enum nor a typedef does. In practice, I think either of the proposed solutions will do. Halil > > Thomas >
On 10/10/2017 01:39 PM, Cornelia Huck wrote: > On Tue, 10 Oct 2017 12:28:35 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> On 09.10.2017 17:00, Halil Pasic wrote: >>> >>> >>> On 10/09/2017 01:07 PM, Thomas Huth wrote: > >>>> Then, in the follow up patches, you do something like this: >>>> >>>> return (IOInstEnding){.cc = 0}; >>>> >>>> ... and that just looks very, very ugly in my eyes. The more I look at >>> >>> Interesting, I found this quite expressive. >> >> C'mon, we're writing C code, not Java ;-) > > Every time I read that construct, I die a little bit inside... > >> Well, you already gave a description in your comment in the struct >> IOInstEnding, so maybe something similar? Or maybe this could even be >> merged with the definitions for the SIGP status codes: >> >> #define SIGP_CC_ORDER_CODE_ACCEPTED 0 >> #define SIGP_CC_STATUS_STORED 1 >> #define SIGP_CC_BUSY 2 >> #define SIGP_CC_NOT_OPERATIONAL 3 > > I'd rather not reuse the definitions for a different instruction, even > if they are similar in semantics. > >>> Sorry, I may be a bit to persistent on this one: I don't think it's >>> a huge difference, but I don't feel great about changing something to >>> what I think is (slightly) worse without being first convinced that >>> I was wrong. >> >> In the end, the code has to be accepted by the maintainers, so let's >> leave the decision up to them whether they like this typedef struct >> IOInstEnding or not... > > Here's a strong 'do not like' from me... using an enum or define is > fine with me. > Got the message. Could we first reach an agreement on the rest of the series? As I've said, I might need to go back to indicating exceptions too (depending on how do we like #3), and that would mean a changed situation. If the price for getting this in is sacrificing my strongly type checked condition code type I can live with that. Halil t
On 10.10.2017 13:41, Halil Pasic wrote: > [..] >>> >>> Yeah, the ABI is smart enough (where it matters) and this one is obviously >>> less that 8 bytes. I read this as you assumed that the return won't be >>> passed via register (general purpose register 2 for a z host + ELF assumed), >>> and that would have been ugly indeed. >>> >>> Btw I have seen putting an integral into a struct for type checking >>> in the linux kernel too. For instance from: >>> arch/s390/include/asm/page.h >>> >>> """ >>> /* >>> * These are used to make use of C type-checking.. >>> */ >>> >>> typedef struct { unsigned long pgprot; } pgprot_t; >>> typedef struct { unsigned long pgste; } pgste_t; >>> typedef struct { unsigned long pte; } pte_t; >>> typedef struct { unsigned long pmd; } pmd_t; >>> typedef struct { unsigned long pud; } pud_t; >>> typedef struct { unsigned long pgd; } pgd_t; >>> typedef pte_t *pgtable_t; >>> >>> #define pgprot_val(x) ((x).pgprot) >>> #define pgste_val(x) ((x).pgste) >>> #define pte_val(x) ((x).pte) >>> #define pmd_val(x) ((x).pmd) >>> #define pud_val(x) ((x).pud) >>> #define pgd_val(x) ((x).pgd) >>> >>> #define __pgste(x) ((pgste_t) { (x) } ) >>> #define __pte(x) ((pte_t) { (x) } ) >>> #define __pmd(x) ((pmd_t) { (x) } ) >>> #define __pud(x) ((pud_t) { (x) } ) >>> #define __pgd(x) ((pgd_t) { (x) } ) >>> #define __pgprot(x) ((pgprot_t) { (x) } ) >>> """ >>> >>> If you think, I could add a similar comment indicating that my >>> struct is also just for type-checking. >> >> No, I think you've got the reason here slightly wrong. The kernel only >> uses this since the pte_t and friends need to be urgently portable >> between the different architectures. Have a look at the kernel >> Documentation/CodingStyle file, it explicitly mentions this in chapter 5 >> ("Typedefs"). > > IMHO we don't talk about typedefs here but about type checking. Btw QEMU > has the exact opposite policy regarding typedefs and structs than Linux. > You want to say that the comment at the top of my quote is wrong, and > that it should be rather "These are used for hiding representation.." > that "These are used to make use of C type-checking.."? I certainly did not want to say that you should change the comment. I just wanted to point you to the fact that these typedefs in the kernel are likely rather used for hiding representation indeed, and not so much for type checking, so using them as an example for introducing something like type checking here in QEMU is quite a bad example. > I'm also curious which of the rules would your original suggestion of > doing "typedef unsigned int IOInstEnding" match (from chapter 5 > "Typedefs")? ;) None, of course. That's the difference between the kernel and QEMU - though I have to say that I rather prefer the kernel philosophy nowadays. So yes, please don't do a "typedef unsigned int IOInstEnding" either. I think the best match for QEMU would be a typedef enum IOInstEnding { CC_..., CC_..., CC_..., CC_... } IOInstEnding; You then also should at least get some compiler checking thanks to -Wswitch and -Wenum-compare, shouldn't you? > I assume you have seen my reply to Connie about the benefit: among others > it prevents using something like -EFAULT as a cc by accident which neither > an enum nor a typedef does. A confused developer could still do something like "return (IOInstEnding){.cc = -EFAULT};", so this is also not completely safe. Thomas
On 10/12/2017 08:58 AM, Thomas Huth wrote: > On 10.10.2017 13:41, Halil Pasic wrote: >> [..] >>>> >>>> Yeah, the ABI is smart enough (where it matters) and this one is obviously >>>> less that 8 bytes. I read this as you assumed that the return won't be >>>> passed via register (general purpose register 2 for a z host + ELF assumed), >>>> and that would have been ugly indeed. >>>> >>>> Btw I have seen putting an integral into a struct for type checking >>>> in the linux kernel too. For instance from: >>>> arch/s390/include/asm/page.h >>>> >>>> """ >>>> /* >>>> * These are used to make use of C type-checking.. >>>> */ >>>> >>>> typedef struct { unsigned long pgprot; } pgprot_t; >>>> typedef struct { unsigned long pgste; } pgste_t; >>>> typedef struct { unsigned long pte; } pte_t; >>>> typedef struct { unsigned long pmd; } pmd_t; >>>> typedef struct { unsigned long pud; } pud_t; >>>> typedef struct { unsigned long pgd; } pgd_t; >>>> typedef pte_t *pgtable_t; >>>> >>>> #define pgprot_val(x) ((x).pgprot) >>>> #define pgste_val(x) ((x).pgste) >>>> #define pte_val(x) ((x).pte) >>>> #define pmd_val(x) ((x).pmd) >>>> #define pud_val(x) ((x).pud) >>>> #define pgd_val(x) ((x).pgd) >>>> >>>> #define __pgste(x) ((pgste_t) { (x) } ) >>>> #define __pte(x) ((pte_t) { (x) } ) >>>> #define __pmd(x) ((pmd_t) { (x) } ) >>>> #define __pud(x) ((pud_t) { (x) } ) >>>> #define __pgd(x) ((pgd_t) { (x) } ) >>>> #define __pgprot(x) ((pgprot_t) { (x) } ) >>>> """ >>>> >>>> If you think, I could add a similar comment indicating that my >>>> struct is also just for type-checking. >>> >>> No, I think you've got the reason here slightly wrong. The kernel only >>> uses this since the pte_t and friends need to be urgently portable >>> between the different architectures. Have a look at the kernel >>> Documentation/CodingStyle file, it explicitly mentions this in chapter 5 >>> ("Typedefs"). >> >> IMHO we don't talk about typedefs here but about type checking. Btw QEMU >> has the exact opposite policy regarding typedefs and structs than Linux. >> You want to say that the comment at the top of my quote is wrong, and >> that it should be rather "These are used for hiding representation.." >> that "These are used to make use of C type-checking.."? > > I certainly did not want to say that you should change the comment. I > just wanted to point you to the fact that these typedefs in the kernel > are likely rather used for hiding representation indeed, and not so much > for type checking, so using them as an example for introducing something > like type checking here in QEMU is quite a bad example. > I've noted. I won't look for another example. I don't understand your logic, but I'm afraid I've already taken too much of your precious time. >> I'm also curious which of the rules would your original suggestion of >> doing "typedef unsigned int IOInstEnding" match (from chapter 5 >> "Typedefs")? ;) > > None, of course. That's the difference between the kernel and QEMU - > though I have to say that I rather prefer the kernel philosophy nowadays. > > So yes, please don't do a "typedef unsigned int IOInstEnding" either. I > think the best match for QEMU would be a > > typedef enum IOInstEnding { > CC_..., > CC_..., > CC_..., > CC_... > } IOInstEnding; > I also prefer this over #define NAME val. > You then also should at least get some compiler checking thanks to > -Wswitch and -Wenum-compare, shouldn't you? None that matter. The switches are going away. And I also don't expect compares. The for me interesting wrong propagation scenario (e.g. return func_ret_errno()) isn't covered. It is not a big thing though. I've compiled with: ~/git/qemu/configure --target-list=s390x-softmmu --extra-cflags="-Wswitch -Wenum-compare" and gcc version 4.8.5 > >> I assume you have seen my reply to Connie about the benefit: among others >> it prevents using something like -EFAULT as a cc by accident which neither >> an enum nor a typedef does. > > A confused developer could still do something like "return > (IOInstEnding){.cc = -EFAULT};", so this is also not completely safe. Of course,but that would be a very confused programmer, who does not bother what his newly written code does, and does not care to look at the definition of IOInstEnding which states the valid values of cc. Since we don't write Ada I was under the impression that just stating the valid values would be enough. #define IOINST_CC(v) (assert(!((v) & ~0x03ULL)), (IOInstEnding){.cc = (v)}) or even better #define IOINST_CC(cv) ({QEMU_BUILD_BUG_ON((cv) & ~0x03ULL);\ (IOInstEnding){.cc = (cv)};}) and then return IOINST_CC(-EFAULT); would fail with an runtime assert or at compile time respectively while good code would look like return IOINST_CC(1); Of course that would still eave the possibility of doing "IOInstEnding){.cc = -EFAULT}" directly. Now this is where my C skill ends (I don't know how to prohibit that, because we need the type public). In the end I found that such a mistake is unlikely enough, and I'm still keeping my opinion. I think I've understood your opinion: type checking is an overkill in this particular use-case. It's a legit opinion -- one I don't share, but one I can't argue with. Again, sorry for taking so much of your time. I'm bad at letting loose. Regards, Halil > > Thomas >
On 10/12/2017 01:44 PM, Halil Pasic wrote: > > > On 10/12/2017 08:58 AM, Thomas Huth wrote: >> On 10.10.2017 13:41, Halil Pasic wrote: [..] >> So yes, please don't do a "typedef unsigned int IOInstEnding" either. I >> think the best match for QEMU would be a >> >> typedef enum IOInstEnding { >> CC_..., >> CC_..., >> CC_..., >> CC_... >> } IOInstEnding; >> > > I also prefer this over #define NAME val. > @Conny @Thomas I'm almost done with v3, but I've realized we did not agree on the names for the enum constants, so before posting something to ugly again, I would like to inquire your opinion. My current version of the enum is the following (but I can easily change to whatever you like with sed): +/* + * IO instructions conclude according this. Currently we have only + * cc codes. Valid values are 0,1,2,3 and the generic semantic for IO instructions + * is described briefly. For more details consult the PoP. + */ +typedef enum IOInstEnding { + IOINST_CC_0 = 0, /* produced expected result */ + IOINST_CC_1 = 1, /* status conditions were present, or alternate result */ + IOINST_CC_2 = 2, /* ineffective, busy with previously initiated function */ + IOINST_CC_3 = 3 /* ineffective, not operational */ +} IOInstEnding; + Alternatives I had in mind are IOINST_CC_0_EXPECTED, IOINST_CC_1_STATUS_PRESENT, IOINST_CC_2_BUSY, IOINST_CC_3_NOT_OPERATIONAL or the same without the numerical code (e.g. just IONIST_CC_EXPECTED). Regards, Halil [..]
On 17.10.2017 13:10, Halil Pasic wrote: > > > On 10/12/2017 01:44 PM, Halil Pasic wrote: >> >> >> On 10/12/2017 08:58 AM, Thomas Huth wrote: >>> On 10.10.2017 13:41, Halil Pasic wrote: > [..] >>> So yes, please don't do a "typedef unsigned int IOInstEnding" either. I >>> think the best match for QEMU would be a >>> >>> typedef enum IOInstEnding { >>> CC_..., >>> CC_..., >>> CC_..., >>> CC_... >>> } IOInstEnding; >>> >> >> I also prefer this over #define NAME val. >> > > @Conny @Thomas > > I'm almost done with v3, but I've realized we did not agree on the > names for the enum constants, so before posting something to ugly > again, I would like to inquire your opinion. > > My current version of the enum is the following (but I can easily change > to whatever you like with sed): > > +/* > + * IO instructions conclude according this. Currently we have only > + * cc codes. Valid values are 0,1,2,3 and the generic semantic for IO instructions > + * is described briefly. For more details consult the PoP. > + */ > +typedef enum IOInstEnding { > + IOINST_CC_0 = 0, /* produced expected result */ > + IOINST_CC_1 = 1, /* status conditions were present, or alternate result */ > + IOINST_CC_2 = 2, /* ineffective, busy with previously initiated function */ > + IOINST_CC_3 = 3 /* ineffective, not operational */ > +} IOInstEnding; > + > > Alternatives I had in mind are IOINST_CC_0_EXPECTED, IOINST_CC_1_STATUS_PRESENT, > IOINST_CC_2_BUSY, IOINST_CC_3_NOT_OPERATIONAL or the same without the numerical > code (e.g. just IONIST_CC_EXPECTED). FWIW, I'd prefer your last suggestion (e.g. IOINST_CC_EXPECTED). Thomas
On Tue, 17 Oct 2017 13:28:57 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 17.10.2017 13:10, Halil Pasic wrote: > > > > > > On 10/12/2017 01:44 PM, Halil Pasic wrote: > >> > >> > >> On 10/12/2017 08:58 AM, Thomas Huth wrote: > >>> On 10.10.2017 13:41, Halil Pasic wrote: > > [..] > >>> So yes, please don't do a "typedef unsigned int IOInstEnding" either. I > >>> think the best match for QEMU would be a > >>> > >>> typedef enum IOInstEnding { > >>> CC_..., > >>> CC_..., > >>> CC_..., > >>> CC_... > >>> } IOInstEnding; > >>> > >> > >> I also prefer this over #define NAME val. > >> > > > > @Conny @Thomas > > > > I'm almost done with v3, but I've realized we did not agree on the > > names for the enum constants, so before posting something to ugly > > again, I would like to inquire your opinion. > > > > My current version of the enum is the following (but I can easily change > > to whatever you like with sed): > > > > +/* > > + * IO instructions conclude according this. Currently we have only > > + * cc codes. Valid values are 0,1,2,3 and the generic semantic for IO instructions > > + * is described briefly. For more details consult the PoP. > > + */ > > +typedef enum IOInstEnding { > > + IOINST_CC_0 = 0, /* produced expected result */ > > + IOINST_CC_1 = 1, /* status conditions were present, or alternate result */ > > + IOINST_CC_2 = 2, /* ineffective, busy with previously initiated function */ > > + IOINST_CC_3 = 3 /* ineffective, not operational */ > > +} IOInstEnding; > > + > > > > Alternatives I had in mind are IOINST_CC_0_EXPECTED, IOINST_CC_1_STATUS_PRESENT, > > IOINST_CC_2_BUSY, IOINST_CC_3_NOT_OPERATIONAL or the same without the numerical > > code (e.g. just IONIST_CC_EXPECTED). > > FWIW, I'd prefer your last suggestion (e.g. IOINST_CC_EXPECTED). Either IOINST_CC_0 or IOINST_CC_EXPECTED, whatever you like best.
On 10/17/2017 02:13 PM, Cornelia Huck wrote: > On Tue, 17 Oct 2017 13:28:57 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> On 17.10.2017 13:10, Halil Pasic wrote: >>> >>> >>> On 10/12/2017 01:44 PM, Halil Pasic wrote: >>>> >>>> >>>> On 10/12/2017 08:58 AM, Thomas Huth wrote: >>>>> On 10.10.2017 13:41, Halil Pasic wrote: >>> [..] >>>>> So yes, please don't do a "typedef unsigned int IOInstEnding" either. I >>>>> think the best match for QEMU would be a >>>>> >>>>> typedef enum IOInstEnding { >>>>> CC_..., >>>>> CC_..., >>>>> CC_..., >>>>> CC_... >>>>> } IOInstEnding; >>>>> >>>> >>>> I also prefer this over #define NAME val. >>>> >>> >>> @Conny @Thomas >>> >>> I'm almost done with v3, but I've realized we did not agree on the >>> names for the enum constants, so before posting something to ugly >>> again, I would like to inquire your opinion. >>> >>> My current version of the enum is the following (but I can easily change >>> to whatever you like with sed): >>> >>> +/* >>> + * IO instructions conclude according this. Currently we have only >>> + * cc codes. Valid values are 0,1,2,3 and the generic semantic for IO instructions >>> + * is described briefly. For more details consult the PoP. >>> + */ >>> +typedef enum IOInstEnding { >>> + IOINST_CC_0 = 0, /* produced expected result */ >>> + IOINST_CC_1 = 1, /* status conditions were present, or alternate result */ >>> + IOINST_CC_2 = 2, /* ineffective, busy with previously initiated function */ >>> + IOINST_CC_3 = 3 /* ineffective, not operational */ >>> +} IOInstEnding; >>> + >>> >>> Alternatives I had in mind are IOINST_CC_0_EXPECTED, IOINST_CC_1_STATUS_PRESENT, >>> IOINST_CC_2_BUSY, IOINST_CC_3_NOT_OPERATIONAL or the same without the numerical >>> code (e.g. just IONIST_CC_EXPECTED). >> >> FWIW, I'd prefer your last suggestion (e.g. IOINST_CC_EXPECTED). > > Either IOINST_CC_0 or IOINST_CC_EXPECTED, whatever you like best. > Thanks, I will go with the non-numerical variant as preferred by Thomas (I've already changed my patches).
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h index 0653d3c9be..66916b6546 100644 --- a/include/hw/s390x/css.h +++ b/include/hw/s390x/css.h @@ -75,6 +75,18 @@ typedef struct CMBE { uint32_t reserved[7]; } QEMU_PACKED CMBE; +/* IO instructions conclude according this */ +typedef struct IOInstEnding { + /* + * General semantic of cc codes of IO instructions is (brief): + * 0 -- produced expected result + * 1 -- status conditions were present or produced alternate result + * 2 -- ineffective, because busy with previously initiated function + * 3 -- ineffective, not operational + */ + int cc; +} IOInstEnding; + typedef struct SubchDev SubchDev; struct SubchDev { /* channel-subsystem related things: */
CSS code needs to tell the IO instruction handlers located in how should the emulated instruction be ended. Currently this is done by returning generic (POSIX) error codes, and mapping them to outcomes like condition codes. This makes bugs easy to create and hard to recognise. As a preparation for moving a way form (mis)using generic error codes for flow control let us introduce a struct which tells the instruction handler function how to end the instruction, in a more straight-forward and less ambiguous way. Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> --- include/hw/s390x/css.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)