Add zPCI definitions in preparation of extending the PCI address
with parameters uid (user-defined identifier) and fid (PCI function
identifier).

Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
---
 src/util/virpci.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/util/virpci.h b/src/util/virpci.h
index 2ac87694df..b137eb01e6 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -37,12 +37,22 @@ typedef virPCIDeviceAddress *virPCIDeviceAddressPtr;
 typedef struct _virPCIDeviceList virPCIDeviceList;
 typedef virPCIDeviceList *virPCIDeviceListPtr;
 
+typedef struct _virZPCIDeviceAddress virZPCIDeviceAddress;
+typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr;
+struct _virZPCIDeviceAddress {
+    unsigned int zpci_fid;
+    unsigned int zpci_uid;
+    bool fid_assigned;
+    bool uid_assigned;
+};
+
 struct _virPCIDeviceAddress {
     unsigned int domain;
     unsigned int bus;
     unsigned int slot;
     unsigned int function;
     int multi; /* virTristateSwitch */
+    virZPCIDeviceAddressPtr zpci;
 };
 
 typedef enum {
-- 
Yi Min

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
> +typedef struct _virZPCIDeviceAddress virZPCIDeviceAddress;
> +typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr;
> +struct _virZPCIDeviceAddress {
> +    unsigned int zpci_fid;
> +    unsigned int zpci_uid;
> +    bool fid_assigned;
> +    bool uid_assigned;
> +};

A couple of questions about the approach here, one of which I have
mentioned already and one of which I probably haven't (my bad):

  * do you really need to have separate booleans tracking whether
    or not either id has been assigned? Wouldn't the same approach
    as virPCIDeviceAddressIsEmpty() work, eg. consider the address
    absent if both are zero and present otherwise?

  * especially if you don't need the additional booleans, would it
    be preferable to just add the two ids to the existing struct
    instead of declaring a new one that you'll have to allocate
    and make sure it's not NULL before accessing it? Again, I seem
    to remember Laine feeling somewhat strongly about the topic.

Cosmetic issues:

  * uid should be listed before fid;

  * the zpci_ prefix is unnecessary if you have a separate struct
    that contains "ZPCI" in the name. But see above :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

在 2018/8/16 下午10:38, Andrea Bolognani 写道:
> On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
>> +typedef struct _virZPCIDeviceAddress virZPCIDeviceAddress;
>> +typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr;
>> +struct _virZPCIDeviceAddress {
>> +    unsigned int zpci_fid;
>> +    unsigned int zpci_uid;
>> +    bool fid_assigned;
>> +    bool uid_assigned;
>> +};
> A couple of questions about the approach here, one of which I have
> mentioned already and one of which I probably haven't (my bad):
>
>    * do you really need to have separate booleans tracking whether
>      or not either id has been assigned? Wouldn't the same approach
>      as virPCIDeviceAddressIsEmpty() work, eg. consider the address
>      absent if both are zero and present otherwise?
It's OK for uid. But for fid, zero is a valid value, so we need a bool 
to track its assignment.
If we add a boolean for fid, why not also add another one for uid to 
keep consistency?
This also makes code easy to read and obvious. It doesn't waste much 
memory space.
>
>    * especially if you don't need the additional booleans, would it
>      be preferable to just add the two ids to the existing struct
>      instead of declaring a new one that you'll have to allocate
>      and make sure it's not NULL before accessing it? Again, I seem
>      to remember Laine feeling somewhat strongly about the topic.
For other platforms, is it OK to waste this unused memory (of course, 
it's little) ?
>
> Cosmetic issues:
>
>    * uid should be listed before fid;
>
>    * the zpci_ prefix is unnecessary if you have a separate struct
>      that contains "ZPCI" in the name. But see above :)
>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, 2018-08-20 at 16:19 +0800, Yi Min Zhao wrote:
> 在 2018/8/16 下午10:38, Andrea Bolognani 写道:
> > On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
> > > +struct _virZPCIDeviceAddress {
> > > +    unsigned int zpci_fid;
> > > +    unsigned int zpci_uid;
> > > +    bool fid_assigned;
> > > +    bool uid_assigned;
> > > +};
> > 
> > A couple of questions about the approach here, one of which I have
> > mentioned already and one of which I probably haven't (my bad):
> > 
> >    * do you really need to have separate booleans tracking whether
> >      or not either id has been assigned? Wouldn't the same approach
> >      as virPCIDeviceAddressIsEmpty() work, eg. consider the address
> >      absent if both are zero and present otherwise?
> 
> It's OK for uid. But for fid, zero is a valid value, so we need a bool 
> to track its assignment.

See virPCIDeviceAddressIsEmpty() and virPCIDeviceAddressIsValid(),
which have very similar requirement but don't use extra booleans
to keep track of state.

You could do the same thing those functions do:

  * the zPCI address is empty if both uid and fid are zero;
  * the zPCI address is invalid if it's empty or uid is too large.

You already have the latter covered, so it's only a matter of
implementing the former.

> If we add a boolean for fid, why not also add another one for uid to 
> keep consistency?
> This also makes code easy to read and obvious. It doesn't waste much 
> memory space.

I think it actually makes more complicated, which is why I'd rather
get rid of it :)

