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

[Boris Fiuczynski]

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.

-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294