Re: [PATCH 1/4] s390x/pci: use a reserved ID for the default PCI group

[David Hildenbrand]

On 02.12.21 18:11, Matthew Rosato wrote:
> On 12/2/21 11:43 AM, David Hildenbrand wrote:
>> On 02.12.21 17:41, Matthew Rosato wrote:
>>> The current default PCI group being used can technically collide with a
>>> real group ID passed from a hostdev.  Let's instead use a group ID that 
>>> comes
>>> from a special pool that is architected to be reserved for simulated 
>>> devices.
>>>
>>> Fixes: 28dc86a072 ("s390x/pci: use a PCI Group structure")
>>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
>>> ---
>>>   include/hw/s390x/s390-pci-bus.h | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/hw/s390x/s390-pci-bus.h 
>>> b/include/hw/s390x/s390-pci-bus.h
>>> index aa891c178d..2727e7bdef 100644
>>> --- a/include/hw/s390x/s390-pci-bus.h
>>> +++ b/include/hw/s390x/s390-pci-bus.h
>>> @@ -313,7 +313,7 @@ typedef struct ZpciFmb {
>>>   } ZpciFmb;
>>>   QEMU_BUILD_BUG_MSG(offsetof(ZpciFmb, fmt0) != 48, "padding in ZpciFmb");
>>>   
>>> -#define ZPCI_DEFAULT_FN_GRP 0x20
>>> +#define ZPCI_DEFAULT_FN_GRP 0xFF
>>>   typedef struct S390PCIGroup {
>>>       ClpRspQueryPciGrp zpci_group;
>>>       int id;
>>>
>>
>> What happens if we migrate a VM from old to new QEMU? Won't the guest be
>> able to observe the change?
>>
> 
> Yes, technically --  But # itself is not really all that important, it 
> is provided from CLP Q PCI FN to be subsequently used as input into Q 
> PCI FNGRP -- With the fundamental notion being that all functions that 
> share the same group # share the same group CLP info.  Whether the 
> number is, say, 1 or 5 doesn't matter so much.
> 
> However..  0xF0 and greater are the only values reserved for hypervisor 
> use.  By using 0x20 we run the risk of accidentally conflating simulated 
> devices and real hardware, hence the desire to change it.
> 
> Is your concern about a migrated guest with a virtio device trying to do 
> a CLP QUERY PCI FNGRP using 0x20 on a new QEMU?  I suppose we could 
> modify 'clp_service_call, case CLP_QUERY_PCI_FNGRP' to silently catch 
> simulated devices trying to use something other than the default group, 
> e.g.:
> 
> if ((pbdev->fh & FH_SHM_EMUL) &&
>      (pbdev->zpci_fn.pfgid != ZPCI_DEFAULT_FN_GRP)) {
>          /* Simulated device MUST have default group */
>       pbdev->zpci_fn.pfgid = ZPCI_DEFAULT_FN_GRP;
>       group = s390_group_find(ZPCI_DEFAULT_FN_GRP);
> }
> 
> What do you think?
> 

Good question, I'm certainly not a zPCI expert. However if you think
that we cannot really break migration in a sane use case, I'm fine with
it as well :)

The question about breaking migration just popped up in my head when I
stumbled over this patch.

-- 
Thanks,

David / dhildenb