> >    * especially if you don't need the additional booleans, would it
> >      be preferable to just add the two ids to the existing struct
> >      instead of declaring a new one that you'll have to allocate
> >      and make sure it's not NULL before accessing it? Again, I seem
> >      to remember Laine feeling somewhat strongly about the topic.
> 
> For other platforms, is it OK to waste this unused memory (of course, 
> it's little) ?

I believe adding a couple of unsigned ints is worth it if we can
get rid of all the checks on addr->zpci because of it.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

在 2018/8/20 下午6:35, Andrea Bolognani 写道:
> On Mon, 2018-08-20 at 16:19 +0800, Yi Min Zhao wrote:
>> 在 2018/8/16 下午10:38, Andrea Bolognani 写道:
>>> On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
>>>> +struct _virZPCIDeviceAddress {
>>>> +    unsigned int zpci_fid;
>>>> +    unsigned int zpci_uid;
>>>> +    bool fid_assigned;
>>>> +    bool uid_assigned;
>>>> +};
>>> A couple of questions about the approach here, one of which I have
>>> mentioned already and one of which I probably haven't (my bad):
>>>
>>>     * do you really need to have separate booleans tracking whether
>>>       or not either id has been assigned? Wouldn't the same approach
>>>       as virPCIDeviceAddressIsEmpty() work, eg. consider the address
>>>       absent if both are zero and present otherwise?
>> It's OK for uid. But for fid, zero is a valid value, so we need a bool
>> to track its assignment.
> See virPCIDeviceAddressIsEmpty() and virPCIDeviceAddressIsValid(),
> which have very similar requirement but don't use extra booleans
> to keep track of state.
>
> You could do the same thing those functions do:
>
>    * the zPCI address is empty if both uid and fid are zero;
uid=0 and fid=0 can't mean zPCI address is empty, because the user might
only define fid with 0. If fid=0 has been assigned, we should report 
error. If
it is not defined by user, fid is also 0, then we should allocate a 
valid value
for fid.
>    * the zPCI address is invalid if it's empty or uid is too large.
>
> You already have the latter covered, so it's only a matter of
> implementing the former.
>
>> If we add a boolean for fid, why not also add another one for uid to
>> keep consistency?
>> This also makes code easy to read and obvious. It doesn't waste much
>> memory space.
> I think it actually makes more complicated, which is why I'd rather
> get rid of it :)
>
>>>     * especially if you don't need the additional booleans, would it
>>>       be preferable to just add the two ids to the existing struct
>>>       instead of declaring a new one that you'll have to allocate
>>>       and make sure it's not NULL before accessing it? Again, I seem
>>>       to remember Laine feeling somewhat strongly about the topic.
>> For other platforms, is it OK to waste this unused memory (of course,
>> it's little) ?
> I believe adding a couple of unsigned ints is worth it if we can
> get rid of all the checks on addr->zpci because of it.
>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, 2018-08-21 at 11:24 +0800, Yi Min Zhao wrote:
> 在 2018/8/20 下午6:35, Andrea Bolognani 写道:
> > You could do the same thing those functions do:
> > 
> >    * the zPCI address is empty if both uid and fid are zero;
> 
> uid=0 and fid=0 can't mean zPCI address is empty, because the user might
> only define fid with 0. If fid=0 has been assigned, we should report 
> error. If
> it is not defined by user, fid is also 0, then we should allocate a 
> valid value
> for fid.

For the PCI part

  <address type='pci' domain='0x0000' bus='0x00'
                      slot='0x00' function='0x0'/>

behaves the same as

  <address type='pci'/>

and results in a PCI address being allocated automatically by
libvirt rather than an error being reported.

As with zPCI, zero is a valid value for some, but not all, of the
attributes: validation is performed if at least one of them is
non-zero and might result in an error being reported.

Doing the same with the zPCI part would not only allow you to get
rid of the extra booleans but would also guarantee a consistent
behavior, which is a worthy goal in and of itself.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

在 2018/8/21 下午4:06, Andrea Bolognani 写道:
> On Tue, 2018-08-21 at 11:24 +0800, Yi Min Zhao wrote:
>> 在 2018/8/20 下午6:35, Andrea Bolognani 写道:
>>> You could do the same thing those functions do:
>>>
>>>     * the zPCI address is empty if both uid and fid are zero;
>> uid=0 and fid=0 can't mean zPCI address is empty, because the user might
>> only define fid with 0. If fid=0 has been assigned, we should report
>> error. If
>> it is not defined by user, fid is also 0, then we should allocate a
>> valid value
>> for fid.
> For the PCI part
>
>    <address type='pci' domain='0x0000' bus='0x00'
>                        slot='0x00' function='0x0'/>
>
> behaves the same as
>
>    <address type='pci'/>
>
> and results in a PCI address being allocated automatically by
> libvirt rather than an error being reported.
>
> As with zPCI, zero is a valid value for some, but not all, of the
> attributes: validation is performed if at least one of them is
> non-zero and might result in an error being reported.
>
> Doing the same with the zPCI part would not only allow you to get
> rid of the extra booleans but would also guarantee a consistent
> behavior, which is a worthy goal in and of itself.
>
I want to ask a question. For pci address, any pci device can't use slot 0.
Is that a reason why PCI part could treat all zeros as empty address?

For zPCI address, if we use the same strategy as PCI part and user
wants to assign fid=0 to the specific device, he might encounter
assignment failure. At least, according to the doc, 0 shoud be a valid
value to assign to fid.

Anyway, when I wrote this code, I also wanted to use the similar logic to
check if zPCI address is empty.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, 2018-08-21 at 17:35 +0800, Yi Min Zhao wrote:
> I want to ask a question. For pci address, any pci device can't use slot 0.
> Is that a reason why PCI part could treat all zeros as empty address?

A PCI address where all attributes are zero can't be used, so
there's no ambiguity there; same for a zPCI address where all
attributes are zero, which also can't be used.

> For zPCI address, if we use the same strategy as PCI part and user
> wants to assign fid=0 to the specific device, he might encounter
> assignment failure. At least, according to the doc, 0 shoud be a valid
> value to assign to fid.

If the user wants to use a specific zPCI address they can simply
specify both attributes, eg. uid=1,fid=0 will work just fine with
the proposed approach and won't produce errors or cause a new zPCI
address to be automatically assigned.

If the user only specified fid=0 while leaving uid unspecified,
well, an address is going to be picked for them. This is consistent
with how PCI addresses are treated so it shouldn't be a problem: if
anything, *deviating* from this behavior would cause confusion.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

在 2018/8/21 下午7:00, Andrea Bolognani 写道:
> On Tue, 2018-08-21 at 17:35 +0800, Yi Min Zhao wrote:
>> I want to ask a question. For pci address, any pci device can't use slot 0.
>> Is that a reason why PCI part could treat all zeros as empty address?
> A PCI address where all attributes are zero can't be used, so
> there's no ambiguity there; same for a zPCI address where all
> attributes are zero, which also can't be used.
Yes, uid must be non-zero value. But the user could only define fid like
you mentioned below. Validation check happens during parsing xml.
So if the user only defines fid=0, zpci address (fid=0, uid=0 although the
user doesn't specify uid value) is going to be invalid as PCI address
check logic. UID and FID must be unique separately, but we can't treat
them as a combination. This doesn't like PCI 
address(domain:bus:slot:function).
>
>> For zPCI address, if we use the same strategy as PCI part and user
>> wants to assign fid=0 to the specific device, he might encounter
>> assignment failure. At least, according to the doc, 0 shoud be a valid
>> value to assign to fid.
> If the user wants to use a specific zPCI address they can simply
> specify both attributes, eg. uid=1,fid=0 will work just fine with
> the proposed approach and won't produce errors or cause a new zPCI
> address to be automatically assigned.
>
> If the user only specified fid=0 while leaving uid unspecified,
> well, an address is going to be picked for them. This is consistent
This would be an empty zpci address while parsing XML so that we might
pick a zpci address with non-zero fid. For example:

dev1: fid=0 (this address would be treated as an unassigned zpci address)
dev2 (no definition for both fid and uid)

Then the process of assigning addresses will iterate devices as type by 
type.
If dev2 is processed before dev1, dev2's fid would be assigned by 0. 
This causes
dev1's fid=1. The result doesn't match what the user wants.
> with how PCI addresses are treated so it shouldn't be a problem: if
> anything, *deviating* from this behavior would cause confusion.
>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, 2018-08-21 at 19:55 +0800, Yi Min Zhao wrote:
> 在 2018/8/21 下午7:00, Andrea Bolognani 写道:
> > On Tue, 2018-08-21 at 17:35 +0800, Yi Min Zhao wrote:
> > > I want to ask a question. For pci address, any pci device can't use slot 0.
> > > Is that a reason why PCI part could treat all zeros as empty address?
> > 
> > A PCI address where all attributes are zero can't be used, so
> > there's no ambiguity there; same for a zPCI address where all
> > attributes are zero, which also can't be used.
> 
> Yes, uid must be non-zero value. But the user could only define fid like
> you mentioned below. Validation check happens during parsing xml.
> So if the user only defines fid=0, zpci address (fid=0, uid=0 although the
> user doesn't specify uid value) is going to be invalid as PCI address
> check logic.

In the context of PCI addresses, empty implies invalid but during
XML parsing the address is only validated if it's not empty. If
that was not the case, an empty address would never have a chance
to be automatically filled in by libvirt :)

> UID and FID must be unique separately, but we can't treat
> them as a combination. This doesn't like PCI
> address(domain:bus:slot:function).

I asked a question about this in reply to patch 09/12 yesterday
by the way, and it would be great if you could answer it.

> > > For zPCI address, if we use the same strategy as PCI part and user
> > > wants to assign fid=0 to the specific device, he might encounter
> > > assignment failure. At least, according to the doc, 0 shoud be a valid
> > > value to assign to fid.
> > 
> > If the user wants to use a specific zPCI address they can simply
> > specify both attributes, eg. uid=1,fid=0 will work just fine with
> > the proposed approach and won't produce errors or cause a new zPCI
> > address to be automatically assigned.
> > 
> > If the user only specified fid=0 while leaving uid unspecified,
> > well, an address is going to be picked for them.
> 
> This would be an empty zpci address while parsing XML so that we might
> pick a zpci address with non-zero fid. For example:
> 
> dev1: fid=0 (this address would be treated as an unassigned zpci address)
> dev2 (no definition for both fid and uid)
> 
> Then the process of assigning addresses will iterate devices as type by 
> type.
> If dev2 is processed before dev1, dev2's fid would be assigned by 0. 
> This causes
> dev1's fid=1. The result doesn't match what the user wants.

This would be the same as specifying a partial PCI address such as

  <address type='pci' slot='0x00'/>

and getting an address with slot != 0x00 back: surprising, perhaps,
but that's the way it's been with PCI addresses forever so you can
assume users are familiar with it by now.

For zPCI addresses to be inconsistent with this behavior would be
utterly confusing; moreover, if the user really needs the uid and
fid to have certain values, all they have to do is specify both
and libvirt will not attempt to override them.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

在 2018/8/21 下午9:19, Andrea Bolognani 写道:
> On Tue, 2018-08-21 at 19:55 +0800, Yi Min Zhao wrote:
>> 在 2018/8/21 下午7:00, Andrea Bolognani 写道:
>>> On Tue, 2018-08-21 at 17:35 +0800, Yi Min Zhao wrote:
>>>> I want to ask a question. For pci address, any pci device can't use slot 0.
>>>> Is that a reason why PCI part could treat all zeros as empty address?
>>> A PCI address where all attributes are zero can't be used, so
>>> there's no ambiguity there; same for a zPCI address where all
>>> attributes are zero, which also can't be used.
>> Yes, uid must be non-zero value. But the user could only define fid like
>> you mentioned below. Validation check happens during parsing xml.
>> So if the user only defines fid=0, zpci address (fid=0, uid=0 although the
>> user doesn't specify uid value) is going to be invalid as PCI address
>> check logic.
> In the context of PCI addresses, empty implies invalid but during
> XML parsing the address is only validated if it's not empty. If
> that was not the case, an empty address would never have a chance
> to be automatically filled in by libvirt :)
>
>> UID and FID must be unique separately, but we can't treat
>> them as a combination. This doesn't like PCI
>> address(domain:bus:slot:function).
> I asked a question about this in reply to patch 09/12 yesterday
> by the way, and it would be great if you could answer it.
I answered you in the morning.
>
>>>> For zPCI address, if we use the same strategy as PCI part and user
>>>> wants to assign fid=0 to the specific device, he might encounter
>>>> assignment failure. At least, according to the doc, 0 shoud be a valid
>>>> value to assign to fid.
>>> If the user wants to use a specific zPCI address they can simply
>>> specify both attributes, eg. uid=1,fid=0 will work just fine with
>>> the proposed approach and won't produce errors or cause a new zPCI
>>> address to be automatically assigned.
>>>
>>> If the user only specified fid=0 while leaving uid unspecified,
>>> well, an address is going to be picked for them.
>> This would be an empty zpci address while parsing XML so that we might
>> pick a zpci address with non-zero fid. For example:
>>
>> dev1: fid=0 (this address would be treated as an unassigned zpci address)
>> dev2 (no definition for both fid and uid)
>>
>> Then the process of assigning addresses will iterate devices as type by
>> type.
>> If dev2 is processed before dev1, dev2's fid would be assigned by 0.
>> This causes
>> dev1's fid=1. The result doesn't match what the user wants.
> This would be the same as specifying a partial PCI address such as
>
>    <address type='pci' slot='0x00'/>
>
> and getting an address with slot != 0x00 back: surprising, perhaps,
> but that's the way it's been with PCI addresses forever so you can
> assume users are familiar with it by now.
>
> For zPCI addresses to be inconsistent with this behavior would be
> utterly confusing; moreover, if the user really needs the uid and
> fid to have certain values, all they have to do is specify both
> and libvirt will not attempt to override them.
>
I tried as your idea. It makes everything complicated, especially 
alloc/reserve/release
zpci address. If the user defines uid=1 and fid=0, we don't know whether 
should
reserve fid. (uid=1 fid=0) is the same with (uid=1).

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, 2018-08-22 at 17:39 +0800, Yi Min Zhao wrote:
> 在 2018/8/21 下午9:19, Andrea Bolognani 写道:
> > On Tue, 2018-08-21 at 19:55 +0800, Yi Min Zhao wrote:
> > > > > For zPCI address, if we use the same strategy as PCI part and user
> > > > > wants to assign fid=0 to the specific device, he might encounter
> > > > > assignment failure. At least, according to the doc, 0 shoud be a valid
> > > > > value to assign to fid.
> > > > 
> > > > If the user wants to use a specific zPCI address they can simply
> > > > specify both attributes, eg. uid=1,fid=0 will work just fine with
> > > > the proposed approach and won't produce errors or cause a new zPCI
> > > > address to be automatically assigned.
> > > > 
> > > > If the user only specified fid=0 while leaving uid unspecified,
> > > > well, an address is going to be picked for them.
> > > 
> > > This would be an empty zpci address while parsing XML so that we might
> > > pick a zpci address with non-zero fid. For example:
> > > 
> > > dev1: fid=0 (this address would be treated as an unassigned zpci address)
> > > dev2 (no definition for both fid and uid)
> > > 
> > > Then the process of assigning addresses will iterate devices as type by
> > > type.
> > > If dev2 is processed before dev1, dev2's fid would be assigned by 0.
> > > This causes
> > > dev1's fid=1. The result doesn't match what the user wants.
> > 
> > This would be the same as specifying a partial PCI address such as
> > 
> >    <address type='pci' slot='0x00'/>
> > 
> > and getting an address with slot != 0x00 back: surprising, perhaps,
> > but that's the way it's been with PCI addresses forever so you can
> > assume users are familiar with it by now.
> > 
> > For zPCI addresses to be inconsistent with this behavior would be
> > utterly confusing; moreover, if the user really needs the uid and
> > fid to have certain values, all they have to do is specify both
> > and libvirt will not attempt to override them.
> 
> I tried as your idea. It makes everything complicated, especially 
> alloc/reserve/release
> zpci address. If the user defines uid=1 and fid=0, we don't know whether 
> should
> reserve fid. (uid=1 fid=0) is the same with (uid=1).

You should reserve it. The user can either

  * not specify zPCI information at all: automatic assignment will
    be performed, no error should be reported;

  * specify a *full* zPCI address: manual assignment, will result
    in either that exact address being used or an error;

  * specify partial information: missing properties will default
    to zero, which will probably cause errors eg. if only the uid
    is specified for a bunch of devices.

The last option is less predictable so it should probably never be
used. All of the above is consistent with how the PCI part works.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

在 2018/8/22 下午6:09, Andrea Bolognani 写道:
> On Wed, 2018-08-22 at 17:39 +0800, Yi Min Zhao wrote:
>> 在 2018/8/21 下午9:19, Andrea Bolognani 写道:
>>> On Tue, 2018-08-21 at 19:55 +0800, Yi Min Zhao wrote:
>>>>>> For zPCI address, if we use the same strategy as PCI part and user
>>>>>> wants to assign fid=0 to the specific device, he might encounter
>>>>>> assignment failure. At least, according to the doc, 0 shoud be a valid
>>>>>> value to assign to fid.
>>>>> If the user wants to use a specific zPCI address they can simply
>>>>> specify both attributes, eg. uid=1,fid=0 will work just fine with
>>>>> the proposed approach and won't produce errors or cause a new zPCI
>>>>> address to be automatically assigned.
>>>>>
>>>>> If the user only specified fid=0 while leaving uid unspecified,
>>>>> well, an address is going to be picked for them.
>>>> This would be an empty zpci address while parsing XML so that we might
>>>> pick a zpci address with non-zero fid. For example:
>>>>
>>>> dev1: fid=0 (this address would be treated as an unassigned zpci address)
>>>> dev2 (no definition for both fid and uid)
>>>>
>>>> Then the process of assigning addresses will iterate devices as type by
>>>> type.
>>>> If dev2 is processed before dev1, dev2's fid would be assigned by 0.
>>>> This causes
>>>> dev1's fid=1. The result doesn't match what the user wants.
>>> This would be the same as specifying a partial PCI address such as
>>>
>>>     <address type='pci' slot='0x00'/>
>>>
>>> and getting an address with slot != 0x00 back: surprising, perhaps,
>>> but that's the way it's been with PCI addresses forever so you can
>>> assume users are familiar with it by now.
>>>
>>> For zPCI addresses to be inconsistent with this behavior would be
>>> utterly confusing; moreover, if the user really needs the uid and
>>> fid to have certain values, all they have to do is specify both
>>> and libvirt will not attempt to override them.
>> I tried as your idea. It makes everything complicated, especially
>> alloc/reserve/release
>> zpci address. If the user defines uid=1 and fid=0, we don't know whether
>> should
>> reserve fid. (uid=1 fid=0) is the same with (uid=1).
> You should reserve it. The user can either
>
>    * not specify zPCI information at all: automatic assignment will
>      be performed, no error should be reported;
>
>    * specify a *full* zPCI address: manual assignment, will result
>      in either that exact address being used or an error;
>
>    * specify partial information: missing properties will default
>      to zero, which will probably cause errors eg. if only the uid
>      is specified for a bunch of devices.
>
> The last option is less predictable so it should probably never be
> used. All of the above is consistent with how the PCI part works.
>
And then, zpci address instance could be a member of pci address structure?
I mean not using pointer at all?

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, 2018-08-23 at 14:40 +0800, Yi Min Zhao wrote:
> 在 2018/8/22 下午6:09, Andrea Bolognani 写道:
> > On Wed, 2018-08-22 at 17:39 +0800, Yi Min Zhao wrote:
> > > I tried as your idea. It makes everything complicated, especially
> > > alloc/reserve/release
> > > zpci address. If the user defines uid=1 and fid=0, we don't know whether
> > > should
> > > reserve fid. (uid=1 fid=0) is the same with (uid=1).
> > 
> > You should reserve it. The user can either
> > 
> >    * not specify zPCI information at all: automatic assignment will
> >      be performed, no error should be reported;
> > 
> >    * specify a *full* zPCI address: manual assignment, will result
> >      in either that exact address being used or an error;
> > 
> >    * specify partial information: missing properties will default
> >      to zero, which will probably cause errors eg. if only the uid
> >      is specified for a bunch of devices.
> > 
> > The last option is less predictable so it should probably never be
> > used. All of the above is consistent with how the PCI part works.
> 
> And then, zpci address instance could be a member of pci address structure?
> I mean not using pointer at all?

Exactly. You can either just add zpci_uid and zpci_fid to the
virPCIDeviceAddress struct, or have

  struct _virZPCIDeviceAddress {
      unsigned int uid;
      unsigned int fid;
  };

  struct _virPCIDeviceAddress {
      unsigned int domain;
      unsigned int bus;
      unsigned int slot;
      unsigned int function;
      int multi; /* virTristateSwitch */
      virZPCIDeviceAddress zpci;
  };

that is, have a separate struct for zPCI but *embed it* inside the
existing one instead of allocating it separately, the same way we
embed virPCIDeviceAddress itself virDomainDeviceInfo, for example.
This second option looks better to me.

This got me thinking that the extension flags *also* belongs to
virPCIDeviceAddress, since we need them to know whether the zPCI
part should be taken into account when formatting and such: you
used to check whether the zpci pointer was NULL, but of course you
can no longer do that once the pointer is gone; moreover, checking
for a flag is more explicit so that's also an improvement. Using
the flags stored into virDomainDeviceInfo would be ugly because it
would make it so virPCIDeviceAddress is no longer a stand-alone
object, so we should avoid that.

I'm not sure you can get away with not storing the extension flags
in virDomainDeviceInfo, though, because IIRC you might need to use
them *before* you have figured out that you're going to assign a
PCI address to the device. We might just have to store them twice,
which is not great but I think preferable to introducing a reverse
dependency from virPCIDeviceAddress to virDomainDeviceInfo. But I
haven't really looked too closely into it, so perhaps there's a
way to avoid that duplication after all :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

在 2018/8/23 下午4:12, Andrea Bolognani 写道:
> On Thu, 2018-08-23 at 14:40 +0800, Yi Min Zhao wrote:
>> 在 2018/8/22 下午6:09, Andrea Bolognani 写道:
>>> On Wed, 2018-08-22 at 17:39 +0800, Yi Min Zhao wrote:
>>>> I tried as your idea. It makes everything complicated, especially
>>>> alloc/reserve/release
>>>> zpci address. If the user defines uid=1 and fid=0, we don't know whether
>>>> should
>>>> reserve fid. (uid=1 fid=0) is the same with (uid=1).
>>> You should reserve it. The user can either
>>>
>>>     * not specify zPCI information at all: automatic assignment will
>>>       be performed, no error should be reported;
>>>
>>>     * specify a *full* zPCI address: manual assignment, will result
>>>       in either that exact address being used or an error;
>>>
>>>     * specify partial information: missing properties will default
>>>       to zero, which will probably cause errors eg. if only the uid
>>>       is specified for a bunch of devices.
>>>
>>> The last option is less predictable so it should probably never be
>>> used. All of the above is consistent with how the PCI part works.
>> And then, zpci address instance could be a member of pci address structure?
>> I mean not using pointer at all?
> Exactly. You can either just add zpci_uid and zpci_fid to the
> virPCIDeviceAddress struct, or have
>
>    struct _virZPCIDeviceAddress {
>        unsigned int uid;
>        unsigned int fid;
>    };
>
>    struct _virPCIDeviceAddress {
>        unsigned int domain;
>        unsigned int bus;
>        unsigned int slot;
>        unsigned int function;
>        int multi; /* virTristateSwitch */
>        virZPCIDeviceAddress zpci;
>    };
I have updated them as your recommendation in my new version.
> that is, have a separate struct for zPCI but *embed it* inside the
> existing one instead of allocating it separately, the same way we
> embed virPCIDeviceAddress itself virDomainDeviceInfo, for example.
> This second option looks better to me.
>
> This got me thinking that the extension flags *also* belongs to
> virPCIDeviceAddress, since we need them to know whether the zPCI
> part should be taken into account when formatting and such: you
> used to check whether the zpci pointer was NULL, but of course you
> can no longer do that once the pointer is gone; moreover, checking
> for a flag is more explicit so that's also an improvement. Using
> the flags stored into virDomainDeviceInfo would be ugly because it
> would make it so virPCIDeviceAddress is no longer a stand-alone
> object, so we should avoid that.
>
> I'm not sure you can get away with not storing the extension flags
> in virDomainDeviceInfo, though, because IIRC you might need to use
> them *before* you have figured out that you're going to assign a
> PCI address to the device. We might just have to store them twice,
> which is not great but I think preferable to introducing a reverse
> dependency from virPCIDeviceAddress to virDomainDeviceInfo. But I
> haven't really looked too closely into it, so perhaps there's a
> way to avoid that duplication after all :)
>
I think it's enough to store extension flags in virDomainDeviceInfo in 
my new code.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, 2018-08-23 at 16:46 +0800, Yi Min Zhao wrote:
> 在 2018/8/23 下午4:12, Andrea Bolognani 写道:
> > This got me thinking that the extension flags *also* belongs to
> > virPCIDeviceAddress, since we need them to know whether the zPCI
> > part should be taken into account when formatting and such: you
> > used to check whether the zpci pointer was NULL, but of course you
> > can no longer do that once the pointer is gone; moreover, checking
> > for a flag is more explicit so that's also an improvement. Using
> > the flags stored into virDomainDeviceInfo would be ugly because it
> > would make it so virPCIDeviceAddress is no longer a stand-alone
> > object, so we should avoid that.
> > 
> > I'm not sure you can get away with not storing the extension flags
> > in virDomainDeviceInfo, though, because IIRC you might need to use
> > them *before* you have figured out that you're going to assign a
> > PCI address to the device. We might just have to store them twice,
> > which is not great but I think preferable to introducing a reverse
> > dependency from virPCIDeviceAddress to virDomainDeviceInfo. But I
> > haven't really looked too closely into it, so perhaps there's a
> > way to avoid that duplication after all :)
> 
> I think it's enough to store extension flags in virDomainDeviceInfo in 
> my new code.

