[PATCH] hw/pci-host/grackle: Verify PIC link is properly set

[PATCH] hw/pci-host/grackle: Verify PIC link is properly set

[Philippe Mathieu-Daudé]

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

Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set

[David Gibson]

[-- 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 --]

Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set

[Philippe Mathieu-Daudé]

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

Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set

[David Gibson]

[-- 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 --]

Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set

[BALATON Zoltan via]

[-- 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,
>>>
>>
>
>

Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set

[Philippe Mathieu-Daudé]

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

Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set

[Markus Armbruster]

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.

Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set

[BALATON Zoltan via]

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

Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set

[Markus Armbruster]

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.

Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set

[Mark Cave-Ayland]

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.

Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set

[BALATON Zoltan via]

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

Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set

[Markus Armbruster]

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.

[...]

Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set

[BALATON Zoltan via]

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

Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set

[Markus Armbruster]

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.

Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set

[BALATON Zoltan via]

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

Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set

[Mark Cave-Ayland]

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.

Re: [PATCH] hw/pci-host/grackle: Verify PIC link is properly set

[Philippe Mathieu-Daudé]

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