[Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices

Cornelia Huck posted 2 patches 6 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20171128134648.21530-1-cohuck@redhat.com
Test checkpatch passed
Test docker passed
Test ppc passed
Test s390x passed
hw/s390x/css-bridge.c      | 2 ++
hw/s390x/s390-virtio-ccw.c | 2 ++
2 files changed, 4 insertions(+)
[Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices
Posted by Cornelia Huck 6 years, 4 months ago
info qom-tree shows several devices under unattached that probably
should go somewhere.

The css bridge should attach to the machine, as it has a similar
purpose as e.g. a pci host bridge.

The autogenerated network devices should be in the same bucket as any
other device; I'm just not sure about the way I went about it.

The zpci devices are still problematic: I don't have a good idea where
they should show up.

Remaining in the unattached container are the sysbus, memory regions
and cpus.

Cornelia Huck (2):
  s390x/css: attach css bridge
  s390x: attach autogenerated nics

 hw/s390x/css-bridge.c      | 2 ++
 hw/s390x/s390-virtio-ccw.c | 2 ++
 2 files changed, 4 insertions(+)

-- 
2.13.6


Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices
Posted by Halil Pasic 6 years, 4 months ago

On 11/28/2017 02:46 PM, Cornelia Huck wrote:
> info qom-tree shows several devices under unattached that probably
> should go somewhere.

I think this was reported by me. See 
MID: <76f95c6f-641e-2fe0-73b4-3ab24fc1a93f@linux.vnet.ibm.com>

I would not mind a Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>.
Or do you see this differently?

If these are bugs. I would prefer commit messages and titles reflecting
this fact.

Otherwise at first glance both patches seem sane.

Regards,
Halil

> 
> The css bridge should attach to the machine, as it has a similar
> purpose as e.g. a pci host bridge.
> 
> The autogenerated network devices should be in the same bucket as any
> other device; I'm just not sure about the way I went about it.
> 
> The zpci devices are still problematic: I don't have a good idea where
> they should show up.
> 
> Remaining in the unattached container are the sysbus, memory regions
> and cpus.
> 
> Cornelia Huck (2):
>   s390x/css: attach css bridge
>   s390x: attach autogenerated nics
> 
>  hw/s390x/css-bridge.c      | 2 ++
>  hw/s390x/s390-virtio-ccw.c | 2 ++
>  2 files changed, 4 insertions(+)
> 


Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices
Posted by Cornelia Huck 6 years, 4 months ago
On Tue, 28 Nov 2017 15:17:38 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 11/28/2017 02:46 PM, Cornelia Huck wrote:
> > info qom-tree shows several devices under unattached that probably
> > should go somewhere.  
> 
> I think this was reported by me. See 
> MID: <76f95c6f-641e-2fe0-73b4-3ab24fc1a93f@linux.vnet.ibm.com>
> 
> I would not mind a Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>.
> Or do you see this differently?

Apparently I forgot to add it to the first one. Will do.

> 
> If these are bugs. I would prefer commit messages and titles reflecting
> this fact.

I don't see this as fixing bugs, more removing oddities.

> 
> Otherwise at first glance both patches seem sane.

Can I count this as an ack, or do you plan to do more review?

> 
> Regards,
> Halil
> 
> > 
> > The css bridge should attach to the machine, as it has a similar
> > purpose as e.g. a pci host bridge.
> > 
> > The autogenerated network devices should be in the same bucket as any
> > other device; I'm just not sure about the way I went about it.
> > 
> > The zpci devices are still problematic: I don't have a good idea where
> > they should show up.
> > 
> > Remaining in the unattached container are the sysbus, memory regions
> > and cpus.
> > 
> > Cornelia Huck (2):
> >   s390x/css: attach css bridge
> >   s390x: attach autogenerated nics
> > 
> >  hw/s390x/css-bridge.c      | 2 ++
> >  hw/s390x/s390-virtio-ccw.c | 2 ++
> >  2 files changed, 4 insertions(+)
> >   
> 


Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices
Posted by Halil Pasic 6 years, 4 months ago

On 11/28/2017 03:27 PM, Cornelia Huck wrote:
> On Tue, 28 Nov 2017 15:17:38 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 11/28/2017 02:46 PM, Cornelia Huck wrote:
>>> info qom-tree shows several devices under unattached that probably
>>> should go somewhere.  
>>
>> I think this was reported by me. See 
>> MID: <76f95c6f-641e-2fe0-73b4-3ab24fc1a93f@linux.vnet.ibm.com>
>>
>> I would not mind a Reported-by: Halil Pasic <pasic@linux.vnet.ibm.com>.
>> Or do you see this differently?
> 
> Apparently I forgot to add it to the first one. Will do.

np

It's just that I've spent quite some time writing that email and
identifying the oddities.

> 
>>
>> If these are bugs. I would prefer commit messages and titles reflecting
>> this fact.
> 
> I don't see this as fixing bugs, more removing oddities.
> 
>>
>> Otherwise at first glance both patches seem sane.
> 
> Can I count this as an ack, or do you plan to do more review?
> 

Yes I was planning to give it another look. And I do already
have questions. Isn't the QOM composition tree API? I mean
let's assume the QMP commands working on this tree are not completely
useless. How is client code (management software) supposed to work,
assumed it can rely on paths of e.g. properties being stable. Just
imagine we had this default-cssid property (for the sake of the
argument, not like we want it) on the css bridge.

Now if the composition tree is API then these can only be bug fixes
(IMHO).

There are also other oddities I've spotted. My idea was to put
this composition tree discussion on hold until the vfio-ccw stuff
is sorted out. I would certainly like to build a better understanding.

Halil

>>
>> Regards,
>> Halil
>>
>>>
>>> The css bridge should attach to the machine, as it has a similar
>>> purpose as e.g. a pci host bridge.
>>>
>>> The autogenerated network devices should be in the same bucket as any
>>> other device; I'm just not sure about the way I went about it.
>>>
>>> The zpci devices are still problematic: I don't have a good idea where
>>> they should show up.
>>>
>>> Remaining in the unattached container are the sysbus, memory regions
>>> and cpus.
>>>
>>> Cornelia Huck (2):
>>>   s390x/css: attach css bridge
>>>   s390x: attach autogenerated nics
>>>
>>>  hw/s390x/css-bridge.c      | 2 ++
>>>  hw/s390x/s390-virtio-ccw.c | 2 ++
>>>  2 files changed, 4 insertions(+)
>>>   
>>
> 
> 


Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices
Posted by Halil Pasic 6 years, 3 months ago

On 11/28/2017 04:21 PM, Halil Pasic wrote:
[..]
>>> Otherwise at first glance both patches seem sane.
>>
>> Can I count this as an ack, or do you plan to do more review?
>>
> 
> Yes I was planning to give it another look. And I do already
> have questions. Isn't the QOM composition tree API? I mean
> let's assume the QMP commands working on this tree are not completely
> useless. How is client code (management software) supposed to work,
> assumed it can rely on paths of e.g. properties being stable. Just
> imagine we had this default-cssid property (for the sake of the
> argument, not like we want it) on the css bridge.

Ping! I would like to get this clarified before proceeding with reviewing
this series.

> 
> Now if the composition tree is API then these can only be bug fixes
> (IMHO).
> 
> There are also other oddities I've spotted. My idea was to put
> this composition tree discussion on hold until the vfio-ccw stuff
> is sorted out. I would certainly like to build a better understanding.
> 
> Halil
> 

[..]


Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices
Posted by Cornelia Huck 6 years, 3 months ago
On Fri, 1 Dec 2017 15:41:21 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 11/28/2017 04:21 PM, Halil Pasic wrote:
> [..]
> >>> Otherwise at first glance both patches seem sane.  
> >>
> >> Can I count this as an ack, or do you plan to do more review?
> >>  
> > 
> > Yes I was planning to give it another look. And I do already
> > have questions. Isn't the QOM composition tree API? I mean
> > let's assume the QMP commands working on this tree are not completely
> > useless. How is client code (management software) supposed to work,
> > assumed it can rely on paths of e.g. properties being stable. Just
> > imagine we had this default-cssid property (for the sake of the
> > argument, not like we want it) on the css bridge.  
> 
> Ping! I would like to get this clarified before proceeding with reviewing
> this series.

[It might be helpful to not drop cc:s.]

I don't think we really want a static tree. As long as the devices are
locateable, it should be fine.

> 
> > 
> > Now if the composition tree is API then these can only be bug fixes
> > (IMHO).
> > 
> > There are also other oddities I've spotted. My idea was to put
> > this composition tree discussion on hold until the vfio-ccw stuff
> > is sorted out. I would certainly like to build a better understanding.
> > 
> > Halil
> >   
> 
> [..]
> 
> 


Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices
Posted by Halil Pasic 6 years, 3 months ago

On 12/04/2017 10:22 AM, Cornelia Huck wrote:
> On Fri, 1 Dec 2017 15:41:21 +0100
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 11/28/2017 04:21 PM, Halil Pasic wrote:
>> [..]
>>>>> Otherwise at first glance both patches seem sane.  
>>>>
>>>> Can I count this as an ack, or do you plan to do more review?
>>>>  
>>>
>>> Yes I was planning to give it another look. And I do already
>>> have questions. Isn't the QOM composition tree API? I mean
>>> let's assume the QMP commands working on this tree are not completely
>>> useless. How is client code (management software) supposed to work,
>>> assumed it can rely on paths of e.g. properties being stable. Just
>>> imagine we had this default-cssid property (for the sake of the
>>> argument, not like we want it) on the css bridge.  
>>
>> Ping! I would like to get this clarified before proceeding with reviewing
>> this series.
> 
> [It might be helpful to not drop cc:s.]
> 

Sorry. Wrong button.

> I don't think we really want a static tree. As long as the devices are
> locateable, it should be fine.
> 

What do you mean by locateable in particular?

Let's say I'm management software guy who want's to access a certain
property of a certain device. For that I'm supposed to use qom-get. Now
qom-get takes a path as input argument (absolute or relative). The question
is, how I'm supposed to figure out this path as management software developer?
In other words what is the algorithm?

One naive approach would be, to assume that the path is stable between
releases. So I have to figure it out when I'm implementing the stuff,
and then it keeps working ever after. I read your answer as this naive
approach is wrong.

(BTW I find static also confusing in this context.)

[..]


Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices
Posted by Cornelia Huck 6 years, 3 months ago
On Mon, 4 Dec 2017 15:47:37 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 12/04/2017 10:22 AM, Cornelia Huck wrote:
> > On Fri, 1 Dec 2017 15:41:21 +0100
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> On 11/28/2017 04:21 PM, Halil Pasic wrote:
> >> [..]  
> >>>>> Otherwise at first glance both patches seem sane.    
> >>>>
> >>>> Can I count this as an ack, or do you plan to do more review?
> >>>>    
> >>>
> >>> Yes I was planning to give it another look. And I do already
> >>> have questions. Isn't the QOM composition tree API? I mean
> >>> let's assume the QMP commands working on this tree are not completely
> >>> useless. How is client code (management software) supposed to work,
> >>> assumed it can rely on paths of e.g. properties being stable. Just
> >>> imagine we had this default-cssid property (for the sake of the
> >>> argument, not like we want it) on the css bridge.    
> >>
> >> Ping! I would like to get this clarified before proceeding with reviewing
> >> this series.  
> > 
> > [It might be helpful to not drop cc:s.]
> >   
> 
> Sorry. Wrong button.
> 
> > I don't think we really want a static tree. As long as the devices are
> > locateable, it should be fine.
> >   
> 
> What do you mean by locateable in particular?
> 
> Let's say I'm management software guy who want's to access a certain
> property of a certain device. For that I'm supposed to use qom-get. Now
> qom-get takes a path as input argument (absolute or relative). The question
> is, how I'm supposed to figure out this path as management software developer?
> In other words what is the algorithm?

I'd expect qom-tree to put out a path to the right object.

No idea if/how much management software relies on this. But hardcoded
paths sound fragile to me.

> 
> One naive approach would be, to assume that the path is stable between
> releases. So I have to figure it out when I'm implementing the stuff,
> and then it keeps working ever after. I read your answer as this naive
> approach is wrong.
> 
> (BTW I find static also confusing in this context.)
> 
> [..]
> 


Re: [Qemu-devel] [qemu-s390x] [PATCH RFC 0/2] s390x: cut down on unattached devices
Posted by David Hildenbrand 6 years, 3 months ago
On 28.11.2017 14:46, Cornelia Huck wrote:
> info qom-tree shows several devices under unattached that probably
> should go somewhere.
> 
> The css bridge should attach to the machine, as it has a similar
> purpose as e.g. a pci host bridge.
> 
> The autogenerated network devices should be in the same bucket as any
> other device; I'm just not sure about the way I went about it.
> 
> The zpci devices are still problematic: I don't have a good idea where
> they should show up.
> 
> Remaining in the unattached container are the sysbus, memory regions
> and cpus.
> 
> Cornelia Huck (2):
>   s390x/css: attach css bridge
>   s390x: attach autogenerated nics
> 
>  hw/s390x/css-bridge.c      | 2 ++
>  hw/s390x/s390-virtio-ccw.c | 2 ++
>  2 files changed, 4 insertions(+)
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

My guess is, that this should not break migration (or anything else).

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices
Posted by Bjoern Walk 6 years, 3 months ago
Cornelia Huck <cohuck@redhat.com> [2017-11-28, 02:46PM +0100]:
> info qom-tree shows several devices under unattached that probably
> should go somewhere.
> 
> The css bridge should attach to the machine, as it has a similar
> purpose as e.g. a pci host bridge.
> 
> The autogenerated network devices should be in the same bucket as any
> other device; I'm just not sure about the way I went about it.
> 
> The zpci devices are still problematic: I don't have a good idea where
> they should show up.
> 
> Remaining in the unattached container are the sysbus, memory regions
> and cpus.
> 
> Cornelia Huck (2):
>   s390x/css: attach css bridge
>   s390x: attach autogenerated nics
> 
>  hw/s390x/css-bridge.c      | 2 ++
>  hw/s390x/s390-virtio-ccw.c | 2 ++
>  2 files changed, 4 insertions(+)
> 
> -- 
> 2.13.6
> 
> 

Regarding the discussion about whether the QOM tree is API and what
exploiters like libvirt should do, Halil asked me to chip in.

This patch is fine from libvirt perspective. I did a quick smoke test
and you can have a

    Tested-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>

for what it's worth.

In general, I kind of agree with Halil. Unless somewhere in QEMU it is
documented that the QOM tree is not guaranteed to be stable for
exploiters, I'd consider is part of the API. libvirt does use at least
some hardcoded paths, most of the time for CPUs in /machine/unattached,
so if that relation would change, things break. However, there is also
code to traverse the QOM tree recursively and find a path for a given
type(?) name. If this is the preferred way, we probably should change
this in libvirt to be safe.
Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices
Posted by Cornelia Huck 6 years, 3 months ago
On Tue, 5 Dec 2017 09:59:06 +0100
Bjoern Walk <bwalk@linux.vnet.ibm.com> wrote:

> Cornelia Huck <cohuck@redhat.com> [2017-11-28, 02:46PM +0100]:
> > info qom-tree shows several devices under unattached that probably
> > should go somewhere.
> > 
> > The css bridge should attach to the machine, as it has a similar
> > purpose as e.g. a pci host bridge.
> > 
> > The autogenerated network devices should be in the same bucket as any
> > other device; I'm just not sure about the way I went about it.
> > 
> > The zpci devices are still problematic: I don't have a good idea where
> > they should show up.
> > 
> > Remaining in the unattached container are the sysbus, memory regions
> > and cpus.
> > 
> > Cornelia Huck (2):
> >   s390x/css: attach css bridge
> >   s390x: attach autogenerated nics
> > 
> >  hw/s390x/css-bridge.c      | 2 ++
> >  hw/s390x/s390-virtio-ccw.c | 2 ++
> >  2 files changed, 4 insertions(+)
> > 
> > -- 
> > 2.13.6
> > 
> >   
> 
> Regarding the discussion about whether the QOM tree is API and what
> exploiters like libvirt should do, Halil asked me to chip in.
> 
> This patch is fine from libvirt perspective. I did a quick smoke test
> and you can have a
> 
>     Tested-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> 
> for what it's worth.

Thanks for checking.

> 
> In general, I kind of agree with Halil. Unless somewhere in QEMU it is
> documented that the QOM tree is not guaranteed to be stable for
> exploiters, I'd consider is part of the API. libvirt does use at least
> some hardcoded paths, most of the time for CPUs in /machine/unattached,
> so if that relation would change, things break. However, there is also
> code to traverse the QOM tree recursively and find a path for a given
> type(?) name. If this is the preferred way, we probably should change
> this in libvirt to be safe.

OK, with that in mind and as we're now adding a property to check on
the css bridge, I vote for including patch 1 now (having a fixed
location under /machine looks saner that having to
check /machine/unattached/device[<n>], which might not be stable).

Patch 2 needs more discussion, as I'm not sure whether what I'm doing
is the correct way to go about this (and other machines are in the same
situation). Not sure whether it is worth trying to attach the zpci
devices somewhere.

Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices
Posted by Halil Pasic 6 years, 3 months ago

On 12/07/2017 05:34 PM, Cornelia Huck wrote:
> On Tue, 5 Dec 2017 09:59:06 +0100
> Bjoern Walk <bwalk@linux.vnet.ibm.com> wrote:
> 
>> Cornelia Huck <cohuck@redhat.com> [2017-11-28, 02:46PM +0100]:
>>> info qom-tree shows several devices under unattached that probably
>>> should go somewhere.
>>>
>>> The css bridge should attach to the machine, as it has a similar
>>> purpose as e.g. a pci host bridge.
>>>
>>> The autogenerated network devices should be in the same bucket as any
>>> other device; I'm just not sure about the way I went about it.
>>>
>>> The zpci devices are still problematic: I don't have a good idea where
>>> they should show up.
>>>
>>> Remaining in the unattached container are the sysbus, memory regions
>>> and cpus.
>>>
>>> Cornelia Huck (2):
>>>   s390x/css: attach css bridge
>>>   s390x: attach autogenerated nics
>>>
>>>  hw/s390x/css-bridge.c      | 2 ++
>>>  hw/s390x/s390-virtio-ccw.c | 2 ++
>>>  2 files changed, 4 insertions(+)
>>>
>>> -- 
>>> 2.13.6
>>>
>>>   
>>
>> Regarding the discussion about whether the QOM tree is API and what
>> exploiters like libvirt should do, Halil asked me to chip in.
>>
>> This patch is fine from libvirt perspective. I did a quick smoke test
>> and you can have a
>>
>>     Tested-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
>>
>> for what it's worth.
> 
> Thanks for checking.
> 
>>
>> In general, I kind of agree with Halil. Unless somewhere in QEMU it is
>> documented that the QOM tree is not guaranteed to be stable for
>> exploiters, I'd consider is part of the API. libvirt does use at least
>> some hardcoded paths, most of the time for CPUs in /machine/unattached,
>> so if that relation would change, things break. However, there is also
>> code to traverse the QOM tree recursively and find a path for a given
>> type(?) name. If this is the preferred way, we probably should change
>> this in libvirt to be safe.
> 
> OK, with that in mind and as we're now adding a property to check on
> the css bridge, I vote for including patch 1 now (having a fixed
> location under /machine looks saner that having to
> check /machine/unattached/device[<n>], which might not be stable).
> 
> Patch 2 needs more discussion, as I'm not sure whether what I'm doing
> is the correct way to go about this (and other machines are in the same
> situation). Not sure whether it is worth trying to attach the zpci
> devices somewhere.
> 

I think, if it's kind of API, then fixing sooner is better than fixing
later.

I also agree that patch 1 should be higher priority.

Before we do patch 1 I would like having agreed and documented whether
this is API or not.

If we decide it's an API, I think we should consider deprecating
the current interface, but keep it working for two releases or
so. I think nothing speaks against introducing a link form unattached
in patch 1 (but I have not tried yet).

Halil


Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices
Posted by Cornelia Huck 6 years, 3 months ago
On Thu, 7 Dec 2017 18:01:46 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 12/07/2017 05:34 PM, Cornelia Huck wrote:
> > On Tue, 5 Dec 2017 09:59:06 +0100
> > Bjoern Walk <bwalk@linux.vnet.ibm.com> wrote:
> >   
> >> Cornelia Huck <cohuck@redhat.com> [2017-11-28, 02:46PM +0100]:  
> >>> info qom-tree shows several devices under unattached that probably
> >>> should go somewhere.
> >>>
> >>> The css bridge should attach to the machine, as it has a similar
> >>> purpose as e.g. a pci host bridge.
> >>>
> >>> The autogenerated network devices should be in the same bucket as any
> >>> other device; I'm just not sure about the way I went about it.
> >>>
> >>> The zpci devices are still problematic: I don't have a good idea where
> >>> they should show up.
> >>>
> >>> Remaining in the unattached container are the sysbus, memory regions
> >>> and cpus.
> >>>
> >>> Cornelia Huck (2):
> >>>   s390x/css: attach css bridge
> >>>   s390x: attach autogenerated nics
> >>>
> >>>  hw/s390x/css-bridge.c      | 2 ++
> >>>  hw/s390x/s390-virtio-ccw.c | 2 ++
> >>>  2 files changed, 4 insertions(+)
> >>>
> >>> -- 
> >>> 2.13.6
> >>>
> >>>     
> >>
> >> Regarding the discussion about whether the QOM tree is API and what
> >> exploiters like libvirt should do, Halil asked me to chip in.
> >>
> >> This patch is fine from libvirt perspective. I did a quick smoke test
> >> and you can have a
> >>
> >>     Tested-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> >>
> >> for what it's worth.  
> > 
> > Thanks for checking.
> >   
> >>
> >> In general, I kind of agree with Halil. Unless somewhere in QEMU it is
> >> documented that the QOM tree is not guaranteed to be stable for
> >> exploiters, I'd consider is part of the API. libvirt does use at least
> >> some hardcoded paths, most of the time for CPUs in /machine/unattached,
> >> so if that relation would change, things break. However, there is also
> >> code to traverse the QOM tree recursively and find a path for a given
> >> type(?) name. If this is the preferred way, we probably should change
> >> this in libvirt to be safe.  
> > 
> > OK, with that in mind and as we're now adding a property to check on
> > the css bridge, I vote for including patch 1 now (having a fixed
> > location under /machine looks saner that having to
> > check /machine/unattached/device[<n>], which might not be stable).
> > 
> > Patch 2 needs more discussion, as I'm not sure whether what I'm doing
> > is the correct way to go about this (and other machines are in the same
> > situation). Not sure whether it is worth trying to attach the zpci
> > devices somewhere.
> >   
> 
> I think, if it's kind of API, then fixing sooner is better than fixing
> later.
> 
> I also agree that patch 1 should be higher priority.
> 
> Before we do patch 1 I would like having agreed and documented whether
> this is API or not.
> 
> If we decide it's an API, I think we should consider deprecating
> the current interface, but keep it working for two releases or
> so. I think nothing speaks against introducing a link form unattached
> in patch 1 (but I have not tried yet).

No, just no. That's completely overengineered.

Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices
Posted by Halil Pasic 6 years, 3 months ago

On 12/07/2017 06:06 PM, Cornelia Huck wrote:
> On Thu, 7 Dec 2017 18:01:46 +0100
[..]
>>>> Regarding the discussion about whether the QOM tree is API and what
>>>> exploiters like libvirt should do, Halil asked me to chip in.
>>>>
>>>> This patch is fine from libvirt perspective. I did a quick smoke test
>>>> and you can have a
>>>>
>>>>     Tested-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
>>>>
>>>> for what it's worth.  
>>>
>>> Thanks for checking.
>>>   
>>>>
>>>> In general, I kind of agree with Halil. Unless somewhere in QEMU it is
>>>> documented that the QOM tree is not guaranteed to be stable for
>>>> exploiters, I'd consider is part of the API. libvirt does use at least
>>>> some hardcoded paths, most of the time for CPUs in /machine/unattached,
>>>> so if that relation would change, things break. However, there is also
>>>> code to traverse the QOM tree recursively and find a path for a given
>>>> type(?) name. If this is the preferred way, we probably should change
>>>> this in libvirt to be safe.  
>>>
>>> OK, with that in mind and as we're now adding a property to check on
>>> the css bridge, I vote for including patch 1 now (having a fixed
>>> location under /machine looks saner that having to
>>> check /machine/unattached/device[<n>], which might not be stable).
>>>
>>> Patch 2 needs more discussion, as I'm not sure whether what I'm doing
>>> is the correct way to go about this (and other machines are in the same
>>> situation). Not sure whether it is worth trying to attach the zpci
>>> devices somewhere.
>>>   
>>
>> I think, if it's kind of API, then fixing sooner is better than fixing
>> later.
>>
>> I also agree that patch 1 should be higher priority.
>>
>> Before we do patch 1 I would like having agreed and documented whether
>> this is API or not.
>>
>> If we decide it's an API, I think we should consider deprecating
>> the current interface, but keep it working for two releases or
>> so. I think nothing speaks against introducing a link form unattached
>> in patch 1 (but I have not tried yet).
> 
> No, just no. That's completely overengineered.
> 

Which part is totally overengineered? Having it clear what is API and
what not? Having this documented? Or caring about our deprecation
policy (if it's API)?


Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices
Posted by Cornelia Huck 6 years, 3 months ago
On Thu, 7 Dec 2017 18:15:57 +0100
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 12/07/2017 06:06 PM, Cornelia Huck wrote:
> > On Thu, 7 Dec 2017 18:01:46 +0100  
> [..]
> >>>> Regarding the discussion about whether the QOM tree is API and what
> >>>> exploiters like libvirt should do, Halil asked me to chip in.
> >>>>
> >>>> This patch is fine from libvirt perspective. I did a quick smoke test
> >>>> and you can have a
> >>>>
> >>>>     Tested-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
> >>>>
> >>>> for what it's worth.    
> >>>
> >>> Thanks for checking.
> >>>     
> >>>>
> >>>> In general, I kind of agree with Halil. Unless somewhere in QEMU it is
> >>>> documented that the QOM tree is not guaranteed to be stable for
> >>>> exploiters, I'd consider is part of the API. libvirt does use at least
> >>>> some hardcoded paths, most of the time for CPUs in /machine/unattached,
> >>>> so if that relation would change, things break. However, there is also
> >>>> code to traverse the QOM tree recursively and find a path for a given
> >>>> type(?) name. If this is the preferred way, we probably should change
> >>>> this in libvirt to be safe.    
> >>>
> >>> OK, with that in mind and as we're now adding a property to check on
> >>> the css bridge, I vote for including patch 1 now (having a fixed
> >>> location under /machine looks saner that having to
> >>> check /machine/unattached/device[<n>], which might not be stable).
> >>>
> >>> Patch 2 needs more discussion, as I'm not sure whether what I'm doing
> >>> is the correct way to go about this (and other machines are in the same
> >>> situation). Not sure whether it is worth trying to attach the zpci
> >>> devices somewhere.
> >>>     
> >>
> >> I think, if it's kind of API, then fixing sooner is better than fixing
> >> later.
> >>
> >> I also agree that patch 1 should be higher priority.
> >>
> >> Before we do patch 1 I would like having agreed and documented whether
> >> this is API or not.
> >>
> >> If we decide it's an API, I think we should consider deprecating
> >> the current interface, but keep it working for two releases or
> >> so. I think nothing speaks against introducing a link form unattached
> >> in patch 1 (but I have not tried yet).  
> > 
> > No, just no. That's completely overengineered.
> >   
> 
> Which part is totally overengineered? Having it clear what is API and
> what not? Having this documented? Or caring about our deprecation
> policy (if it's API)?
> 

You're building a monster to fix a non-existing problem. I will not go
down that rabbit hole any further, and just apply patch 1.

Re: [Qemu-devel] [PATCH RFC 0/2] s390x: cut down on unattached devices
Posted by Halil Pasic 6 years, 3 months ago

On 12/08/2017 12:42 PM, Cornelia Huck wrote:
[..]
>>>>>> In general, I kind of agree with Halil. Unless somewhere in QEMU it is
>>>>>> documented that the QOM tree is not guaranteed to be stable for
>>>>>> exploiters, I'd consider is part of the API. libvirt does use at least
>>>>>> some hardcoded paths, most of the time for CPUs in /machine/unattached,
>>>>>> so if that relation would change, things break. However, there is also
>>>>>> code to traverse the QOM tree recursively and find a path for a given
>>>>>> type(?) name. If this is the preferred way, we probably should change
>>>>>> this in libvirt to be safe.    
>>>>>
>>>>> OK, with that in mind and as we're now adding a property to check on
>>>>> the css bridge, I vote for including patch 1 now (having a fixed
>>>>> location under /machine looks saner that having to
>>>>> check /machine/unattached/device[<n>], which might not be stable).
>>>>>
>>>>> Patch 2 needs more discussion, as I'm not sure whether what I'm doing
>>>>> is the correct way to go about this (and other machines are in the same
>>>>> situation). Not sure whether it is worth trying to attach the zpci
>>>>> devices somewhere.
>>>>>     
>>>>
>>>> I think, if it's kind of API, then fixing sooner is better than fixing
>>>> later.
>>>>
>>>> I also agree that patch 1 should be higher priority.
>>>>
>>>> Before we do patch 1 I would like having agreed and documented whether
>>>> this is API or not.
>>>>
>>>> If we decide it's an API, I think we should consider deprecating
>>>> the current interface, but keep it working for two releases or
>>>> so. I think nothing speaks against introducing a link form unattached
>>>> in patch 1 (but I have not tried yet).  
>>>
>>> No, just no. That's completely overengineered.
>>>   
>>
>> Which part is totally overengineered? Having it clear what is API and
>> what not? Having this documented? Or caring about our deprecation
>> policy (if it's API)?
>>
> 
> You're building a monster to fix a non-existing problem. I will not go
> down that rabbit hole any further, and just apply patch 1.
> 

I'm not building anything. I've basically just asked a simple question:
Are  paths in the qom composition tree external API or not (and if not,
what is the canonical way to accomplish certain things)? Then I though
out loud about the branches we can take based on the answer.

Based on the answers I got it seems I'm not particularly good at asking
questions. Sorry about that.

Halil