* [PATCH] hw/pci-host/grackle: Verify PIC link is properly set @ 2020-10-11 19:03 Philippe Mathieu-Daudé 2020-10-11 22:34 ` David Gibson 2020-10-12 9:23 ` Mark Cave-Ayland 0 siblings, 2 replies; 17+ messages in thread From: Philippe Mathieu-Daudé @ 2020-10-11 19:03 UTC (permalink / raw) To: qemu-devel Cc: Mark Cave-Ayland, qemu-ppc, Philippe Mathieu-Daudé, David Gibson The Grackle PCI host model expects the interrupt controller being set, but does not verify it is present. Add a check to help developers using this model. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- hw/pci-host/grackle.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c index 57c29b20afb..20361d215ca 100644 --- a/hw/pci-host/grackle.c +++ b/hw/pci-host/grackle.c @@ -76,6 +76,10 @@ static void grackle_realize(DeviceState *dev, Error **errp) GrackleState *s = GRACKLE_PCI_HOST_BRIDGE(dev); PCIHostState *phb = PCI_HOST_BRIDGE(dev); + if (!s->pic) { + error_setg(errp, TYPE_GRACKLE_PCI_HOST_BRIDGE ": 'pic' link not set"); + return; + } phb->bus = pci_register_root_bus(dev, NULL, pci_grackle_set_irq, pci_grackle_map_irq, -- 2.26.2 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set 2020-10-11 19:03 [PATCH] hw/pci-host/grackle: Verify PIC link is properly set Philippe Mathieu-Daudé @ 2020-10-11 22:34 ` David Gibson 2020-10-12 6:21 ` Philippe Mathieu-Daudé 2020-10-12 9:23 ` Mark Cave-Ayland 1 sibling, 1 reply; 17+ messages in thread From: David Gibson @ 2020-10-11 22:34 UTC (permalink / raw) To: Philippe Mathieu-Daudé; +Cc: Mark Cave-Ayland, qemu-ppc, qemu-devel [-- Attachment #1: Type: text/plain, Size: 1322 bytes --] On Sun, Oct 11, 2020 at 09:03:32PM +0200, Philippe Mathieu-Daudé wrote: > The Grackle PCI host model expects the interrupt controller > being set, but does not verify it is present. Add a check to > help developers using this model. I don't think thaqt's very likely, but, sure, applied to ppc-for-5.2 > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/pci-host/grackle.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c > index 57c29b20afb..20361d215ca 100644 > --- a/hw/pci-host/grackle.c > +++ b/hw/pci-host/grackle.c > @@ -76,6 +76,10 @@ static void grackle_realize(DeviceState *dev, Error **errp) > GrackleState *s = GRACKLE_PCI_HOST_BRIDGE(dev); > PCIHostState *phb = PCI_HOST_BRIDGE(dev); > > + if (!s->pic) { > + error_setg(errp, TYPE_GRACKLE_PCI_HOST_BRIDGE ": 'pic' link not set"); > + return; > + } > phb->bus = pci_register_root_bus(dev, NULL, > pci_grackle_set_irq, > pci_grackle_map_irq, -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set 2020-10-11 22:34 ` David Gibson @ 2020-10-12 6:21 ` Philippe Mathieu-Daudé 2020-10-12 6:54 ` David Gibson 0 siblings, 1 reply; 17+ messages in thread From: Philippe Mathieu-Daudé @ 2020-10-12 6:21 UTC (permalink / raw) To: David Gibson; +Cc: qemu-ppc, Mark Cave-Ayland, qemu-devel On 10/12/20 12:34 AM, David Gibson wrote: > On Sun, Oct 11, 2020 at 09:03:32PM +0200, Philippe Mathieu-Daudé wrote: >> The Grackle PCI host model expects the interrupt controller >> being set, but does not verify it is present. Add a check to >> help developers using this model. > > I don't think thaqt's very likely, but, sure, applied to ppc-for-5.2 Do you want I correct the description as: "The Grackle PCI host model expects the interrupt controller being set, but does not verify it is present. A developer basing its implementation on the Grackle model might hit this problem. Add a check to help future developers using this model as reference."? > >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/pci-host/grackle.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c >> index 57c29b20afb..20361d215ca 100644 >> --- a/hw/pci-host/grackle.c >> +++ b/hw/pci-host/grackle.c >> @@ -76,6 +76,10 @@ static void grackle_realize(DeviceState *dev, Error **errp) >> GrackleState *s = GRACKLE_PCI_HOST_BRIDGE(dev); >> PCIHostState *phb = PCI_HOST_BRIDGE(dev); >> >> + if (!s->pic) { >> + error_setg(errp, TYPE_GRACKLE_PCI_HOST_BRIDGE ": 'pic' link not set"); >> + return; >> + } >> phb->bus = pci_register_root_bus(dev, NULL, >> pci_grackle_set_irq, >> pci_grackle_map_irq, > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set 2020-10-12 6:21 ` Philippe Mathieu-Daudé @ 2020-10-12 6:54 ` David Gibson 2020-10-12 11:50 ` BALATON Zoltan via 0 siblings, 1 reply; 17+ messages in thread From: David Gibson @ 2020-10-12 6:54 UTC (permalink / raw) To: Philippe Mathieu-Daudé; +Cc: qemu-ppc, Mark Cave-Ayland, qemu-devel [-- Attachment #1: Type: text/plain, Size: 2005 bytes --] On Mon, Oct 12, 2020 at 08:21:41AM +0200, Philippe Mathieu-Daudé wrote: > On 10/12/20 12:34 AM, David Gibson wrote: > > On Sun, Oct 11, 2020 at 09:03:32PM +0200, Philippe Mathieu-Daudé wrote: > > > The Grackle PCI host model expects the interrupt controller > > > being set, but does not verify it is present. Add a check to > > > help developers using this model. > > > > I don't think thaqt's very likely, but, sure, applied to ppc-for-5.2 > > Do you want I correct the description as: > "The Grackle PCI host model expects the interrupt controller > being set, but does not verify it is present. > A developer basing its implementation on the Grackle model > might hit this problem. Add a check to help future developers > using this model as reference."? No, it's fine. All I was saying is that the chances of anyone using Grackle in future seem very low to me. > > > > > > > > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > > --- > > > hw/pci-host/grackle.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c > > > index 57c29b20afb..20361d215ca 100644 > > > --- a/hw/pci-host/grackle.c > > > +++ b/hw/pci-host/grackle.c > > > @@ -76,6 +76,10 @@ static void grackle_realize(DeviceState *dev, Error **errp) > > > GrackleState *s = GRACKLE_PCI_HOST_BRIDGE(dev); > > > PCIHostState *phb = PCI_HOST_BRIDGE(dev); > > > + if (!s->pic) { > > > + error_setg(errp, TYPE_GRACKLE_PCI_HOST_BRIDGE ": 'pic' link not set"); > > > + return; > > > + } > > > phb->bus = pci_register_root_bus(dev, NULL, > > > pci_grackle_set_irq, > > > pci_grackle_map_irq, > > > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set 2020-10-12 6:54 ` David Gibson @ 2020-10-12 11:50 ` BALATON Zoltan via 2020-10-12 12:00 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 17+ messages in thread From: BALATON Zoltan via @ 2020-10-12 11:50 UTC (permalink / raw) To: David Gibson; +Cc: qemu-ppc, Philippe Mathieu-Daudé, qemu-devel [-- Attachment #1: Type: text/plain, Size: 1892 bytes --] On Mon, 12 Oct 2020, David Gibson wrote: > On Mon, Oct 12, 2020 at 08:21:41AM +0200, Philippe Mathieu-Daudé wrote: >> On 10/12/20 12:34 AM, David Gibson wrote: >>> On Sun, Oct 11, 2020 at 09:03:32PM +0200, Philippe Mathieu-Daudé wrote: >>>> The Grackle PCI host model expects the interrupt controller >>>> being set, but does not verify it is present. Add a check to >>>> help developers using this model. >>> >>> I don't think thaqt's very likely, but, sure, applied to ppc-for-5.2 >> >> Do you want I correct the description as: >> "The Grackle PCI host model expects the interrupt controller >> being set, but does not verify it is present. >> A developer basing its implementation on the Grackle model >> might hit this problem. Add a check to help future developers >> using this model as reference."? > > No, it's fine. All I was saying is that the chances of anyone using > Grackle in future seem very low to me. So maybe an assert instead of a user visible error would be enough? Regards, BALATON Zoltan > > >> >>> >>>> >>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>> --- >>>> hw/pci-host/grackle.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c >>>> index 57c29b20afb..20361d215ca 100644 >>>> --- a/hw/pci-host/grackle.c >>>> +++ b/hw/pci-host/grackle.c >>>> @@ -76,6 +76,10 @@ static void grackle_realize(DeviceState *dev, Error **errp) >>>> GrackleState *s = GRACKLE_PCI_HOST_BRIDGE(dev); >>>> PCIHostState *phb = PCI_HOST_BRIDGE(dev); >>>> + if (!s->pic) { >>>> + error_setg(errp, TYPE_GRACKLE_PCI_HOST_BRIDGE ": 'pic' link not set"); >>>> + return; >>>> + } >>>> phb->bus = pci_register_root_bus(dev, NULL, >>>> pci_grackle_set_irq, >>>> pci_grackle_map_irq, >>> >> > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set 2020-10-12 11:50 ` BALATON Zoltan via @ 2020-10-12 12:00 ` Philippe Mathieu-Daudé 2020-10-19 7:50 ` Markus Armbruster 0 siblings, 1 reply; 17+ messages in thread From: Philippe Mathieu-Daudé @ 2020-10-12 12:00 UTC (permalink / raw) To: BALATON Zoltan, David Gibson, Markus Armbruster, Eduardo Habkost Cc: qemu-ppc, qemu-devel On 10/12/20 1:50 PM, BALATON Zoltan via wrote: > On Mon, 12 Oct 2020, David Gibson wrote: >> On Mon, Oct 12, 2020 at 08:21:41AM +0200, Philippe Mathieu-Daudé >> wrote: >>> On 10/12/20 12:34 AM, David Gibson wrote: >>>> On Sun, Oct 11, 2020 at 09:03:32PM +0200, Philippe Mathieu-Daudé >>>> wrote: >>>>> The Grackle PCI host model expects the interrupt controller >>>>> being set, but does not verify it is present. Add a check to >>>>> help developers using this model. >>>> >>>> I don't think thaqt's very likely, but, sure, applied to ppc-for-5.2 >>> >>> Do you want I correct the description as: >>> "The Grackle PCI host model expects the interrupt controller >>> being set, but does not verify it is present. >>> A developer basing its implementation on the Grackle model >>> might hit this problem. Add a check to help future developers >>> using this model as reference."? >> >> No, it's fine. All I was saying is that the chances of anyone using >> Grackle in future seem very low to me. > > So maybe an assert instead of a user visible error would be enough? My understanding is realize() shouldn't abort() (the caller might choose to by using &error_abort). > > Regards, > BALATON Zoltan > >> >> >>> >>>> >>>>> >>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>>>> --- >>>>> hw/pci-host/grackle.c | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c >>>>> index 57c29b20afb..20361d215ca 100644 >>>>> --- a/hw/pci-host/grackle.c >>>>> +++ b/hw/pci-host/grackle.c >>>>> @@ -76,6 +76,10 @@ static void grackle_realize(DeviceState *dev, >>>>> Error **errp) >>>>> GrackleState *s = GRACKLE_PCI_HOST_BRIDGE(dev); >>>>> PCIHostState *phb = PCI_HOST_BRIDGE(dev); >>>>> + if (!s->pic) { >>>>> + error_setg(errp, TYPE_GRACKLE_PCI_HOST_BRIDGE ": 'pic' >>>>> link not set"); >>>>> + return; >>>>> + } >>>>> phb->bus = pci_register_root_bus(dev, NULL, >>>>> pci_grackle_set_irq, >>>>> pci_grackle_map_irq, >>>> >>> >> >> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set 2020-10-12 12:00 ` Philippe Mathieu-Daudé @ 2020-10-19 7:50 ` Markus Armbruster 2020-10-19 11:11 ` BALATON Zoltan via 0 siblings, 1 reply; 17+ messages in thread From: Markus Armbruster @ 2020-10-19 7:50 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: qemu-devel, David Gibson, qemu-ppc, Eduardo Habkost Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > On 10/12/20 1:50 PM, BALATON Zoltan via wrote: >> On Mon, 12 Oct 2020, David Gibson wrote: >>> On Mon, Oct 12, 2020 at 08:21:41AM +0200, Philippe >>> Mathieu-Daudé wrote: >>>> On 10/12/20 12:34 AM, David Gibson wrote: >>>>> On Sun, Oct 11, 2020 at 09:03:32PM +0200, Philippe >>>>> Mathieu-Daudé wrote: >>>>>> The Grackle PCI host model expects the interrupt controller >>>>>> being set, but does not verify it is present. Add a check to >>>>>> help developers using this model. >>>>> >>>>> I don't think thaqt's very likely, but, sure, applied to ppc-for-5.2 >>>> >>>> Do you want I correct the description as: >>>> "The Grackle PCI host model expects the interrupt controller >>>> being set, but does not verify it is present. >>>> A developer basing its implementation on the Grackle model >>>> might hit this problem. Add a check to help future developers >>>> using this model as reference."? >>> >>> No, it's fine. All I was saying is that the chances of anyone using >>> Grackle in future seem very low to me. >> So maybe an assert instead of a user visible error would be enough? > > My understanding is realize() shouldn't abort() > (the caller might choose to by using &error_abort). assert() is for checking invariants. A violated invariant is a programming error: developers screwed up, safe recovery is impossible. Abusing assert() to catch errors that are not programming errors is wrong. You may check invariants with assert() anywhere in the code. You should not misuse assert() anywhere in the code. Sometimes, an error condition that is *not* a programming error in the function where it is detected *is* a programming error for certain calls. Having these calls pass &error_abort is a common solution for this problem. Hope this helps. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set 2020-10-19 7:50 ` Markus Armbruster @ 2020-10-19 11:11 ` BALATON Zoltan via 2020-10-19 14:00 ` Markus Armbruster 0 siblings, 1 reply; 17+ messages in thread From: BALATON Zoltan via @ 2020-10-19 11:11 UTC (permalink / raw) To: Markus Armbruster Cc: David Gibson, qemu-ppc, Philippe Mathieu-Daudé, Eduardo Habkost, qemu-devel [-- Attachment #1: Type: text/plain, Size: 2937 bytes --] On Mon, 19 Oct 2020, Markus Armbruster wrote: > Philippe Mathieu-Daudé <f4bug@amsat.org> writes: >> On 10/12/20 1:50 PM, BALATON Zoltan via wrote: >>> On Mon, 12 Oct 2020, David Gibson wrote: >>>> On Mon, Oct 12, 2020 at 08:21:41AM +0200, Philippe >>>> Mathieu-Daudé wrote: >>>>> On 10/12/20 12:34 AM, David Gibson wrote: >>>>>> On Sun, Oct 11, 2020 at 09:03:32PM +0200, Philippe >>>>>> Mathieu-Daudé wrote: >>>>>>> The Grackle PCI host model expects the interrupt controller >>>>>>> being set, but does not verify it is present. Add a check to >>>>>>> help developers using this model. >>>>>> >>>>>> I don't think thaqt's very likely, but, sure, applied to ppc-for-5.2 >>>>> >>>>> Do you want I correct the description as: >>>>> "The Grackle PCI host model expects the interrupt controller >>>>> being set, but does not verify it is present. >>>>> A developer basing its implementation on the Grackle model >>>>> might hit this problem. Add a check to help future developers >>>>> using this model as reference."? >>>> >>>> No, it's fine. All I was saying is that the chances of anyone using >>>> Grackle in future seem very low to me. >>> So maybe an assert instead of a user visible error would be enough? >> >> My understanding is realize() shouldn't abort() >> (the caller might choose to by using &error_abort). > > assert() is for checking invariants. A violated invariant is a > programming error: developers screwed up, safe recovery is impossible. > > Abusing assert() to catch errors that are not programming errors is > wrong. > > You may check invariants with assert() anywhere in the code. > > You should not misuse assert() anywhere in the code. > > Sometimes, an error condition that is *not* a programming error in the > function where it is detected *is* a programming error for certain > calls. Having these calls pass &error_abort is a common solution for > this problem. > > Hope this helps. Helps just a bit but after reading this I'm still confused if this particular case should be assert or ser error. I was suggesting assert and I think it's a programming error to use the grackle model without setting PIC link but not sure if users can also create this instance via command line (even if it does not make much sense) in which case it's probably better to return error. Having all devices user creatable via -device without a way to describe their dependencies is a nice way to make all sorts of errors possible. But maybe aborting with assert during creation of the machine is still OK. If people device_add a model later and that crashes then it's their problem. Unless we want to avoid that being used as DoS in enterprise setting. So maybe we should never abort then if there's a way to fail with an error instead. I can look at this problem from different angles and all seem to be plausible resulting in different solutions. Regards, BALATON Zoltan ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set 2020-10-19 11:11 ` BALATON Zoltan via @ 2020-10-19 14:00 ` Markus Armbruster 2020-10-19 14:38 ` Mark Cave-Ayland 0 siblings, 1 reply; 17+ messages in thread From: Markus Armbruster @ 2020-10-19 14:00 UTC (permalink / raw) To: BALATON Zoltan via Cc: David Gibson, qemu-ppc, Philippe Mathieu-Daudé, Eduardo Habkost BALATON Zoltan via <qemu-devel@nongnu.org> writes: > On Mon, 19 Oct 2020, Markus Armbruster wrote: >> Philippe Mathieu-Daudé <f4bug@amsat.org> writes: >>> On 10/12/20 1:50 PM, BALATON Zoltan via wrote: >>>> On Mon, 12 Oct 2020, David Gibson wrote: >>>>> On Mon, Oct 12, 2020 at 08:21:41AM +0200, Philippe >>>>> Mathieu-Daudé wrote: >>>>>> On 10/12/20 12:34 AM, David Gibson wrote: >>>>>>> On Sun, Oct 11, 2020 at 09:03:32PM +0200, Philippe >>>>>>> Mathieu-Daudé wrote: >>>>>>>> The Grackle PCI host model expects the interrupt controller >>>>>>>> being set, but does not verify it is present. Add a check to >>>>>>>> help developers using this model. >>>>>>> >>>>>>> I don't think thaqt's very likely, but, sure, applied to ppc-for-5.2 >>>>>> >>>>>> Do you want I correct the description as: >>>>>> "The Grackle PCI host model expects the interrupt controller >>>>>> being set, but does not verify it is present. >>>>>> A developer basing its implementation on the Grackle model >>>>>> might hit this problem. Add a check to help future developers >>>>>> using this model as reference."? >>>>> >>>>> No, it's fine. All I was saying is that the chances of anyone using >>>>> Grackle in future seem very low to me. >>>> So maybe an assert instead of a user visible error would be enough? >>> >>> My understanding is realize() shouldn't abort() >>> (the caller might choose to by using &error_abort). >> >> assert() is for checking invariants. A violated invariant is a >> programming error: developers screwed up, safe recovery is impossible. >> >> Abusing assert() to catch errors that are not programming errors is >> wrong. >> >> You may check invariants with assert() anywhere in the code. >> >> You should not misuse assert() anywhere in the code. >> >> Sometimes, an error condition that is *not* a programming error in the >> function where it is detected *is* a programming error for certain >> calls. Having these calls pass &error_abort is a common solution for >> this problem. >> >> Hope this helps. > > Helps just a bit but after reading this I'm still confused if this > particular case should be assert or ser error. > > I was suggesting assert and I think it's a programming error to use > the grackle model without setting PIC link but not sure if users can > also create this instance via command line (even if it does not make > much sense) in which case it's probably better to return error. They can't: "info qdm" shows name "grackle-pcihost", bus System, no-user ~~~~~~~ > Having > all devices user creatable via -device without a way to describe their > dependencies is a nice way to make all sorts of errors possible. But > maybe aborting with assert during creation of the machine is still > OK. If people device_add a model later and that crashes then it's > their problem. Unless we want to avoid that being used as DoS in > enterprise setting. So maybe we should never abort then if there's a > way to fail with an error instead. > > I can look at this problem from different angles and all seem to be > plausible resulting in different solutions. As long as the device is no-user, asserting that properties have sane values feels reasonable enough to me. Setting an error instead is not wrong, of course. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set 2020-10-19 14:00 ` Markus Armbruster @ 2020-10-19 14:38 ` Mark Cave-Ayland 2020-10-19 16:17 ` BALATON Zoltan via 2020-10-20 5:30 ` Markus Armbruster 0 siblings, 2 replies; 17+ messages in thread From: Mark Cave-Ayland @ 2020-10-19 14:38 UTC (permalink / raw) To: Markus Armbruster, BALATON Zoltan via Cc: qemu-ppc, Philippe Mathieu-Daudé, Eduardo Habkost, David Gibson On 19/10/2020 15:00, Markus Armbruster wrote: > BALATON Zoltan via <qemu-devel@nongnu.org> writes: > >> On Mon, 19 Oct 2020, Markus Armbruster wrote: >>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes: >>>> On 10/12/20 1:50 PM, BALATON Zoltan via wrote: >>>>> On Mon, 12 Oct 2020, David Gibson wrote: >>>>>> On Mon, Oct 12, 2020 at 08:21:41AM +0200, Philippe >>>>>> Mathieu-Daudé wrote: >>>>>>> On 10/12/20 12:34 AM, David Gibson wrote: >>>>>>>> On Sun, Oct 11, 2020 at 09:03:32PM +0200, Philippe >>>>>>>> Mathieu-Daudé wrote: >>>>>>>>> The Grackle PCI host model expects the interrupt controller >>>>>>>>> being set, but does not verify it is present. Add a check to >>>>>>>>> help developers using this model. >>>>>>>> >>>>>>>> I don't think thaqt's very likely, but, sure, applied to ppc-for-5.2 >>>>>>> >>>>>>> Do you want I correct the description as: >>>>>>> "The Grackle PCI host model expects the interrupt controller >>>>>>> being set, but does not verify it is present. >>>>>>> A developer basing its implementation on the Grackle model >>>>>>> might hit this problem. Add a check to help future developers >>>>>>> using this model as reference."? >>>>>> >>>>>> No, it's fine. All I was saying is that the chances of anyone using >>>>>> Grackle in future seem very low to me. >>>>> So maybe an assert instead of a user visible error would be enough? >>>> >>>> My understanding is realize() shouldn't abort() >>>> (the caller might choose to by using &error_abort). >>> >>> assert() is for checking invariants. A violated invariant is a >>> programming error: developers screwed up, safe recovery is impossible. >>> >>> Abusing assert() to catch errors that are not programming errors is >>> wrong. >>> >>> You may check invariants with assert() anywhere in the code. >>> >>> You should not misuse assert() anywhere in the code. >>> >>> Sometimes, an error condition that is *not* a programming error in the >>> function where it is detected *is* a programming error for certain >>> calls. Having these calls pass &error_abort is a common solution for >>> this problem. >>> >>> Hope this helps. >> >> Helps just a bit but after reading this I'm still confused if this >> particular case should be assert or ser error. >> >> I was suggesting assert and I think it's a programming error to use >> the grackle model without setting PIC link but not sure if users can >> also create this instance via command line (even if it does not make >> much sense) in which case it's probably better to return error. > > They can't: "info qdm" shows > > name "grackle-pcihost", bus System, no-user > ~~~~~~~ > >> Having >> all devices user creatable via -device without a way to describe their >> dependencies is a nice way to make all sorts of errors possible. But >> maybe aborting with assert during creation of the machine is still >> OK. If people device_add a model later and that crashes then it's >> their problem. Unless we want to avoid that being used as DoS in >> enterprise setting. So maybe we should never abort then if there's a >> way to fail with an error instead. >> >> I can look at this problem from different angles and all seem to be >> plausible resulting in different solutions. > > As long as the device is no-user, asserting that properties have sane > values feels reasonable enough to me. > > Setting an error instead is not wrong, of course. One thing I have thought about is being able to mark a link property as mandatory so if a value hasn't been set before realize then you receive a fatal error. This would be for cases like this where 2 internal devices are connected together without any formal interface, i.e. in cases where -device wouldn't work anyway. I should also add that this patch has been dropped, and its replacement is now in git master - the wiring of the PIC has been moved from the grackle PCI host bridge up to the machine level where it really belongs. ATB, Mark. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set 2020-10-19 14:38 ` Mark Cave-Ayland @ 2020-10-19 16:17 ` BALATON Zoltan via 2020-10-20 5:30 ` Markus Armbruster 1 sibling, 0 replies; 17+ messages in thread From: BALATON Zoltan via @ 2020-10-19 16:17 UTC (permalink / raw) To: Mark Cave-Ayland Cc: Eduardo Habkost, BALATON Zoltan via, Philippe Mathieu-Daudé, Markus Armbruster, qemu-ppc, David Gibson [-- Attachment #1: Type: text/plain, Size: 4434 bytes --] On Mon, 19 Oct 2020, Mark Cave-Ayland wrote: > On 19/10/2020 15:00, Markus Armbruster wrote: >> BALATON Zoltan via <qemu-devel@nongnu.org> writes: >>> On Mon, 19 Oct 2020, Markus Armbruster wrote: >>>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes: >>>>> On 10/12/20 1:50 PM, BALATON Zoltan via wrote: >>>>>> On Mon, 12 Oct 2020, David Gibson wrote: >>>>>>> On Mon, Oct 12, 2020 at 08:21:41AM +0200, Philippe >>>>>>> Mathieu-Daudé wrote: >>>>>>>> On 10/12/20 12:34 AM, David Gibson wrote: >>>>>>>>> On Sun, Oct 11, 2020 at 09:03:32PM +0200, Philippe >>>>>>>>> Mathieu-Daudé wrote: >>>>>>>>>> The Grackle PCI host model expects the interrupt controller >>>>>>>>>> being set, but does not verify it is present. Add a check to >>>>>>>>>> help developers using this model. >>>>>>>>> >>>>>>>>> I don't think thaqt's very likely, but, sure, applied to ppc-for-5.2 >>>>>>>> >>>>>>>> Do you want I correct the description as: >>>>>>>> "The Grackle PCI host model expects the interrupt controller >>>>>>>> being set, but does not verify it is present. >>>>>>>> A developer basing its implementation on the Grackle model >>>>>>>> might hit this problem. Add a check to help future developers >>>>>>>> using this model as reference."? >>>>>>> >>>>>>> No, it's fine. All I was saying is that the chances of anyone using >>>>>>> Grackle in future seem very low to me. >>>>>> So maybe an assert instead of a user visible error would be enough? >>>>> >>>>> My understanding is realize() shouldn't abort() >>>>> (the caller might choose to by using &error_abort). >>>> >>>> assert() is for checking invariants. A violated invariant is a >>>> programming error: developers screwed up, safe recovery is impossible. >>>> >>>> Abusing assert() to catch errors that are not programming errors is >>>> wrong. >>>> >>>> You may check invariants with assert() anywhere in the code. >>>> >>>> You should not misuse assert() anywhere in the code. >>>> >>>> Sometimes, an error condition that is *not* a programming error in the >>>> function where it is detected *is* a programming error for certain >>>> calls. Having these calls pass &error_abort is a common solution for >>>> this problem. >>>> >>>> Hope this helps. >>> >>> Helps just a bit but after reading this I'm still confused if this >>> particular case should be assert or ser error. >>> >>> I was suggesting assert and I think it's a programming error to use >>> the grackle model without setting PIC link but not sure if users can >>> also create this instance via command line (even if it does not make >>> much sense) in which case it's probably better to return error. >> >> They can't: "info qdm" shows >> >> name "grackle-pcihost", bus System, no-user >> ~~~~~~~ >> >>> Having >>> all devices user creatable via -device without a way to describe their >>> dependencies is a nice way to make all sorts of errors possible. But >>> maybe aborting with assert during creation of the machine is still >>> OK. If people device_add a model later and that crashes then it's >>> their problem. Unless we want to avoid that being used as DoS in >>> enterprise setting. So maybe we should never abort then if there's a >>> way to fail with an error instead. >>> >>> I can look at this problem from different angles and all seem to be >>> plausible resulting in different solutions. >> >> As long as the device is no-user, asserting that properties have sane >> values feels reasonable enough to me. >> >> Setting an error instead is not wrong, of course. > > One thing I have thought about is being able to mark a link property as > mandatory so if a value hasn't been set before realize then you receive a > fatal error. This would be for cases like this where 2 internal devices are > connected together without any formal interface, i.e. in cases where -device > wouldn't work anyway. In that case we should have no-user anyway and the device has to be instantiated by board code so then assert is enough to avoid error in board code. More complex way to describe dependencies may only be needed if we want to allow dynamic creation of new boards via the command line which was the original idea but that's quite a long way away if possible at all so I would not worry about that at this point. Regards, BALATON Zoltan ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set 2020-10-19 14:38 ` Mark Cave-Ayland 2020-10-19 16:17 ` BALATON Zoltan via @ 2020-10-20 5:30 ` Markus Armbruster 2020-10-20 11:37 ` BALATON Zoltan via 1 sibling, 1 reply; 17+ messages in thread From: Markus Armbruster @ 2020-10-20 5:30 UTC (permalink / raw) To: Mark Cave-Ayland Cc: David Gibson, qemu-ppc, BALATON Zoltan via, Eduardo Habkost, Philippe Mathieu-Daudé Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes: > One thing I have thought about is being able to mark a link property > as mandatory so if a value hasn't been set before realize then you A non-null value, I presume. > receive a fatal error. This would be for cases like this where 2 > internal devices are connected together without any formal interface, > i.e. in cases where -device wouldn't work anyway. Moves the check from code one step closer to data: from the realize method to the object_property_add_link() call. I like doing things in data, because data is easier to reason about than code. [...] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set 2020-10-20 5:30 ` Markus Armbruster @ 2020-10-20 11:37 ` BALATON Zoltan via 2020-10-21 3:31 ` Markus Armbruster 0 siblings, 1 reply; 17+ messages in thread From: BALATON Zoltan via @ 2020-10-20 11:37 UTC (permalink / raw) To: Markus Armbruster Cc: Eduardo Habkost, Mark Cave-Ayland, Philippe Mathieu-Daudé, BALATON Zoltan via, qemu-ppc, David Gibson On Tue, 20 Oct 2020, Markus Armbruster wrote: > Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes: > >> One thing I have thought about is being able to mark a link property >> as mandatory so if a value hasn't been set before realize then you > > A non-null value, I presume. Do you mean something like distinguish between NULL and INVALID_VALUE where setting the latter as initial value means property is mandatory? >> receive a fatal error. This would be for cases like this where 2 >> internal devices are connected together without any formal interface, >> i.e. in cases where -device wouldn't work anyway. > > Moves the check from code one step closer to data: from the realize > method to the object_property_add_link() call. > > I like doing things in data, because data is easier to reason about than > code. Except when object initialisation is scattered around in boiler plate code as in QEMU where it may not be obvious why a realize method fails due to something set/documented in a class_init method elsewhere, whereas an assert in the realize method is quite self evident and would document the same requirements. Regards. BALATON Zoltan ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set 2020-10-20 11:37 ` BALATON Zoltan via @ 2020-10-21 3:31 ` Markus Armbruster 2020-10-21 10:21 ` BALATON Zoltan via 0 siblings, 1 reply; 17+ messages in thread From: Markus Armbruster @ 2020-10-21 3:31 UTC (permalink / raw) To: BALATON Zoltan via Cc: Eduardo Habkost, Mark Cave-Ayland, Philippe Mathieu-Daudé, qemu-ppc, David Gibson BALATON Zoltan via <qemu-devel@nongnu.org> writes: > On Tue, 20 Oct 2020, Markus Armbruster wrote: >> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes: >> >>> One thing I have thought about is being able to mark a link property >>> as mandatory so if a value hasn't been set before realize then you >> >> A non-null value, I presume. > > Do you mean something like distinguish between NULL and INVALID_VALUE > where setting the latter as initial value means property is mandatory? I doubt "somebody must have set some value (which could be null)" is useful here. I believe Mark was thinking about "somebody must have connected the link (i.e. set a non-null value)". >>> receive a fatal error. This would be for cases like this where 2 >>> internal devices are connected together without any formal interface, >>> i.e. in cases where -device wouldn't work anyway. >> >> Moves the check from code one step closer to data: from the realize >> method to the object_property_add_link() call. >> >> I like doing things in data, because data is easier to reason about than >> code. > > Except when object initialisation is scattered around in boiler plate > code as in QEMU where it may not be obvious why a realize method fails > due to something set/documented in a class_init method elsewhere, > whereas an assert in the realize method is quite self evident and > would document the same requirements. Two aspects. One, making sense of an assertion failure. Whether some ad hoc assert(s->some_other_object); in the device realize method crashes, or a generic if (!object_property_get_link(s, prop->name, &error_abort)) { error_setg(errp, "link %s must be connected", prop->name); } crashes in the error_setg() is all the same to me. Some developers may find the latter's crash message superior (I don't care). Two, making sense of the code. You have to consider property definition (which might be code in .class_init(), code in .instance_init(), or data passed to .device_class_set_props()), property use (in the code that sets up the device), and property value checking (generic checks in qom/object.c, specific checks in .realize()). Moving one check from "specific" to "generic" won't make much of a difference. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set 2020-10-21 3:31 ` Markus Armbruster @ 2020-10-21 10:21 ` BALATON Zoltan via 0 siblings, 0 replies; 17+ messages in thread From: BALATON Zoltan via @ 2020-10-21 10:21 UTC (permalink / raw) To: Markus Armbruster Cc: Eduardo Habkost, Mark Cave-Ayland, Philippe Mathieu-Daudé, BALATON Zoltan via, qemu-ppc, David Gibson On Wed, 21 Oct 2020, Markus Armbruster wrote: > BALATON Zoltan via <qemu-devel@nongnu.org> writes: >> On Tue, 20 Oct 2020, Markus Armbruster wrote: >>> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes: >>> >>>> One thing I have thought about is being able to mark a link property >>>> as mandatory so if a value hasn't been set before realize then you >>> >>> A non-null value, I presume. >> >> Do you mean something like distinguish between NULL and INVALID_VALUE >> where setting the latter as initial value means property is mandatory? > > I doubt "somebody must have set some value (which could be null)" is > useful here. I believe Mark was thinking about "somebody must have > connected the link (i.e. set a non-null value)". This is all theoretical, as in this case we have no_user anyway so the problem can only happen through programmer error but if an object has several properties some of which can be NULL while others are mandatory how do you tell which one is mandataory? That's what I thouhgt having initial value some INVALID_VALUE could be used to say these are mandatory while those properties that are initially NULL don't need to have a valie. That way your generic check can work, otherwise it will need another way to describe mandatory properties which is additional complexity and more boilerplate or will make every link property mandatory which may not always work. Anyway, I was just trying to understand your suggestion and since we concluded that we don't really need it for this case we can just say "YAGNI" as I've learned from you and leave it at that. Regards, BALATON Zoltan ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set 2020-10-11 19:03 [PATCH] hw/pci-host/grackle: Verify PIC link is properly set Philippe Mathieu-Daudé 2020-10-11 22:34 ` David Gibson @ 2020-10-12 9:23 ` Mark Cave-Ayland 2020-10-12 12:01 ` Philippe Mathieu-Daudé 1 sibling, 1 reply; 17+ messages in thread From: Mark Cave-Ayland @ 2020-10-12 9:23 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel; +Cc: qemu-ppc, David Gibson On 11/10/2020 20:03, Philippe Mathieu-Daudé wrote: > The Grackle PCI host model expects the interrupt controller > being set, but does not verify it is present. Add a check to > help developers using this model. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/pci-host/grackle.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c > index 57c29b20afb..20361d215ca 100644 > --- a/hw/pci-host/grackle.c > +++ b/hw/pci-host/grackle.c > @@ -76,6 +76,10 @@ static void grackle_realize(DeviceState *dev, Error **errp) > GrackleState *s = GRACKLE_PCI_HOST_BRIDGE(dev); > PCIHostState *phb = PCI_HOST_BRIDGE(dev); > > + if (!s->pic) { > + error_setg(errp, TYPE_GRACKLE_PCI_HOST_BRIDGE ": 'pic' link not set"); > + return; > + } > phb->bus = pci_register_root_bus(dev, NULL, > pci_grackle_set_irq, > pci_grackle_map_irq, Reviewing this now, my feeling is that both grackle and uninorth are doing the wrong thing here by passing in a link to the PIC - certainly if I had written this more recently than I originally did, I would simply use qdev_init_gpio_out() for the IRQ lines and do the wiring during machine init. I've got a few other things to look at first, but I'll post a patchset later which I think will improve this for both Mac machines. ATB, Mark. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set 2020-10-12 9:23 ` Mark Cave-Ayland @ 2020-10-12 12:01 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 17+ messages in thread From: Philippe Mathieu-Daudé @ 2020-10-12 12:01 UTC (permalink / raw) To: Mark Cave-Ayland, qemu-devel; +Cc: qemu-ppc, David Gibson On 10/12/20 11:23 AM, Mark Cave-Ayland wrote: > On 11/10/2020 20:03, Philippe Mathieu-Daudé wrote: > >> The Grackle PCI host model expects the interrupt controller >> being set, but does not verify it is present. Add a check to >> help developers using this model. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> hw/pci-host/grackle.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c >> index 57c29b20afb..20361d215ca 100644 >> --- a/hw/pci-host/grackle.c >> +++ b/hw/pci-host/grackle.c >> @@ -76,6 +76,10 @@ static void grackle_realize(DeviceState *dev, Error **errp) >> GrackleState *s = GRACKLE_PCI_HOST_BRIDGE(dev); >> PCIHostState *phb = PCI_HOST_BRIDGE(dev); >> >> + if (!s->pic) { >> + error_setg(errp, TYPE_GRACKLE_PCI_HOST_BRIDGE ": 'pic' link not set"); >> + return; >> + } >> phb->bus = pci_register_root_bus(dev, NULL, >> pci_grackle_set_irq, >> pci_grackle_map_irq, > > Reviewing this now, my feeling is that both grackle and uninorth are doing the wrong > thing here by passing in a link to the PIC - certainly if I had written this more > recently than I originally did, I would simply use qdev_init_gpio_out() for the IRQ > lines and do the wiring during machine init. > > I've got a few other things to look at first, but I'll post a patchset later which I > think will improve this for both Mac machines. Awesome, thanks! > > > ATB, > > Mark. > ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-10-21 10:22 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-11 19:03 [PATCH] hw/pci-host/grackle: Verify PIC link is properly set Philippe Mathieu-Daudé 2020-10-11 22:34 ` David Gibson 2020-10-12 6:21 ` Philippe Mathieu-Daudé 2020-10-12 6:54 ` David Gibson 2020-10-12 11:50 ` BALATON Zoltan via 2020-10-12 12:00 ` Philippe Mathieu-Daudé 2020-10-19 7:50 ` Markus Armbruster 2020-10-19 11:11 ` BALATON Zoltan via 2020-10-19 14:00 ` Markus Armbruster 2020-10-19 14:38 ` Mark Cave-Ayland 2020-10-19 16:17 ` BALATON Zoltan via 2020-10-20 5:30 ` Markus Armbruster 2020-10-20 11:37 ` BALATON Zoltan via 2020-10-21 3:31 ` Markus Armbruster 2020-10-21 10:21 ` BALATON Zoltan via 2020-10-12 9:23 ` Mark Cave-Ayland 2020-10-12 12:01 ` Philippe Mathieu-Daudé
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.