As explained above, that would cause virPCIDeviceAddress to no
longer be usable on its own, which I don't think is a good idea.

If you have to store the flags in virDomainDeviceInfo for address
allocation purposes that's fine, but once address allocation has
been performed you should no longer need to look at those and
should use virPCIDeviceAddress on its own instead.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

在 2018/8/23 下午4:12, Andrea Bolognani 写道:
> Exactly. You can either just add zpci_uid and zpci_fid to the
> virPCIDeviceAddress struct, or have
>
>    struct _virZPCIDeviceAddress {
>        unsigned int uid;
>        unsigned int fid;
>    };
>
>    struct _virPCIDeviceAddress {
>        unsigned int domain;
>        unsigned int bus;
>        unsigned int slot;
>        unsigned int function;
>        int multi; /* virTristateSwitch */
>        virZPCIDeviceAddress zpci;
>    };
There's an error in syntax-check. I think it's from common code.

src/util/virpci.h:47:    unsigned int uid;
maint.mk: use pid_t for pid, uid_t for uid, gid_t for gid

I think it mistakes zpci's uid.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, 2018-08-23 at 17:15 +0800, Yi Min Zhao wrote:
> 
> 在 2018/8/23 下午4:12, Andrea Bolognani 写道:
> > Exactly. You can either just add zpci_uid and zpci_fid to the
> > virPCIDeviceAddress struct, or have
> > 
> >    struct _virZPCIDeviceAddress {
> >        unsigned int uid;
> >        unsigned int fid;
> >    };
> > 
> >    struct _virPCIDeviceAddress {
> >        unsigned int domain;
> >        unsigned int bus;
> >        unsigned int slot;
> >        unsigned int function;
> >        int multi; /* virTristateSwitch */
> >        virZPCIDeviceAddress zpci;
> >    };
> 
> There's an error in syntax-check. I think it's from common code.
> 
> src/util/virpci.h:47:    unsigned int uid;
> maint.mk: use pid_t for pid, uid_t for uid, gid_t for gid
> 
> I think it mistakes zpci's uid.

