[PATCH libvirt v1 0/6] Fix ZPCI address auto-generation on s390

[Shalini Chellathurai Saroja]

Hi Andrea,

Ping, in case you missed it.

On 4/20/20 9:55 PM, Boris Fiuczynski wrote:
> On 4/10/20 2:06 PM, Andrea Bolognani wrote:
>> On Thu, 2020-04-09 at 12:30 +0200, Shalini Chellathurai Saroja wrote:
>>> The ZPCI address validation or autogeneration does not work as
>>> expected in the following scenarios
>>> 1. uid = 0 and fid = 0
>>> 2. uid = 0 and fid > 0
>>> 3. uid = 0 and fid not specified
>>> 4. uid not specified and fid > 0
>>> 5. 2 ZPCI devices with uid > 0 and fid not specified.
>>>
>>> This is because of the following reasons
>>> 1. If uid = 0 or fid = 0 the code assumes that user has not specified
>>>     the corresponding address
>>> 2. If either uid or fid is provided, the code assumes that both uid
>>>     and fid addresses are specified by the user.
>>
>> I'd have to dig up the old threads, but based on what I remember the
>> behaviors you describe are entirely intentional.
>>
>> For PCI addresses, setting all parts of the address to zero or not
>> setting it at all is equivalent, and we wanted to be consistent with
>> that behavior for ZPCI; additionally, zero is not a valid value for
>> uid so of course neither is the address uid=0 fid=0, which means that
>> we're not preventing the user from specifying a valid address by
>> conflating the all-zero address with the unspecified address.
>>
>> For partially-specified addresses, the behavior is also the same as
>> PCI: any part you don't specify is considered to be zero, which
>> results in
>>
>>    uid=0 fid=0 -> uid=0 fid=0 -> address gets autogenerated
>>    uid=0 fid=x -> uid=0 fid=x -> address is rejected as invalid
>>    uid=0       -> uid=0 fid=0 -> address gets autogenerated
>>          fid=x -> uid=0 fid=x -> address is rejected as invalid
>>    uid=x       -> uid=x fid=0 -> address is accepted
>>
>> So, just like for PCI addresses, you have basically two reasonable
>> options: either don't specify any zPCI address and leave allocation
>> entirely up to libvirt, or specify all of the addresses completely:
>> anything in between will likely not work as you'd expect or want.
>>
>> Again, this is based purely on my recollection of design discussions
>> that happened one and a half years ago, so I might have gotten some
>> of the details wrong - in which case by all means call me out on
>> that O:-)
>>
> Hi Andrea,
> sorry for the delayed answer. I (and some others as well) lost some 
> emails on my IMAP account and I just found your answer today.
> I can remember that you had a discussion with the original author of 
> the zpci code. There are a few issues with the currently implemented 
> "rules" which partially are not even working as you outlined above in 
> more complex scenarios.
>
> First: Setting uid=0 or uid='0x0000'
> The architecture allows to do that BUT if you do than you are NOT 
> using the uid mode which results for the guest OS to use the legacy 
> mode for assigning PCI addresses starting with 0 increasing by one 
> following an unpredictable order by which the pci device are presented 
> to guest OS. Since we never ever wanted to support the legacy mode in 
> KVM guests we decided to never allow uid=0. Allowing the uid to be set 
> to 0 is a contradiction.
> Actually the user can also set uid='0x0000' which I consider very 
> specific and one would end up with something like uid='0x0001' and 
> even more confusing is that suddenly setting uid='0x0000' on more than 
> one PCI device is allowed.
>
> Besides that the current zpci code still contains at least one flaw 
> that is simply caused by the fact that there is no knowledge about 
> which value was specified by the user.
> In Shalini's and your list it is case 5: This scenario runs into 
> errors when another PCI device already has a fid set to 0 OR another 
> PCI device exists specified with a uid > 0 and without a fid. The user 
> gets the error message for something he did not specify:
>  error: Failed to define domain from pci-addr-test.xml
>  error: internal error: zPCI fid 0 is already reserved
>
> Regarding setting fid=0 or fid='0x00000000'
> Since it is a legal value for fid specifying it should not be 
> considered as a wildcard or set equivalent to not specifying it at all.
> Doing so things like this happen and for the user it certainly seems 
> like a bug:
> Specify this in the domain:
>   pcidev1: uid='0x0000' fid='0x00000000'
>   pcidev2: uid='0x0000'
> Results in a defined domain:
>   pcidev1: uid='0x0002' fid='0x00000001'
>   pcidev2: uid='0x0001' fid='0x00000000'
> Another example:
> Specify this in the domain:
>   pcidev1: fid='0x00000000'
>   pcidev2: fid='0x00000000'
> Results in a defined domain:
>   pcidev1: uid='0x0002' fid='0x00000001'
>   pcidev2: uid='0x0001' fid='0x00000000'
>  BUT
> Specify this in the domain:
>   pcidev1: uid='0x0002' fid='0x00000000'
>   pcidev2: uid='0x0001' fid='0x00000000'
> Results in error:
>   error: Failed to define domain from pci-addr-test.xml
>   error: internal error: zPCI fid 0 is already reserved
> (Btw remove one of the fids results in the flaw described above.)
>
>
> I think that Shalini's patch series improves the zpci address 
> generation to better meet the users expected behavior. It also removes 
> a correlation between uid and fid that does not really exist.
>