Easy enough to fix: just apply the patch below, change the
struct definition to

    struct _virZPCIDeviceAddress {
        unsigned int uid; /* exempt from syntax-check */
        unsigned int fid;
    };

and it will go away :)


diff --git a/cfg.mk b/cfg.mk
index 609ae869c2..1116feb299 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -472,6 +472,7 @@ sc_prohibit_canonicalize_file_name:
 # Insist on correct types for [pug]id.
 sc_correct_id_types:
        @prohibit='\<(int|long) *[pug]id\>' \
+       exclude='exempt from syntax-check' \
        halt='use pid_t for pid, uid_t for uid, gid_t for gid' \
          $(_sc_search_regexp)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

在 2018/8/23 下午6:01, Andrea Bolognani 写道:
> On Thu, 2018-08-23 at 17:15 +0800, Yi Min Zhao wrote:
>> 在 2018/8/23 下午4:12, Andrea Bolognani 写道:
>>> Exactly. You can either just add zpci_uid and zpci_fid to the
>>> virPCIDeviceAddress struct, or have
>>>
>>>     struct _virZPCIDeviceAddress {
>>>         unsigned int uid;
>>>         unsigned int fid;
>>>     };
>>>
>>>     struct _virPCIDeviceAddress {
>>>         unsigned int domain;
>>>         unsigned int bus;
>>>         unsigned int slot;
>>>         unsigned int function;
>>>         int multi; /* virTristateSwitch */
>>>         virZPCIDeviceAddress zpci;
>>>     };
>> There's an error in syntax-check. I think it's from common code.
>>
>> src/util/virpci.h:47:    unsigned int uid;
>> maint.mk: use pid_t for pid, uid_t for uid, gid_t for gid
>>
>> I think it mistakes zpci's uid.
> Easy enough to fix: just apply the patch below, change the
> struct definition to
>
>      struct _virZPCIDeviceAddress {
>          unsigned int uid; /* exempt from syntax-check */
>          unsigned int fid;
>      };
>
> and it will go away :)
>
>
> diff --git a/cfg.mk b/cfg.mk
> index 609ae869c2..1116feb299 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -472,6 +472,7 @@ sc_prohibit_canonicalize_file_name:
>   # Insist on correct types for [pug]id.
>   sc_correct_id_types:
>          @prohibit='\<(int|long) *[pug]id\>' \
> +       exclude='exempt from syntax-check' \
>          halt='use pid_t for pid, uid_t for uid, gid_t for gid' \
>            $(_sc_search_regexp)
>
Thanks for your help! I will look into this. Never meet this before.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

在 2018/8/16 下午10:38, Andrea Bolognani 写道:
> On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
>> +typedef struct _virZPCIDeviceAddress virZPCIDeviceAddress;
>> +typedef virZPCIDeviceAddress *virZPCIDeviceAddressPtr;
>> +struct _virZPCIDeviceAddress {
>> +    unsigned int zpci_fid;
>> +    unsigned int zpci_uid;
>> +    bool fid_assigned;
>> +    bool uid_assigned;
>> +};
> A couple of questions about the approach here, one of which I have
> mentioned already and one of which I probably haven't (my bad):
>
>    * do you really need to have separate booleans tracking whether
>      or not either id has been assigned? Wouldn't the same approach
>      as virPCIDeviceAddressIsEmpty() work, eg. consider the address
>      absent if both are zero and present otherwise?
>
>    * especially if you don't need the additional booleans, would it
>      be preferable to just add the two ids to the existing struct
>      instead of declaring a new one that you'll have to allocate
>      and make sure it's not NULL before accessing it? Again, I seem
>      to remember Laine feeling somewhat strongly about the topic.
>
> Cosmetic issues:
Sorry, forgot this comment.
>
>    * uid should be listed before fid;
Sure.
>
>    * the zpci_ prefix is unnecessary if you have a separate struct
>      that contains "ZPCI" in the name. But see above :)
>
Let's see your response to another mail.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list